Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zed-editor: fix darwin #329653

Merged
merged 2 commits into from
Oct 21, 2024
Merged

zed-editor: fix darwin #329653

merged 2 commits into from
Oct 21, 2024

Conversation

niklaskorz
Copy link
Contributor

@niklaskorz niklaskorz commented Jul 24, 2024

Description of changes

Fixes #320084.

Once zed-industries/zed#13343 lands, building Zed on macOS becomes much easier as we do not have to deal with building a separate Swift library that Zed depends on anymore.

As zed-industries/zed#13343 seems to need much longer to implement than I originally anticipated, I updated this PR to disable livekit until this change lands, so we can already go ahead and release darwin support without it, using the same internal fallbacks the Linux version is using.

As this PR is making use of the new pattern for using macOS SDKs, it's targetting staging-next, so merging this PR will have to wait until staging-next has been merged into master (and this PR has been retargetted at master).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 6.topic: qt/kde 6.topic: kernel The Linux kernel 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: printing 6.topic: rust 6.topic: golang 6.topic: ruby 6.topic: vim 6.topic: stdenv Standard environment 6.topic: nodejs 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: lua 6.topic: systemd 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: dotnet Language: .NET labels Jul 24, 2024
@niklaskorz niklaskorz changed the base branch from master to staging-next July 24, 2024 13:38
@github-actions github-actions bot removed 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 6.topic: qt/kde 6.topic: kernel The Linux kernel 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 24, 2024
@ofborg ofborg bot requested a review from GaetanLepage October 18, 2024 15:55
@niklaskorz niklaskorz force-pushed the zed-darwin branch 2 times, most recently from 3d7a223 to b988f96 Compare October 18, 2024 16:40
@niklaskorz niklaskorz marked this pull request as ready for review October 19, 2024 21:03
@@ -93,14 +95,23 @@ rustPlatform.buildRustPackage rec {
repo = "zed";
rev = "refs/tags/v${version}";
hash = "sha256-xtSdlzj1AxhJN4aXLJ+Oy51LX4QduLwcuCfK42kthvE=";
fetchSubmodules = true;
Copy link
Contributor Author

@niklaskorz niklaskorz Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have become obsolete for a while now for the Zed repository, so I disabled fetching submodules, but correct me if I'm wrong.

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 329653


x86_64-linux

✅ 1 package built:
  • zed-editor

aarch64-linux

✅ 1 package built:
  • zed-editor

x86_64-darwin

❌ 1 package failed to build:
  • zed-editor

aarch64-darwin

❌ 1 package failed to build:
  • zed-editor

@niklaskorz
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 329653

x86_64-linux

✅ 1 package built:

aarch64-linux

✅ 1 package built:

x86_64-darwin

❌ 1 package failed to build:

aarch64-darwin

❌ 1 package failed to build:

Could you provide any logs for the failing builds? As it builds fine on aarch64-darwin here. I'll try x86_64-darwin in a moment.

@GaetanLepage
Copy link
Contributor

Could you provide any logs for the failing builds? As it builds fine on aarch64-darwin here. I'll try x86_64-darwin in a moment.

Actually I naively ran the tool and didn't notice that the PR was targetting staging.
I think it failed on darwin because cargo wasn't building.

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Oct 20, 2024

Could you provide any logs for the failing builds? As it builds fine on aarch64-darwin here. I'll try x86_64-darwin in a moment.

Actually I naively ran the tool and didn't notice that the PR was targetting staging. I think it failed on darwin because cargo wasn't building.

I see! Yeah, it takes a good while because Rust and Cargo 1.82 are not cached yet on staging-next, but both should build fine :)

I'm still compiling x86_64-darwin:

┏━ Dependency Graph:
┃    ┌─ ⏸ xim-parser-0.2.1-x86_64-darwin waiting for 1 ⏵
┃    │           ┌─ ⏸ cargo-build-hook.sh-x86_64-darwin waiting for 1 ⏵
┃    │           │           ┌─ ⏵ llvm-18.1.8-x86_64-darwin (buildPhase) ⏱ 15m39s
┃    │           │        ┌─ ⏸ rustc-1.82.0-x86_64-darwin
┃    │           │     ┌─ ⏸ rustc-wrapper-1.82.0-x86_64-darwin
┃    │           │  ┌─ ⏸ cargo-1.82.0-x86_64-darwin
┃    │           ├─ ⏸ cargo-check-hook.sh-x86_64-darwin
┃    │        ┌─ ⏸ cargo-auditable-0.6.2-x86_64-darwin
┃    │     ┌─ ⏸ auditable-cargo-bootstrap-1.81.0-x86_64-darwin
┃    │  ┌─ ⏸ cargo-1.82.0-x86_64-darwin
┃    ├─ ⏸ xim-ctext-0.3.0-x86_64-darwin
┃ ┌─ ⏸ cargo-vendor-dir-x86_64-darwin
┃ ⏸ zed-editor-0.157.5-x86_64-darwin

