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

codesign_allocate: use absolute path #238323

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

szlend
Copy link
Member

@szlend szlend commented Jun 17, 2023

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.

> libc++abi: terminating with uncaught exception of type std::runtime_error: Failed to spawn codesign_allocate: No such file or directory
       > /nix/store/apxhw055i0z86i3m27xn81w0qg2ssp1q-post-link-sign-hook: line 2: 50824 Abort trap: 6  

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
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jun 17, 2023
@szlend szlend marked this pull request as ready for review June 17, 2023 21:01
pkgs/top-level/darwin-packages.nix Outdated Show resolved Hide resolved
pkgs/build-support/writers/default.nix Outdated Show resolved Hide resolved
@szlend
Copy link
Member Author

szlend commented Jun 18, 2023

I force-pushed over the previous changes as I found a better solution. We can just push down postLinkSignHook from stage0 to later stages and that solves the bootstrap build errors. I looked at the derivation dependency tree and everything looks ok to me. It will take a few hours to rebuild and confirm that this is working as expected but I'm fairly confident this will work.

@szlend szlend force-pushed the fixed-path-codesign-allocate branch from e4e41c6 to ce298c4 Compare June 18, 2023 09:03
@sternenseemann
Copy link
Member

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?

@szlend szlend changed the base branch from master to staging June 18, 2023 12:43
@szlend
Copy link
Member Author

szlend commented Jun 18, 2023

Took a few hours, but I can confirm these two build now:

# freetype from aarch64-darwin to aarch64-linux
nix build .#legacyPackages.aarch64-darwin.pkgsCross.aarch64-multiplatform.freetype

# hello from x86_64-darwin to aarch64-darwin
nix build .#legacyPackages.x86_64-darwin.pkgsCross.aarch64-darwin.hello

@tjni
Copy link
Contributor

tjni commented Jul 8, 2023

This looks sensible to me. Does postLinkSignHook still need to be passed down after #240433?

@szlend
Copy link
Member Author

szlend commented Jul 8, 2023

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.

@szlend
Copy link
Member Author

szlend commented Jul 8, 2023

Looks like the changes in #240433 broke cross-compilation from x86_64-darwin to aarch64-darwin:

$ nix build .#legacyPackages.x86_64-darwin.pkgsCross.aarch64-darwin.hello
error: assertion '((binutils-unwrapped).targetPrefix == (cctools).targetPrefix)' failed

at /nix/store/3x2d8jqa63skr78pmnfjfyksi464lh3i-source/pkgs/os-specific/darwin/binutils/default.nix:5:1:

     4| # with the same targetPrefix.
     5| assert binutils-unwrapped.targetPrefix == cctools.targetPrefix;
     | ^
     6|

Not that I need this, but this was one of the sanity checks for this MR.

@tjni
Copy link
Contributor

tjni commented Jul 8, 2023

Good catch, it might be fixed by #242202 (once it's fixed up).

@szlend
Copy link
Member Author

szlend commented Jul 8, 2023

I think the hook override is probably still necessary if we don't want to alter in what stages cctools is referenced from.

I did a quick nix-tree check to see cctools-port references in staging, with hook override and without hook override.

staging:

cctools-port ref 1 -> stdenv-darwin
cctools-port ref 2 -> bootstrap-stage-xclang-stdenv-darwin
cctools-port ref 3 -> cctools-binutils-darwin-wrapper
cctools-port ref 4 -> bootstrap-stage1-sysctl-stdenv-darwin

with hook override in stage1:

cctools-port ref 1 -> cctools-binutils-darwin-wrapper
cctools-port ref 2 -> bootstrap-stage-xclang-stdenv-darwin
cctools-port ref 3 -> stdenv-darwin
cctools-port ref 4 -> bootstrap-stage1-sysctl-stdenv-darwin

without hook override in stage1:

cctools-port ref 1 -> cctools-binutils-darwin-wrapper
cctools-port ref 2 -> stdenv-darwin
cctools-port ref 3 -> bootstrap-stage2-CF-stdenv-darwin
cctools-port ref 4 -> bootstrap-stage2-CF-stdenv-darwin

As you can see staging and with hook override are equivalent, while cctools in without hook override was moved to bootstrap-stage2-CF-stdenv-darwin.

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 cctools reference the store path, without changing existing bootstrapping behavior.

@szlend szlend force-pushed the fixed-path-codesign-allocate branch from 05d2369 to 2cab8be Compare July 8, 2023 09:44
@szlend
Copy link
Member Author

szlend commented Jul 8, 2023

I rebased the branch on staging and the cctools-port references look ok to me in nix-tree, however I'm not able to test this because cross-compilation in darwin is broken right now.

@szlend
Copy link
Member Author

szlend commented Jul 8, 2023

@tjni Actually I can confirm nix build .#legacyPackages.aarch64-darwin.pkgsCross.aarch64-multiplatform.freetype builds, so this is ready for review.

@szlend szlend requested a review from tjni July 8, 2023 13:25
Copy link
Contributor

@tjni tjni left a 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.

@reckenrode
Copy link
Contributor

reckenrode commented Jul 16, 2023

The definition of targetPrefix was wrong in cctools-llvm. That’s why cross-compilation was failing. It was fixed in #242316.

Copy link
Contributor

@reckenrode reckenrode left a 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).

@szlend
Copy link
Member Author

szlend commented Jul 17, 2023 via email

@szlend szlend force-pushed the fixed-path-codesign-allocate branch from 2cab8be to e8ff6a8 Compare August 1, 2023 19:26
@szlend szlend force-pushed the fixed-path-codesign-allocate branch from e8ff6a8 to 8e912fe Compare August 1, 2023 19:27
@szlend
Copy link
Member Author

szlend commented Aug 1, 2023

@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?

@szlend szlend requested review from tjni and reckenrode August 1, 2023 19:32
@tjni
Copy link
Contributor

tjni commented Aug 1, 2023

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?

Copy link
Contributor

@reckenrode reckenrode left a 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.

@ncfavier
Copy link
Member

ncfavier commented Aug 4, 2023

Can #148189 and #154203 be closed now?

@tjni
Copy link
Contributor

tjni commented Aug 4, 2023

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 staging (it took me about 3-4 hours), so perhaps we can wait until this gets built in staging-next before asking for help testing it.

It doesn't look entirely related to #154203 but I am also not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants