Remove requirement for --target when invoking Cargo with -Zbuild-std #14317

harmou01 · 2024-07-29T14:59:06Z

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.

rustbot · 2024-07-29T14:59:11Z

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

weihanglo · 2024-07-29T15:17:25Z

src/cargo/core/compiler/build_config.rs

@@ -97,11 +97,6 @@ impl BuildConfig {
            },
        };

-        if gctx.cli_unstable().build_std.is_some() && requested_kinds[0].is_host() {


Should we add some build-std tests for relying on host target?

This might not be easy to set up though. Maybe we can only do this on certain platforms like x86_64 linux.

@ehuss I meant this but now it seems to be a separate issue so you're right.

cargo/tests/build-std/main.rs

Lines 235 to 240 in a7c4206

// Fixing rust-lang/rust#117839.

// on macOS it never gets remapped.

// Might be a separate issue, so only run on Linux.

#[cargo_test(build_std_real)]

#[cfg(target_os = "linux")]

fn remap_path_scope() {

ehuss · 2024-07-29T16:53:44Z

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 rust-lang/wg-cargo-std-aware#31, since cargo would need to be careful about how it disables the sysroot. But overall I think it should be workable.

harmou01 · 2024-08-01T09:24:22Z

Thanks @ehuss. Could you clarify what you mean relating to the disable sysroot issue?

ehuss · 2024-08-10T01:22:07Z

rust-lang/wg-cargo-std-aware#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.

adamgemmell · 2024-08-29T16:01:53Z

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.

There is no longer a need for the --target to be specified in every case when using -Zbuild-std. Cargo will default to the Host CompileKind when no --target is specified.

Add a new test case for building a crate with -Zbuild-std, without the requirement for the --target flag, and that uses a proc_macro.

Add a test case which ensures that -Zbuild-std without --target correctly handles building a crate that has a shared dependency between it's own build script, and std.

harmou01 · 2024-10-21T13:06:45Z

@ehuss & @weihanglo , have you had a chance to look at the updated changes? Thanks!

ehuss · 2024-10-30T13:52:53Z

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+

bors · 2024-10-30T13:52:56Z

📌 Commit 5a66672 has been approved by ehuss

It is now in the queue for this repository.

bors · 2024-10-30T13:54:05Z

⌛ Testing commit 5a66672 with merge 9abcaef...

bors · 2024-10-30T14:33:48Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 9abcaef to master...

Update cargo 18 commits in e75214ea4936d2f2c909a71a1237042cc0e14b07..0310497822a7a673a330a5dd068b7aaa579a265e 2024-10-25 16:34:32 +0000 to 2024-11-01 19:27:56 +0000 - Add more metadata to `rustc_fingerprint` (rust-lang/cargo#14761) - test(rustfix): switch to a simpler case for dedup-suggestions (rust-lang/cargo#14765) - chore(deps): update rust crate security-framework to v3 (rust-lang/cargo#14766) - chore(deps): update rust crate gix to 0.67.0 (rust-lang/cargo#14762) - fix(util): Respect all `..`s in `normalize_path` (rust-lang/cargo#14750) - test(doc): Resolve flaky test (rust-lang/cargo#14760) - refactor(test): Remove dead 'expect_stdout_contains_n' check (rust-lang/cargo#14759) - add unstable -Zroot-dir flag to configure the path from which rustc should be invoked (rust-lang/cargo#14752) - docs(resolver): Further v3 prep (rust-lang/cargo#14753) - fix: track version in fingerprint dep-info files (rust-lang/cargo#14751) - test: Remove unused msrv-policy (rust-lang/cargo#14748) - download targeted transitive deps of with artifact deps' target platform (rust-lang/cargo#14723) - Remove requirement for --target when invoking Cargo with -Zbuild-std (rust-lang/cargo#14317) - docs(fingerprint): document the encoding of Cargo's depinfo (rust-lang/cargo#14745) - Allow build scripts to report error messages through `cargo::error` (rust-lang/cargo#14743) - fix(publish): Downgrade version-exists error to warning on dry-run (rust-lang/cargo#14742) - fix: clean up for deprecated and removed commands (rust-lang/cargo#14739) - Deprecate `cargo verify-project` (rust-lang/cargo#14736)

rustbot assigned ehuss Jul 29, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2024

weihanglo reviewed Jul 29, 2024

View reviewed changes

Remove requirement for --target when invoking Cargo with -Zbuild-std

879efa6

There is no longer a need for the --target to be specified in every case when using -Zbuild-std. Cargo will default to the Host CompileKind when no --target is specified.

harmou01 force-pushed the dev/harmou01/remove-target-flag-req branch from c3edeff to 388bceb Compare September 24, 2024 17:01

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Sep 24, 2024

harmou01 added 3 commits September 25, 2024 10:52

test: build-std proc_macro test case without --target requirement

3597f4c

Add a new test case for building a crate with -Zbuild-std, without the requirement for the --target flag, and that uses a proc_macro.

test: mock-std test case for shared dependencies without --target req

b6e1942

Add a test case which ensures that -Zbuild-std without --target correctly handles building a crate that has a shared dependency between it's own build script, and std.

docs: update documentation for removal of build-std --target requirement

5a66672

harmou01 force-pushed the dev/harmou01/remove-target-flag-req branch from 388bceb to 5a66672 Compare September 25, 2024 12:23

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2024

bors merged commit 9abcaef into rust-lang:master Oct 30, 2024
24 checks passed

weihanglo mentioned this pull request Nov 1, 2024

Update cargo rust-lang/rust#132475

Merged

rustbot added this to the 1.84.0 milestone Nov 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove requirement for --target when invoking Cargo with -Zbuild-std #14317

Remove requirement for --target when invoking Cargo with -Zbuild-std #14317

harmou01 commented Jul 29, 2024

rustbot commented Jul 29, 2024

weihanglo Jul 29, 2024

weihanglo Jul 29, 2024

weihanglo Jul 29, 2024

ehuss commented Jul 29, 2024

harmou01 commented Aug 1, 2024

ehuss commented Aug 10, 2024

adamgemmell commented Aug 29, 2024

harmou01 commented Oct 21, 2024 •

edited

Loading

ehuss commented Oct 30, 2024

bors commented Oct 30, 2024

bors commented Oct 30, 2024

bors commented Oct 30, 2024

	// Fixing rust-lang/rust#117839.
	// on macOS it never gets remapped.
	// Might be a separate issue, so only run on Linux.
	#[cargo_test(build_std_real)]
	#[cfg(target_os = "linux")]
	fn remap_path_scope() {

Remove requirement for --target when invoking Cargo with -Zbuild-std #14317

Remove requirement for --target when invoking Cargo with -Zbuild-std #14317

Conversation

harmou01 commented Jul 29, 2024

rustbot commented Jul 29, 2024

weihanglo Jul 29, 2024

Choose a reason for hiding this comment

weihanglo Jul 29, 2024

Choose a reason for hiding this comment

weihanglo Jul 29, 2024

Choose a reason for hiding this comment

ehuss commented Jul 29, 2024

harmou01 commented Aug 1, 2024

ehuss commented Aug 10, 2024

adamgemmell commented Aug 29, 2024

harmou01 commented Oct 21, 2024 • edited Loading

ehuss commented Oct 30, 2024

bors commented Oct 30, 2024

bors commented Oct 30, 2024

bors commented Oct 30, 2024

harmou01 commented Oct 21, 2024 •

edited

Loading