cargo
cargo copied to clipboard
Remove requirement for --target when invoking Cargo with -Zbuild-std
This PR addresses this issue re: build-std stabilization. We believe the requirement for --target to be specified when invoking cargo with -Zbuild-std, from our testing, is no longer needed. Now, with this change, by default Cargo will use the Host CompileKind, rather than a manually specified CompileTarget. We propose removing this restriction in order to test this more widely. Our own testing is detailed below.
This change has been tested in the following manner:
- Building crates depending on proc_macro, std, and build scripts (which themselves depend on proc_macro)
- Various RUSTFLAGS, such as
-Zsanitizer=cfi,-Cembed-bitcode=yes,-Cforce-frame-pointers,-Cforce-unwind-tables=yes,-Csoft-float=yes,-Zbranch-protection=pac-ret.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.
Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:
@rustbot author: the review is finished, PR author should check the comments and take action accordingly@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue
Yea, I'd like to see some tests added. There is a dedicated build-std suite that would be appropriate for this. Testing proc-macros is the most important part, and including shared dependencies I think will also be important.
Also, the documentation will need updating, too.
I'm not sure what @weihanglo means by only working on certain platforms. This should work on all host platforms that we test with.
One key difference of how cargo works today versus when this was written is that cargo now defaults to "cross compile mode", and then has a separate pass which unifies things when building in "host only" mode. Because the host dependencies will not be wired up to the std dependencies, that means they essentially get forked and built twice. Overall that seems like it should work, though I'm still uncertain about RUSTFLAGS compatibility.
This may complicate https://github.com/rust-lang/wg-cargo-std-aware/issues/31, since cargo would need to be careful about how it disables the sysroot. But overall I think it should be workable.
Thanks @ehuss. Could you clarify what you mean relating to the disable sysroot issue?
https://github.com/rust-lang/wg-cargo-std-aware/issues/31 is recommending to add a flag to rustc to disable sysroot lookups. Cargo would pass that flag when using build-std to ensure that rustc does not ever attempt to load crates from the sysroot (to avoid various weird errors). With the change in this PR, proc-macros and build scripts will start using the standard library from the sysroot.
We will need some way to tell cargo to not pass that flag for host crates. That may be complicated due to the way rebuild_unit_graph_shared works, since it might lose the information about whether or not something is for "host" or not. I have not looked at if that is a problem or not, or how complicated that will be to support.
That seems like something we can deal with in the future, though.
With the change in this PR, proc-macros and build scripts will start using the standard library from the sysroot.
I think this is already the case. It looks like cargo assumes, for the purpose of build dependencies, that artifact deps can execute on the host when a --target isn't provided but assumes they can't even if --target=[host]. It therefore makes sense that when in --target mode build scripts would need to use the sysroot.
Even though they technically can, I don't think build scripts using --extern std in host mode is even any better since right now building and running build scripts doesn't depend on building std. I think this would just slow down builds.
@ehuss & @weihanglo , have you had a chance to look at the updated changes? Thanks!
Thanks, sorry for the delay!
I'm not fully confident this won't unearth some issues since there are a lot of subtle interactions (like the RUSTFLAGS thing I mentioned above). However, I can't think of anything specific that will be a problem. Really appreciate you helping with this!
@bors r+
:pushpin: Commit 5a6667222c0f3fe4aa8d2b6666ec3767a38a2bdb has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit 5a6667222c0f3fe4aa8d2b6666ec3767a38a2bdb with merge 9abcaefd516bf1e78aafaadd6a49b9d9f5143a7b...
:sunny: Test successful - checks-actions Approved by: ehuss Pushing 9abcaefd516bf1e78aafaadd6a49b9d9f5143a7b to master...