Skip to content

clang: don't set machine flags for overridden target#317273

Merged
alyssais merged 2 commits intoNixOS:stagingfrom
alyssais:machine-flags
Jun 15, 2024
Merged

clang: don't set machine flags for overridden target#317273
alyssais merged 2 commits intoNixOS:stagingfrom
alyssais:machine-flags

Conversation

@alyssais
Copy link
Member

@alyssais alyssais commented Jun 4, 2024

Description of changes

We already did this for -march in 12b0e8a ("clang: don't set -march for overridden target"), but it should have been done for all machine flags, for the same reason.

Example bug this fixes:

nix-shell -E '
  with import ./. {
    crossSystem = {
      system = "powerpc64le-linux";
      gcc.cpu = "power10";
    };
  };
  clangStdenv.mkDerivation { name = "test"; }
' --run '$CC -target wasm32-unknown-unknown -c /dev/null'

Which previously failed with:

  clang: error: unsupported option '-mcpu=' for target 'wasm32-unknown-unknown'

@vcunat could you please test whether this fixes rustc on aarch64-darwin, as reported in #309580 (comment)? If it does, we can make a temporary fix on staging-next.

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.

@alyssais alyssais requested a review from Ericson2314 as a code owner June 4, 2024 19:13
@alyssais alyssais added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label Jun 4, 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: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jun 4, 2024
@Samasaur1
Copy link
Member

Samasaur1 commented Jun 5, 2024

The Swift change (paralleling #301103) looks right to me, but I'm building it and a Swift package just to make sure everything works out

@vcunat
Copy link
Member

vcunat commented Jun 5, 2024

rustc built for me on this PR's commit on aarch64-darwin (the whitespace change doesn't cause rebuild).

Copy link
Member

@Samasaur1 Samasaur1 left a comment

Choose a reason for hiding this comment

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

My Swift package builds and runs. LGTM

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 7, 2024
@vcunat
Copy link
Member

vcunat commented Jun 7, 2024

If it does, we can make a temporary fix on staging-next.

@alyssais are you working on that?

@alyssais
Copy link
Member Author

alyssais commented Jun 8, 2024

Should have it done later today.

@alyssais
Copy link
Member Author

alyssais commented Jun 9, 2024

#318447

@github-actions github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Jun 9, 2024
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Passes my eye-ball test, didn't test it.

alyssais added 2 commits June 10, 2024 10:13
We already did this for -march in 12b0e8a ("clang: don't set
-march for overridden target"), but it should have been done for all
machine flags, for the same reason.

Example bug this fixes:

	nix-shell -E '
	  with import ./. {
	    crossSystem = {
	      system = "powerpc64le-linux";
	      gcc.cpu = "power10";
	    };
	  };
	  clangStdenv.mkDerivation { name = "test"; }
	' --run '$CC -target wasm32-unknown-unknown -c /dev/null'

Which previously failed with:

      clang: error: unsupported option '-mcpu=' for target 'wasm32-unknown-unknown'
This reverts commits 6d0ba08,
723100d, and
bf13eca.

The underlying issue has now been fixed, so we can re-enable
wasm32-unknown-unknown on all platforms.
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 10, 2024
@alyssais alyssais merged commit b00f262 into NixOS:staging Jun 15, 2024
@alyssais alyssais deleted the machine-flags branch June 15, 2024 06:02
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/is-there-a-way-to-install-a-macos-pkg-from-an-url/49660/10

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

Labels

6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 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: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments