-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
codesign_allocate: use absolute path #238323
Conversation
cb8c942
to
e4e41c6
Compare
I force-pushed over the previous changes as I found a better solution. We can just push down |
e4e41c6
to
ce298c4
Compare
ce298c4
to
05d2369
Compare
Should go to staging since it is an aarch64-darwin stdenv rebuild—aarch64-darwin is built as part of the normal jobsets now even though ofborg doesn't seem to care that much? |
Took a few hours, but I can confirm these two build now:
|
This looks sensible to me. Does postLinkSignHook still need to be passed down after #240433? |
It's still unprefixed so it will likely cause issues sooner or later. I'll check if the postLinkSignHook override is still necessary. |
Looks like the changes in #240433 broke cross-compilation from
Not that I need this, but this was one of the sanity checks for this MR. |
Good catch, it might be fixed by #242202 (once it's fixed up). |
I think the hook override is probably still necessary if we don't want to alter in what stages I did a quick staging:
with hook override in stage1:
without hook override in stage1:
As you can see I don't really have a good enough understanding of the darwin bootstrap process to tell if this is good or not. My intention with this PR is just to make the |
05d2369
to
2cab8be
Compare
I rebased the branch on |
@tjni Actually I can confirm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work!
I'll CC @NixOS/darwin-maintainers too in case anyone can take a look.
The definition of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stdenv changes lgtm. postLinkSignHook probably should be included in the assertions, but it’s not a big deal if it’s not (because sigtool is already included).
I’m currently on vacation so I can’t make the change for a while. Feel free to add it though.
|
2cab8be
to
e8ff6a8
Compare
e8ff6a8
to
8e912fe
Compare
@reckenrode @tjni @sternenseemann could you guys have another look at this? I don't want to rebase and resolve conflicts again. As for the assertions, I'm not sure what exactly to check for. That stage 2 uses postLinkSignHook from stage 1? |
Thank you for sticking with this. It still LGTM. @wegank can we leave another day for pinged reviewers to take a look and then merge this if no additional discussion takes place? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also LGTM. I dug into things a bit. I’m pretty sure isFromNixpkgs
would be true at every stage, so adding postLinkSignHook
to the assertions would be of limited value.
I quickly tried to test #148189 but ran into an unrelated issue (ccache tests failing), and I don't have the energy right now to try to set it up properly. Unfortunately, testing this right now for anyone else will require building a ton of things on It doesn't look entirely related to #154203 but I am also not sure. |
Description of changes
Use absolute reference to
codesign_allocate
. This should fix the issue in a more reliable way so there shouldn't be a case where this still props up.Issues:
#154203
#148189
Previous attempts:
#148282
#208120
Fixed writers-only:
#220966
To reproduce the issue, run
nix build .#pkgsCross.aarch64-multiplatform.freetype
. Works fine after applying these changes.Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)