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.

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

src/cargo/core/compiler/standard_lib.rs

tests/testsuite/standard_lib.rs

tests/testsuite/freshness.rs

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.

weihanglo · 2024-11-12T22:18:00Z

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

+#[derive(Deserialize)]
+pub struct Metadata {
+    pub std: Option<bool>,
+}
+
+#[derive(Deserialize)]
+pub struct TargetSpec {


No need to be pub.

Also, while not really a blocker, personally I may move these closer to where they are used, Like inside the gctx.cli_unstable().build_std.is_some() {} block.

See also the suggestion in #14183 (comment)

weihanglo · 2024-11-12T22:24:43Z

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

+            // try to fetch this information if rustc allows nightly features. Additionally,
+            // to avoid making two rustc queries when not required, only try to fetch the
+            // target-spec when the '-Zbuild-std' option is passed.
+            if gctx.nightly_features_allowed && gctx.cli_unstable().build_std.is_some() {


You cannot set build_std = Some(…) when not on nightly, so it is redundant.

Suggested change

if gctx.nightly_features_allowed && gctx.cli_unstable().build_std.is_some() {

if gctx.cli_unstable().build_std.is_some() {

weihanglo · 2024-11-12T23:29:46Z

tests/testsuite/standard_lib.rs

@@ -183,6 +184,11 @@ impl BuildStd for Execs {
        self.arg("--target").arg(rustc_host());
        self
    }
+
+    fn target(&mut self, target: &str) -> &mut Self {


While this method looks quite convenient, I consider it as a refactor. There are a lot of places just calling cargo.run("build --target).arg("<triple>") now. I may put off this change a bit if it is only used by one place.

weihanglo · 2024-11-12T23:45:48Z

src/cargo/core/compiler/standard_lib.rs

+        for (target, target_info) in target_data.target_info() {
+            if target_info.supports_std == Some(false) {
+                anyhow::bail!(
+                    "building std is not supported on this target: {:?}",
+                    target.short_name()
+                );
+            }
+        }


Instead of exposing fn target_info(), should we have a method say RustcTargetData::supports_std instead?

Suggested change

for (target, target_info) in target_data.target_info() {

if target_info.supports_std == Some(false) {

anyhow::bail!(

"building std is not supported on this target: {:?}",

target.short_name()

);

}

}

target_data.supports_std()?;

A perhaps could show all unsupported targets instead of the first one found.
(though I don't know if people really build multi-target std in one invocation)

I think this is a good idea. I'll have a think about how to handle listing all unsupported targets.

weihanglo · 2024-11-13T00:03:52Z

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

+
+                let target_spec_cached_output =
+                    rustc.cached_output(&target_spec_process, extra_fingerprint);
+
+                if target_spec_cached_output.is_ok() {
+                    let (stdout, _stderr) = target_spec_cached_output.unwrap();
+                    match serde_json::from_str(&stdout) {
+                        Result::Ok(val) => {
+                            let target_spec: TargetSpec = val;
+                            supports_std = target_spec.metadata.std;
+                        }
+                        Result::Err(_) => (),
+                    }
+                }


rustc.cached_output caches the rustc output to target/.rustc_info.json file. I guess it is better not to cache it. A direct call to ProcessBuilder::output is probably good enough. Would be something like:

Suggested change

let target_spec_cached_output =

rustc.cached_output(&target_spec_process, extra_fingerprint);

if target_spec_cached_output.is_ok() {

let (stdout, _stderr) = target_spec_cached_output.unwrap();

match serde_json::from_str(&stdout) {

Result::Ok(val) => {

let target_spec: TargetSpec = val;

supports_std = target_spec.metadata.std;

}

Result::Err(_) => (),

}

}

#[derive(Deserialize)]

pub struct Metadata {

pub std: Option<bool>,

}

#[derive(Deserialize)]

pub struct TargetSpec {

pub metadata: Metadata,

}

if let Ok(output) = target_spec_process.output() {

if let Ok(spec) = serde_json::from_slice::<TargetSpec>(&output.stdout) {

supports_std = spec.metadata.std;

}

}

weihanglo · 2024-11-13T00:12:25Z

tests/testsuite/cfg.rs

As Eric has noted, target-spec-json is not going to be stabilized in a near feature, I would suggest not adding a new mode to this test. It is already complicated. The test_std_on_unsupported_target test should have covered the parsing end-to-end pretty much.

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

weihanglo reviewed Nov 13, 2024

View reviewed changes

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

harmou01 commented Oct 16, 2024

adamgemmell commented Oct 16, 2024

weihanglo Nov 12, 2024

weihanglo Nov 13, 2024

weihanglo Nov 12, 2024

weihanglo Nov 12, 2024

weihanglo Nov 12, 2024

harmou01 Nov 13, 2024

weihanglo Nov 13, 2024

weihanglo Nov 13, 2024

	if gctx.nightly_features_allowed && gctx.cli_unstable().build_std.is_some() {
	if gctx.cli_unstable().build_std.is_some() {

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

harmou01 commented Oct 16, 2024

adamgemmell commented Oct 16, 2024

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

adamgemmell commented Jul 3, 2024 •

edited

Loading