Edit: Took quite a while to compile through Rosetta, but finished now.

┃ ✔ zed-editor-0.157.5-x86_64-darwin ⏱ 1h3m58s
┣━━━ Builds            │ Downloads          │ Host
┃        │ ✔ 330 │     │     │        │     │ localhost
┃        │       │     │     │ ↓ 1308 │     │ https://cache.nixos.org
┗━ ∑ ⏵ 0 │ ✔ 330 │ ⏸ 0 │ ↓ 0 │ ↓ 1308 │ ⏸ 0 │ Finished at 13:55:58 after 3h24m47s

$ file result/Applications/Zed.app/Contents/MacOS/zed
result/Applications/Zed.app/Contents/MacOS/zed: Mach-O 64-bit executable x86_64
grafik grafik

@GaetanLepage
Copy link
Contributor

Good job on this @niklaskorz !
Do you want to merge this against staging-next or wait that it lands on master first and then merge it on master ?

@@ -191,6 +195,9 @@ rustPlatform.buildRustPackage rec {
ZED_UPDATE_EXPLANATION = "zed has been installed using nix. Auto-updates have thus been disabled.";
Copy link
Contributor

@jansol jansol Oct 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ZED_UPDATE_EXPLANATION = "zed has been installed using nix. Auto-updates have thus been disabled.";
ZED_UPDATE_EXPLANATION = "Zed has been installed using Nix. Auto-updates have thus been disabled.";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but I agree that looks nicer, will add it as a separate commit :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, hence not an actual review :D It just caught my eye on the above screenshots

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Oct 20, 2024

Good job on this @niklaskorz ! Do you want to merge this against staging-next or wait that it lands on master first and then merge it on master ?

I am fine with both. On the "Nix on macOS" Matrix room I asked what the usual procedure was and @khaneliman answered:

If there's a dependency on something in staging/staging-next I don't think it's necessarily wrong to target them if you don't want to wait until it propagates down. I think there's a chance stuff gets ripped out or reverted in those branches, though so it might make sense to wait until it gets to master. Unless, you're more involved in the staging flow.

But I don't think there is any chance at this point that the new Apple SDK and darwin minimum version hook get removed from staging-next, as they were a long time coming before they were merged into staging. And these are the only two things from staging-next this PR relies on. Also @emilazy is already aware of the NIX_CFLAGS_COMPILE workaround in this PR for the currently disabled -isysroot, so she knows where to find and remove it once #349555 progresses.

@GaetanLepage
Copy link
Contributor

Good job on this @niklaskorz ! Do you want to merge this against staging-next or wait that it lands on master first and then merge it on master ?

I am fine with both. On the "Nix on macOS" Matrix room I asked what the usual procedure was and @khaneliman answered:

If there's a dependency on something in staging/staging-next I don't think it's necessarily wrong to target them if you don't want to wait until it propagates down. I think there's a chance stuff gets ripped out or reverted in those branches, though so it might make sense to wait until it gets to master. Unless, you're more involved in the staging flow.

But I don't think there is any chance at this point that the new Apple SDK and darwin minimum version hook get removed from staging-next, as they were a long time coming before they were merged into staging. And these are the only two things from staging-next this PR relies on. Also @emilazy is already aware of the NIX_CFLAGS_COMPILE workaround in this PR for the currently disabled -isysroot, so she knows where to find and remove it once #349555 progresses.

Ok, let's go for it then :)

@ofborg ofborg bot requested a review from GaetanLepage October 20, 2024 22:30
@adamcstephens adamcstephens merged commit 67d8538 into NixOS:staging-next Oct 21, 2024
27 of 28 checks passed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/title-the-darwin-sdks-have-been-updated/55295/1

@niklaskorz niklaskorz deleted the zed-darwin branch December 20, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: zed-editor
9 participants