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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

harmou01
Copy link

@harmou01 harmou01 commented Jul 3, 2024

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.

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.
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

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

@rustbot 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
Copy link
Author

harmou01 commented Jul 3, 2024

r? @ehuss

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2024

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.

Add a new test case to check cargo handles building a target which
doesn't support the standard library properly.
@harmou01 harmou01 force-pushed the dev/harmou01/handle-target-build-std branch from c09ba9c to d79de0d Compare July 3, 2024 15:41
@ehuss
Copy link
Contributor

ehuss commented Jul 3, 2024

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
Copy link

adamgemmell commented Jul 3, 2024

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
Copy link
Contributor

ehuss commented Jul 3, 2024

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
Copy link
Collaborator

bors commented Jul 5, 2024

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

@adamgemmell
Copy link

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.

"failed to parse target spec from `rustc --print=target-spec-json`, got:\n{}",
output
)
})?;
Copy link
Member

@Noratrieb Noratrieb Jul 14, 2024

Choose a reason for hiding this comment

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

can you please make this non-fatal, such that you just keep a None if the format doesn't match what you expect? (and probably also when not using the nightly channel, the RUSTC_BOOTSTRAP=1 is quite problematic above and should be avoided), and then avoid emitting any error in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg-expr Area: Platform cfg expressions S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants