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

apple-sdk: only rewrite old SDK paths #353383

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

reckenrode
Copy link
Contributor

Rewriting SDK paths is only needed when a text-based stub is an umbrella framework that does not include the contents of the stubs for the libraries re-exported by the framework. This is only the case for older SDKs. Once macOS introduced the dyld cache (and removed system dylibs) in 11.0, all umbrella frameworks in the SDK include the contents of their re-exported stubs.

While rewriting newer SDKs is mostly harmless, it breaks Zig, which tries to interpret the paths relative to the SDKROOT.

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.

Rewriting SDK paths is only needed when a text-based stub is an umbrella
framework that does not include the contents of the stubs for the
libraries re-exported by the framework. This is only the case for older
SDKs. Once macOS introduced the dyld cache (and removed system dylibs)
in 11.0, all umbrella frameworks in the SDK include the contents of
their re-exported stubs.

While rewriting newer SDKs is mostly harmless, it breaks Zig, which
tries to interpret the paths relative to the SDKROOT.
@nix-owners nix-owners bot requested review from emilazy and toonn November 3, 2024 13:57
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 1-10 labels Nov 4, 2024
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 4, 2024
@nixos-discourse
Copy link

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

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

Copy link
Contributor

@adamcstephens adamcstephens left a comment

Choose a reason for hiding this comment

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

Successfully built zig using this on aarch64-darwin.

@reckenrode reckenrode merged commit b15dd30 into NixOS:staging Nov 4, 2024
33 of 34 checks passed
@reckenrode reckenrode deleted the push-zpzxzvznovzq branch November 4, 2024 22:18
@emilazy emilazy mentioned this pull request Nov 5, 2024
@cpick
Copy link

cpick commented Nov 7, 2024

It looks like zig.x86_64-darwin has been removed from the latest staging-next eval jobset. I'm still learning how jobs are built on the staging -> staging-next -> trunk pipeline so I may be misinterpreting, but I wanted to make sure this removal was intentional?

@reckenrode
Copy link
Contributor Author

This change isn’t in staging-next yet. I assume Zig will be built in the last (before 24.11) cycle coming up.

@cpick
Copy link

cpick commented Nov 7, 2024

Thank you for your reply @reckenrode (here and elsewhere), it's helping me learn how the system works!
I based my assumption off of:

$ git branch --remote --contains e166c9bb94431551b3ac4c0674261144171e5775
  origin/emily/push-nkyqpvqvxwzn
  origin/llvm-19
  origin/staging
  origin/staging-next

And the fact that #354201 was created a few days after this change was merged into staging?

I have a hard time navigating Hydra, but I think the latest evaluation of nixpkgs:staging-next is being built off of https://github.com/NixOS/nixpkgs.git revision 127a31c
which contained this change:

$ git merge-base --is-ancestor e166c9bb94431551b3ac4c0674261144171e5775 127a31c3cf6e9ed385392357d72a65bbd5afe8b0 ; echo $?
0

And I think that's the evaluation that's reporting that zig.x86_64-darwin has been removed from?

@reckenrode
Copy link
Contributor Author

Oh, sorry. I hadn’t realized they started the last cycle. I thought they were still on the last 24.05 cycle. 😅

I don’t know why it wouldn’t be part of the evaluation. It seems like it should be. I asked in #staging:nixos.org to see if someone there knows.

@reckenrode
Copy link
Contributor Author

Thanks to @lilyinstarlight for the pointer: it was removed because of a Python eval error on staging-next. Once that’s fixed, Zig and a bunch of other stuff that got dropped should be part of the eval after that on staging-next.

@emilazy
Copy link
Member

emilazy commented Nov 7, 2024

Should be fixed now. Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants