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

rustc: add a compiler wrapper #267876

Merged
3 commits merged into from
Nov 30, 2023
Merged

rustc: add a compiler wrapper #267876

3 commits merged into from
Nov 30, 2023

Conversation

alyssais
Copy link
Member

Description of changes

We keep running into situations where we can't get the right combination of rustc flags through build systems into rustc. RUSTFLAGS is the only variable supported across build systems, but if RUSTFLAGS is set, Cargo will ignore all other ways of specifying rustc flags, including the target-specific ones, which we need to make dynamic musl builds work. (This is why pkgsCross.musl64.crosvm is currently broken — it works if you unset separateDebugInfo, which causes RUSTFLAGS not to be set.)

So, we need to do the same thing we do for C and C++ compilers, and add a compiler wrapper so we can inject the flags we need, regardless of the build system.

Currently the wrapper only supports a single mechanism for injecting flags — the NIX_RUSTFLAGS environment variable. As time goes on, we'll probably want to add additional features, like target-specific environment variables.

This fixes #261727, and pkgsCross.musl64.crosvm.

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/)
  • 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.

@alyssais alyssais requested review from szlend, a user and yu-re-ka November 16, 2023 11:34
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: rust labels Nov 16, 2023
@ghost

This comment was marked as duplicate.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hooray! It's about time we did this.

I pushed a patch (hope that's okay) to this PR to fix CI. Running builds now; I'll approve this PR as soon as they finish. Two stylistic suggestions included below; neither one is critical but I think the second one will save us headaches later on.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Built (8257 derivations):

  • x86_64-linux workstation+server package sets
  • powerpc64le-linux workstation+server package sets
  • aarch64-linux (cross from x86_64-linux) workstation package set
  • mips64el-linux32 (cross from x86_64-linux) router package set
  • tests.cross.sanity

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 17, 2023
@yu-re-ka
Copy link
Contributor

Thank you for doing this!
Maybe we can fix aarch64 static with this too (later).
I wonder how this behaves with packages like rust-hypervisor-firmware, which was broken by the fast_cross wrapper because it uses a custom sysroot already.

@alyssais
Copy link
Member Author

I wonder how this behaves with packages like rust-hypervisor-firmware, which was broken by the fast_cross wrapper because it uses a custom sysroot already.

Should be unchanged — unless separateDebugInfo is enabled, this should result in rustc being run with exactly the same flags as before, fastCross or not.

@pbsds pbsds mentioned this pull request Nov 17, 2023
13 tasks
@andresilva

This comment was marked as off-topic.

We keep running into situations where we can't get the right
combination of rustc flags through build systems into rustc.
RUSTFLAGS is the only variable supported across build systems, but if
RUSTFLAGS is set, Cargo will ignore all other ways of specifying rustc
flags, including the target-specific ones, which we need to make
dynamic musl builds work.  (This is why pkgsCross.musl64.crosvm is
currently broken — it works if you unset separateDebugInfo, which
causes RUSTFLAGS not to be set.)

So, we need to do the same thing we do for C and C++ compilers, and
add a compiler wrapper so we can inject the flags we need, regardless
of the build system.

Currently the wrapper only supports a single mechanism for injecting
flags — the NIX_RUSTFLAGS environment variable.  As time goes on,
we'll probably want to add additional features, like target-specific
environment variables.
This avoids having two layers of wrapper for cross rustc.
Setting RUSTFLAGS causes Cargo to ignore other ways of configuring
flags, including the target-specific RUSTFLAGS options.  This broke
pkgsCross.musl64.crosvm, and was surprising to users.

Fixes: NixOS#261727
@ghost

This comment was marked as off-topic.

@ghost

This comment was marked as off-topic.

@alyssais
Copy link
Member Author

pkgsCross.musl64.rustfmt is similarly broken on current master for me — how's it relevant here?

@ghost
Copy link

ghost commented Nov 20, 2023

pkgsCross.musl64.rustfmt is similarly broken on current master for me

Sorry, I did not realize that. I built tests.cross.sanity and everything else passes.

Copy link
Contributor

@yu-re-ka yu-re-ka left a comment

Choose a reason for hiding this comment

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

I have some more ideas, but for now this improves the situation. Thanks!

@ghost ghost merged commit 52a13b8 into NixOS:staging Nov 30, 2023
@alyssais alyssais deleted the rustc-wrapper branch November 30, 2023 15:54
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 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.

4 participants