Skip to content

Comments

darwin: remove -arch from cc/ld wrapper#354511

Merged
emilazy merged 1 commit intoNixOS:llvm-19from
paparodeo:no-arch
Nov 10, 2024
Merged

darwin: remove -arch from cc/ld wrapper#354511
emilazy merged 1 commit intoNixOS:llvm-19from
paparodeo:no-arch

Conversation

@paparodeo
Copy link
Contributor

-arch in the cc/ld wrapper causes problems when specifying different --target options, which would succeed except for the conflicting -arch specifier.

Reverts fc0456b and b26e0ba

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.

@paparodeo paparodeo marked this pull request as ready for review November 8, 2024 15:56
@nix-owners nix-owners bot requested a review from Ericson2314 November 8, 2024 15:57
@paparodeo
Copy link
Contributor Author

cc @emilazy

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Nov 8, 2024
@emilazy
Copy link
Member

emilazy commented Nov 8, 2024

@thefloweringash do you remember why you added these originally?

@github-actions github-actions bot removed the 6.topic: darwin Running or building packages on Darwin label Nov 8, 2024
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: darwin Running or building packages on Darwin labels Nov 8, 2024
@thefloweringash
Copy link
Member

@thefloweringash do you remember why you added these originally?

It was long enough ago that some details have been lost. There's a little background in #114817. At the time I was focused on bringing up aarch64-darwin by cross compiling stdenv from x86_64-darwin. For the CC wrapper I don't see why this couldn't have been covered by -target, but perhaps that was somehow inconvenient? For the bintools wrapper, it seems important that invocations of ld use the correct architecture. When invoked through clang the Darwin toolchain driver will pass -arch, but I would want to be sure that a direct invocation in a cross compiling environment used the correct arch value.

There's precedent in the nixpkgs clang wrapper for making some flags conditional on the absence of the --target argument, see #291901. If the flag is still required for cross completion, perhaps it could be similarly conditional?

@emilazy emilazy force-pushed the llvm-19 branch 2 times, most recently from a482a40 to 483a684 Compare November 10, 2024 07:36
`-arch` in the cc/ld wrapper causes problems when specifying different
`--target` options, which would succeed except for the conflicting
`-arch` specifier.

Reverts fc0456b and
b26e0ba
@emilazy
Copy link
Member

emilazy commented Nov 10, 2024

Thanks for the reply! We’ve tested that basic Darwin–Darwin cross still works on this branch, and looking at the ld64 source code it seems it infers the default architecture from the format of its input files rather than the platform it’s running on or anything like that. So I think that hopefully we should be good to land this, as any invocation that doesn’t result in a default being computed would presumably fail with the Xcode toolchain anyway.

@emilazy emilazy merged commit 1c43d06 into NixOS:llvm-19 Nov 10, 2024
@paparodeo paparodeo deleted the no-arch branch November 10, 2024 13:24
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Nov 16, 2024
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Nov 18, 2024
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: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 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.

3 participants