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

Add a sysroot crate to represent the standard library crates #108865

Merged
merged 1 commit into from
May 4, 2023

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 7, 2023

This adds a dummy crate named sysroot to represent the standard library target instead of using the test crate. This allows the removal of proc_macro as a dependency of test allowing these 2 crates to build in parallel saving around 9 seconds locally.

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2023

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@jyn514
Copy link
Member

jyn514 commented Mar 7, 2023

I don't understand why this change is necessary. Why not just run cargo build -p proc_macro -p test? Where does the requirement for a single crate come from?

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 7, 2023

I don't think cargo supports specifying features per crate. There may be some other reason I don't know about also.

@jyn514
Copy link
Member

jyn514 commented Mar 7, 2023

I don't think that's true, but even if so, the features are additive between test and proc_macro, it shouldn't hurt anything. library/library just seems confusing, and will also break x build library (which today is an alias for "build all crates under library/").

@albertlarsan68
Copy link
Member

Maybe name it rustlibs?

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 9, 2023

@jyn514 It seems that cargo doc only supports a single crate. Not sure how to solve that in #108871.

sysroot could be an alternative crate name?

@jyn514
Copy link
Member

jyn514 commented Mar 9, 2023

I don't think that's what's going on in that PR. Here's the error message:

RUSTDOCFLAGS=--document-private-items --document-hidden-items python3 ../x.py doc --stage 0 library/test
Documenting {test} stage0 library (x86_64-unknown-linux-gnu) in HTML format
error: none of the selected packages contains these features: backtrace, compiler-builtins-c, panic-unwind

library/test is telling bootstrap to only document test and its dependencies. I think if you change the shell scripts to use library instead it will work fine. (We should probably also support documenting just test, that will likely need changes to std_features, but you should be able to test your changes before fixing that.)

@Zoxc Zoxc force-pushed the library-dummy-crate branch 2 times, most recently from e213891 to 5fada71 Compare March 15, 2023 20:08
@Zoxc Zoxc changed the title Add a library crate to represent the standard library crates Add a sysroot crate to represent the standard library crates Mar 15, 2023
@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 25, 2023

Given that is is more of a bootstrap change, r? @jyn514

@rustbot rustbot assigned jyn514 and unassigned m-ou-se Apr 25, 2023
@jyn514
Copy link
Member

jyn514 commented Apr 29, 2023

@bors r+

See #108871 (comment) for why we ended up not taking that approach.

@bors
Copy link
Contributor

bors commented Apr 29, 2023

📌 Commit fd4c81f has been approved by jyn514

It is now in the queue for this repository.

@bors 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 Apr 29, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108865 (Add a `sysroot` crate to represent the standard library crates)
 - rust-lang#110651 (libtest: include test output in junit xml reports)
 - rust-lang#110826 (Make PlaceMention a non-mutating use.)
 - rust-lang#110982 (Do not recurse into const generic args when resolving self lifetime elision.)
 - rust-lang#111009 (Add `ascii::Char` (ACP#179))
 - rust-lang#111100 (check array type of repeat exprs is wf)
 - rust-lang#111186 (Add `is_positive` method for signed non-zero integers.)
 - rust-lang#111201 (bootstrap: add .gitmodules to the sources)

Failed merges:

 - rust-lang#110954 (Reject borrows of projections in ConstProp.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0098cd4 into rust-lang:master May 4, 2023
@ehuss
Copy link
Contributor

ehuss commented May 5, 2023

This is going to cause a lot of breakage (build-std, cargo, miri, anyone who builds std manually, etc.). I kindly ask that in the future if there is a major change like this that you let people know ahead of time so that they can prepare, or perhaps provide suggestions to maintain compatibility to prevent this kind of situation.

I'm going to mark this with relnotes for a suggestion to add it to the compatibility section.

@ehuss ehuss added the relnotes Marks issues that should be documented in the release notes of the next release. label May 5, 2023
@Zoxc
Copy link
Contributor Author

Zoxc commented May 5, 2023

I did fail to release that this would break build-std, etc. Is build-std tested only on cargo CI? Do we not gate nightly on cargo CI passing with the rustc components that will be shipped?

bors added a commit to rust-lang/cargo that referenced this pull request May 5, 2023
fix: hack around `libsysroot` instead of `libtest`

### What does this PR try to resolve?

This is a fix in response to rust-lang/rust#108865.

Cargo `-Zbuild-std` now use `sysroot` crate to resolve cargo features instead
of the old hack around `libtest`. `sysroot` is just a dummy crate depending on
other standard library crates.
SortaUnknown added a commit to SortaUnknown/test-os that referenced this pull request May 5, 2023
@ehuss
Copy link
Contributor

ehuss commented May 5, 2023

Is build-std tested only on cargo CI? Do we not gate nightly on cargo CI passing with the rustc components that will be shipped?

Cargo's nightly features are not tested in the rust-lang/rust CI because of the cyclical nature of trying to change unstable interfaces.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2024
Make `profiler_builtins` an optional dependency of sysroot, not std

This avoids unnecessary rebuilds of std (and the compiler) when `build.profiler` is toggled off or on.

Fixes rust-lang#131812.

---

Background: The `profiler_builtins` crate has been an optional dependency of std (behind a cargo feature) ever since it was added back in rust-lang#42433. But as far as I can tell that has only ever been a convenient way to force the crate to be built, not a genuine dependency.

The side-effect of this false dependency is that toggling `build.profiler` causes a rebuild of std and the compiler, which shouldn't be necessary. This PR therefore makes `profiler_builtins` an optional dependency of the dummy sysroot crate (rust-lang#108865), rather than a dependency of std.

What makes this change so small is that all of the necessary infrastructure already exists. Previously, bootstrap would enable the `profiler` feature on the sysroot crate, which would forward that feature to std. Now, enabling that feature directly enables sysroot's `profiler_builtins` dependency instead.

---

I believe this is more of a bootstrap change than a libs change, so tentatively:
r? bootstrap
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
Make `profiler_builtins` an optional dependency of sysroot, not std

This avoids unnecessary rebuilds of std (and the compiler) when `build.profiler` is toggled off or on.

Fixes rust-lang#131812.

---

Background: The `profiler_builtins` crate has been an optional dependency of std (behind a cargo feature) ever since it was added back in rust-lang#42433. But as far as I can tell that has only ever been a convenient way to force the crate to be built, not a genuine dependency.

The side-effect of this false dependency is that toggling `build.profiler` causes a rebuild of std and the compiler, which shouldn't be necessary. This PR therefore makes `profiler_builtins` an optional dependency of the dummy sysroot crate (rust-lang#108865), rather than a dependency of std.

What makes this change so small is that all of the necessary infrastructure already exists. Previously, bootstrap would enable the `profiler` feature on the sysroot crate, which would forward that feature to std. Now, enabling that feature directly enables sysroot's `profiler_builtins` dependency instead.

---

I believe this is more of a bootstrap change than a libs change, so tentatively:
r? bootstrap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants