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

zig: fix darwin SDK lookup #376427

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

niklaskorz
Copy link
Contributor

@niklaskorz niklaskorz commented Jan 24, 2025

Building Zig packages on darwin can currently fail because of the hardcoded /usr/bin/xcrun call in Zig's macOS SDK lookup method (ziglang/zig#22600).

https://github.com/ziglang/zig/blob/f3c29dcb247be5e50d2d05f9fb2e814ebf115a86/lib/std/zig/system/darwin.zig#L51

This is fixed by replacing it with just xcrun, which will then use the xcrun provided by the nixpkgs darwin stdenv. I decided against using an absolute nix store path for xcrun as replacement as @reckenrode raised concerns about that causing issues in Nix development shells.

Zig also calls /usr/bin/xcode-select, which I have replaced by xcode-select in the xcbuild package. In this case, the absolute path to the nix store is acceptable even for devShells as Zig discards the output of xcode-select and just checks if the command is successful:

https://github.com/ziglang/zig/blob/f77e1b86225cd49c2d04dfa6ca4a7ede315dc0b1/lib/std/zig/system/darwin.zig#L18

I also removed the by now rather old Zig versions 0.9 and 0.10 (not completely arbitrarily, they are the only releases marked as "pre-release" on https://github.com/ziglang/zig/releases and had either no or only one dependant in nixpkgs). For zig_0_9, there was a single dependant colorstorm which already is Zig 0.13 compatible in its HEAD state. I opened an issue requesting a new tagged release for colorstorm as the last one is three years old, and in the meanwhile bumped colorstorm to the commit bringing Zig 0.13 compatibility. This part has been moved to a separate PR: #376540

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jan 24, 2025
@nix-owners nix-owners bot requested review from andrewrk and figsoda January 24, 2025 15:54
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Jan 24, 2025
@niklaskorz niklaskorz marked this pull request as ready for review January 24, 2025 15:56
@niklaskorz niklaskorz mentioned this pull request Jan 24, 2025
13 tasks
@niklaskorz
Copy link
Contributor Author

niklaskorz commented Jan 24, 2025

Zig 0.11 still fails to build in the sandbox, looking into it

Edit: this was fixed by #320374 but the PR only addresses 0.12 and 0.13, so I'm adding this change to 0.11 as well

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Jan 24, 2025

pkgs/development/compilers/zig/generic.nix appears to be completely unused since #314987 but I don't intend to address whether this file should be removed or all three remaining Zig versions can be combined into one package definition in this PR. Instead, I just applied the darwin sandbox fixes to generic.nix too.

Edit: This is finalized by #331011

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Jan 24, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr --build-args "--option sandbox true" 376427


aarch64-darwin

⏩ 6 packages marked as broken and skipped:
  • cyber
  • ghostty
  • ghostty.man
  • ghostty.shell_integration
  • ghostty.terminfo
  • ghostty.vim
❌ 1 package failed to build:
  • cargo-lambda
✅ 26 packages built:
  • arocc
  • aroccPackages.latest-unwrapped
  • aroccStdenv
  • cargo-zigbuild
  • clipbuzz
  • colorstorm
  • dt
  • emacsPackages.zig-mode
  • findup
  • flow-editor
  • glsl_analyzer
  • linuxwave
  • minizign
  • ncdu
  • superhtml
  • zf
  • zig
  • zig.doc
  • zigStdenv
  • zig_0_11
  • zig_0_11.doc
  • zig_0_12
  • zig_0_12.doc
  • zls
  • zon2nix
  • ztags

@niklaskorz niklaskorz requested a review from a team January 24, 2025 18:27
@Samasaur1
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 376427


aarch64-darwin

⏩ 6 packages marked as broken and skipped:
  • cyber
  • ghostty
  • ghostty.man
  • ghostty.shell_integration
  • ghostty.terminfo
  • ghostty.vim
✅ 27 packages built:
  • arocc
  • aroccPackages.latest-unwrapped
  • aroccStdenv
  • cargo-lambda
  • cargo-zigbuild
  • clipbuzz
  • colorstorm
  • dt
  • emacsPackages.zig-mode
  • findup
  • flow-editor
  • glsl_analyzer
  • linuxwave
  • minizign
  • ncdu
  • superhtml
  • zf
  • zig
  • zig.doc
  • zigStdenv
  • zig_0_11
  • zig_0_11.doc
  • zig_0_12
  • zig_0_12.doc
  • zls
  • zon2nix
  • ztags

@Samasaur1
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 376427


x86_64-darwin

⏩ 6 packages marked as broken and skipped:
  • cyber
  • ghostty
  • ghostty.man
  • ghostty.shell_integration
  • ghostty.terminfo
  • ghostty.vim
✅ 27 packages built:
  • arocc
  • aroccPackages.latest-unwrapped
  • aroccStdenv
  • cargo-lambda
  • cargo-zigbuild
  • clipbuzz
  • colorstorm
  • dt
  • emacsPackages.zig-mode
  • findup
  • flow-editor
  • glsl_analyzer
  • linuxwave
  • minizign
  • ncdu
  • superhtml
  • zf
  • zig
  • zig.doc
  • zigStdenv
  • zig_0_11
  • zig_0_11.doc
  • zig_0_12
  • zig_0_12.doc
  • zls
  • zon2nix
  • ztags

@github-actions github-actions bot removed the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jan 24, 2025
@github-actions github-actions bot removed 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Jan 24, 2025
@niklaskorz niklaskorz changed the title zig: fix darwin SDK lookup and remove old versions zig: fix darwin SDK lookup Jan 24, 2025
@github-actions github-actions bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 labels Jan 24, 2025
@Eveeifyeve
Copy link
Contributor

This fixes the ghostty on darwin incident see here for context: #369788

@drupol
Copy link
Contributor

drupol commented Jan 25, 2025

Looks like the CI is broken

@emilazy
Copy link
Member

emilazy commented Jan 25, 2025

This fixes the ghostty on darwin incident see here for context: #369788

This has absolutely nothing to do with that.

substituteInPlace lib/std/zig/system/darwin.zig \
--replace-fail /usr/bin/xcrun xcrun \
--replace-fail /usr/bin/xcode-select '${lib.getExe' xcbuild "xcode-select"}'
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that this is duplicated three times over the span of three compilers. This is something unlikely to change in the future compilers as well, so this will continue to be duplicated in 0.14, 0.15, ...

Copy link
Contributor

@WeetHet WeetHet Jan 25, 2025

Choose a reason for hiding this comment

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

This also applies to code above, but I don't know how you can select a range of lines in GitHub PR reviews

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already addressed by #331011

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing, I didn't know about that PR, sorry :(

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Jan 25, 2025

Looks like the CI is broken

Zig 0.11 for Linux is broken on master as well, so it's not caused by the changes of this PR:

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 6.topic: zig 10.rebuild-darwin: 11-100 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants