Check build target supports std when building with -Zbuild-std=std #14183

harmou01 · 2024-07-03T10:36:54Z

What does this PR try to resolve?

Ensures that Cargo first verifies whether a given target supports building the standard library when the -Zbuild-std=std option is passed to Cargo (see issue here). This information can be obtained by querying rustc --print=target-spec-json. The target spec "metadata" contains an Option<bool> determining whether the target supports building std.

In the case this value is false, cargo will stop the build. This avoids the numerous "use of unstable library" errors, giving a cleaner, and simpler, "building std is not supported on this target".

How should we test and review this PR?

It can be manually tested by running cargo build --target <target> -Zbuild-std=std. If a target who's target-spec marks std as false is passed, cargo will exit. This works with multiple --target's, and if any of them don't support std, cargo will exit.

Additional Information

This change relies on two things:

The target-spec metadata in rustc needs to be filled out. Currently, most fields are None, which is treated as OK with this change. Meaning this can be merged before the rustc changes.
The new test case added with this change will fail without at least aarch64-unknown-none having it's target-spec metadata completed.
Some targets have std support marked as "?". If this state is properly represented in the target-spec in the future, it needs to be determined whether this is allowed, so the build can continue, or whether it fails.

rustbot · 2024-07-03T10:36:59Z

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

harmou01 · 2024-07-03T10:37:11Z

r? @ehuss

rustbot · 2024-07-03T10:37:13Z

Could not assign reviewer from: ehuss.
User(s) ehuss are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

ehuss · 2024-07-03T15:46:14Z

Thanks for the PR! Unfortunately I'm not sure this is something we can move forward with in the short term. The --print target-spec-json option would need to be stabilized in order for this to be supported in build-std. I'm not sure how feasible that is. My impression is that it seems like that will be difficult, since some of the fields are somewhat inherently unstable.

There is some more information at rust-lang/rust#38338 and rust-lang/wg-cargo-std-aware#6.

I would suggest getting in contact with the compiler team to suss out the viability of that. There might be other alternatives, such as providing other rustc options for querying/extracting information like this.

adamgemmell · 2024-07-03T16:06:57Z

I didn't think that target-spec-json would need to be stable because Cargo and Rustc are versioned together - this creates a contract between the two, but one that can be changed, similar to the contract between Rustc and std (or stdarch as a closer example). Would a test in Rust that tests this integration alleviate your concerns? We only need to parse part of the spec.

Otherwise I doubt the spec can be stabilised, so I think we'd need some other mechanism of getting target info from Rustc. I think this might be useful for things like getting the default panic or unwind strategy too.

ehuss · 2024-07-03T17:08:57Z

I didn't think that target-spec-json would need to be stable because Cargo and Rustc are versioned together - this creates a contract between the two, but one that can be changed, similar to the contract between Rustc and std (or stdarch as a closer example).

They aren't exactly tightly coupled with each other. Cargo is expected to work on the previous two versions of rustc (although build-std makes that complicated). Also, since cargo is developed in a separate repository, it makes it difficult to synchronize changes, since a change in rustc could immediately break development in cargo.

I think it might be possible to support this on an advisory basis, but not something we can strictly rely upon. It would likely need a different approach than what is taken in this PR, though. A few things to consider:

I'm uncomfortable with setting RUSTC_BOOTSTRAP in TargetInfo::new, since that could leak exposure of unstable options on stable (and generally uncomfortable setting it anywhere). I would suggest collecting the JSON information in a separate call to cached_output. That would also avoid needing to guess where the JSON boundaries are.
Ignore all errors from rustc, to handle the case where target-spec-json changes.
Ignore all errors from deserialization, to also handle the case if target-spec-json changes.

However, I'm reluctant to do that since I think it introduces risk of breaking cargo's CI and development (since cargo's testsuite could fail). I'm not sure how a test in rust-lang/rust would help, since if the compiler needs to make a change to the target-spec-json output or CLI option, there would be a problem where you can't update both rustc and cargo at the same time. That has been a fundamental problem that has prevented us from adding build-std tests to rust-lang/rust.

I would suggest opening a dialog with the compiler team to see if they have any input on how cargo and rustc can coordinate to obtain this information in a way that would be reliable.

bors · 2024-07-05T01:00:35Z

☔ The latest upstream changes (presumably #14185) made this pull request unmergeable. Please resolve the merge conflicts.

adamgemmell · 2024-07-05T17:22:53Z

I appreciate the detailed answer, thanks. Agreed on the RUSTC_BOOTSTRAP=1 hack. On checking the target-spec-json tracking issue it does seem like there's some support for stabilising the print option which surprised me - I think I had it mixed up in my head with the idea of providing the JSON as an input target.

Considering the case where it was stabilised with all fields, current and future, optional. In the case of this PR, it would mean that Cargo handles the "not found" case for the Metadata::std field, probably by printing out a warning, but not error out or break Cargo's CI. Then we'd basically have to wait for a bug report from someone who got the warning and parse the new format while continuing to support the old one.

This seems not ideal, but might it be good enough? The alternative would be a stricter form of stabilisation that makes all current fields mandatory, any future fields optional and all enums non-exhaustive and strings have an undefined format.

src/cargo/core/compiler/build_context/target_info.rs

weihanglo

Regardless of my review, Eric's concern in #14183 (comment) should be deeply considered.

weihanglo · 2024-10-15T00:05:47Z

src/cargo/core/compiler/build_context/target_info.rs

+            // The '--print=target-spec-json' is an unstable option of rustc, therefore if
+            // rustc is not a 'Prerelease' build, it's safe to assume that we cannot fetch
+            // the target-spec-json. So don't bother trying.
+            if rustc.version.is_prerelease() {


We normally use GlobalContext::nightly_features_allowed to detect it. Any reason preferring is_prerelease over that?

No reason, I wasn't aware of the preferred method for detecting this. I'll make this change.

weihanglo · 2024-10-15T00:11:09Z

src/cargo/core/compiler/build_context/target_info.rs

+            if rustc.version.is_prerelease() {
+                let mut target_spec_process = rustc.workspace_process();
+                apply_env_config(gctx, &mut target_spec_process)?;
+                target_spec_process


I would suggest collecting the JSON information in a separate call to cached_output. That would also avoid needing to guess where the JSON boundaries are.

Couldn't we again use --print=crate-name, which print ___ as a delimiter again? I know that is quite hacky, though a second call to rustc isn't a good thing for cargo startup time, especially for the stabilization of cargo -Zscript.

I'm not sure what the purpose of adding the delimiter back in would be? Do you mean reverting back to doing this all in a single rustc call?

We put this in a second rustc call to avoid RUSTC_BOOTSTRAP being used on the first one. If startup time is a concern we could ensure it only runs if build-std is requested.

Ah, RUSTC_BOOTSTRAP isn't used any more, perhaps this could go into one rustc call.

If startup time is a concern we could ensure it only runs if build-std is requested.

That would be ideal! build_std can be accessed via gctx.cli_unstable(). build_std.is_some()

https://doc.rust-lang.org/nightly/nightly-rustc/cargo/core/features/struct.CliUnstable.html#structfield.build_std

weihanglo · 2024-10-15T00:35:30Z

src/cargo/core/compiler/standard_lib.rs

+                return Err(anyhow::Error::msg(format!(
+                    "building std is not supported on this target: {:?}",
+                    target.short_name()
+                )));


anyhow::bail! is a better way to handle this.

weihanglo · 2024-10-15T00:37:49Z

tests/testsuite/standard_lib.rs

+            "\
+[ERROR] building std is not supported on this target: [..]
+",


Unless intentionally, would recommend wrapping with str![[...]] so that the snapshot could get auto-updated.

Noted, will make this change.

weihanglo · 2024-10-15T00:41:57Z

tests/testsuite/freshness.rs

@@ -2493,6 +2493,9 @@ LLVM version: 9.0
            "src/main.rs",
            r#"
            fn main() {
+                if std::env::args_os().any(|s| s.into_string().unwrap().contains("unstable-options")) {


Why do we need this guard? Seems the nightly channel detection is off?

When running the test with a stable toolchain, the test masquerades it as a nightly compiler. The target-spec-json print therefore fails. Probably we should make cargo tolerate the failure instead though

This test case was failing for me locally, and this fixed the issue. However i'm not seeing this fail now, so it's possible it can be reverted.

harmou01 · 2024-10-16T09:30:41Z

@weihanglo Regarding Eric's concerns about the use of the --print=target-spec-json and the possibility of it changing, @adamgemmell has an RFC open to add a --print=std-support flag, which gives us a simpler and more reliable method for determining std support. We wouldn't have to grab the entire target-spec-json anymore, and therefore wouldn't risk things changing between versions, because rustc would always give a true/false answer. I think this should address Eric's concerns.

The second rustc query wouldn't be needed either if this RFC is implemented, we could revert back to collecting everything with a single query.

rust-lang/rfcs#3693

adamgemmell · 2024-10-16T10:18:29Z

rust-lang/rfcs#3693

To elaborate on this we tried to make this patch improve things without waiting for the RFC's proposal to become stable by removing RUSTC_BOOTSTRAP and making Cargo tolerate all failure cases. If you'd feel more comfortable waiting for that solution then we're fine with that of course.

Running cargo with "-Zbuild-std=std" should check whether the build target supports building the standard library. This information can be obtained from rustc with the target-spec-json "--print" option. When 'std' is false for the build target, cargo should not attempt to build the standard library. This avoids the "use of unstable library" errors, as this check is performed before Cargo starts trying to build restricted_std code. Cargo will now emit a warning if the requested target does not support building the standard library, or if there was an issue when collecting the necessary information via rustc

Add a new test case to check cargo handles building a target which doesn't support the standard library properly.

An additional rustc query is made in Cargo to collect the target-spec-json from rustc to determine if the build target supports compiling the standard library. As this functionality relies on a nightly rustc, Cargo does not output any errors when parsing, and continues as normal. This commit adds a new test case to bad_cfg_discovery to ensure that Cargo handles this properly. Unlike the other tests, there is no expected output and an exit code 0.

rustbot assigned ehuss Jul 3, 2024

rustbot added A-cfg-expr Area: Platform cfg expressions S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2024

harmou01 mentioned this pull request Jul 3, 2024

Fill out target-spec metadata for all targets rust-lang/rust#127265

Merged

harmou01 force-pushed the dev/harmou01/handle-target-build-std branch from c09ba9c to d79de0d Compare July 3, 2024 15:41

Noratrieb reviewed Jul 14, 2024

View reviewed changes

src/cargo/core/compiler/build_context/target_info.rs Outdated Show resolved Hide resolved

harmou01 force-pushed the dev/harmou01/handle-target-build-std branch from d79de0d to db383e7 Compare October 14, 2024 13:09

weihanglo reviewed Oct 15, 2024

View reviewed changes

weihanglo added the Z-build-std Nightly: build-std label Oct 15, 2024

harmou01 force-pushed the dev/harmou01/handle-target-build-std branch from db383e7 to e34950e Compare October 15, 2024 09:25

harmou01 added 3 commits October 30, 2024 11:45

Add new test case 'test_std_on_unsupported_target'

820251e

Add a new test case to check cargo handles building a target which doesn't support the standard library properly.

harmou01 force-pushed the dev/harmou01/handle-target-build-std branch from e34950e to 37a326e Compare October 30, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check build target supports std when building with -Zbuild-std=std #14183

Check build target supports std when building with -Zbuild-std=std #14183

harmou01 commented Jul 3, 2024

rustbot commented Jul 3, 2024

harmou01 commented Jul 3, 2024

rustbot commented Jul 3, 2024

ehuss commented Jul 3, 2024

adamgemmell commented Jul 3, 2024 •

edited

Loading

ehuss commented Jul 3, 2024

bors commented Jul 5, 2024

adamgemmell commented Jul 5, 2024

weihanglo left a comment

weihanglo Oct 15, 2024

harmou01 Oct 15, 2024

weihanglo Oct 15, 2024

harmou01 Oct 16, 2024

adamgemmell Oct 16, 2024

adamgemmell Oct 16, 2024

weihanglo Oct 16, 2024

weihanglo Oct 15, 2024

harmou01 Oct 15, 2024

weihanglo Oct 15, 2024

harmou01 Oct 16, 2024

weihanglo Oct 15, 2024

adamgemmell Oct 16, 2024

harmou01 Oct 16, 2024

harmou01 commented Oct 16, 2024

adamgemmell commented Oct 16, 2024

Check build target supports std when building with -Zbuild-std=std #14183

Are you sure you want to change the base?

Check build target supports std when building with -Zbuild-std=std #14183

Conversation

harmou01 commented Jul 3, 2024

rustbot commented Jul 3, 2024

harmou01 commented Jul 3, 2024

rustbot commented Jul 3, 2024

ehuss commented Jul 3, 2024

adamgemmell commented Jul 3, 2024 • edited Loading

ehuss commented Jul 3, 2024

bors commented Jul 5, 2024

adamgemmell commented Jul 5, 2024

weihanglo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmou01 commented Oct 16, 2024

adamgemmell commented Oct 16, 2024

adamgemmell commented Jul 3, 2024 •

edited

Loading