Skip to content

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Jul 7, 2025

The previous logic was wrong for no_std targets, it just didn't do anything. The logic was added there because by default, the Std step would otherwise have a list of all std crates to check, but these would fail for no_std targets. What has to happen instead is to select the default set of packages to check/doc/build, which currently happens in the std_cargo function, but the self.crates list was overriding that.

In general, using crates: Vec<String> in the Std steps is quite fishy, because it's difficult to distinguish between all crates (either they are all enumerated or crates is empty) and the default (e.g. x <kind> [library]) vs a subset (e.g. x <kind> core). I wanted to improve that using an enum that would distinguish these situations, avoid passing -p for all of the crates explicitly, and unify the selection of packages to compile/check/... in std_cargo, based on this enum.

However, I found out from some other bootstrap comments that when you pass -p explicitly for all crates, cargo behaves differently (apparently for check it will also check targets/examples etc. with -p, but not without it). Furthermore, the doc step has a special case where it does not document the sysroot package. So as usually, unifying this logic would get into some edge cases... So instead I opted for a seemingly simpler solution, where I try to prefilter only two allowed crates (core and alloc) for no_std targets in the std_crates_for_run_make function.

It's not perfect, but I think it's better than the status quo (words to live by when working on bootstrap...).

Fixes this Zulip topic.

r? @jieyouxu

@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) labels Jul 7, 2025
@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Jul 8, 2025

I think some tests need reblessing tho.

@Kobzol
Copy link
Member Author

Kobzol commented Jul 8, 2025

Yeah, I'll actually need to reimplement the tests, the way I did host normalization was not working outside x64.

@Kobzol
Copy link
Member Author

Kobzol commented Jul 8, 2025

Yeah, looks like that did the trick.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, a few nits

Comment on lines +149 to +150
/// Additional opaque string printed in the metadata
metadata: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Remark: I'm not so fussed about this, but maybe crates more specifically? Though of course I suppose for many Steps crates doesn't make sense anyway. We'll see if more specific use cases warrant changing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Metadata is a bit silly name here, but I just wanted this to be an opaque string that steps could just use as additional snapshot data.

@jieyouxu
Copy link
Member

jieyouxu commented Jul 9, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2025
@Kobzol
Copy link
Member Author

Kobzol commented Jul 9, 2025

Thanks!

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 9, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, you can r=me once PR CI is green.

@Kobzol
Copy link
Member Author

Kobzol commented Jul 9, 2025

@bors r=jieyouxu

@bors
Copy link
Collaborator

bors commented Jul 9, 2025

📌 Commit 6c38d38 has been approved by jieyouxu

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 Jul 9, 2025
bors added a commit that referenced this pull request Jul 10, 2025
Rollup of 9 pull requests

Successful merges:

 - #143446 (use `--dynamic-list` for exporting executable symbols)
 - #143590 (Fix weird rustdoc output when single and glob reexport conflict on a name)
 - #143599 (emit `.att_syntax` when global/naked asm use that option)
 - #143615 (Fix handling of no_std targets in `doc::Std` step)
 - #143632 (fix: correct parameter names in LLVMRustBuildMinNum and LLVMRustBuildMaxNum FFI declarations)
 - #143640 (Constify `Fn*` traits)
 - #143651 (Win: Use exceptions with empty data for SEH panic exception copies instead of a new panic)
 - #143660 (Disable docs for `compiler-builtins` and `sysroot`)
 - #143665 ([rustdoc-json] Add tests for `#[doc(hidden)]` handling of items.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 55a57fc into rust-lang:master Jul 10, 2025
11 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 10, 2025
rust-timer added a commit that referenced this pull request Jul 10, 2025
Rollup merge of #143615 - Kobzol:doc-std, r=jieyouxu

Fix handling of no_std targets in `doc::Std` step

The previous logic was wrong for no_std targets, it just didn't do anything. The logic was added there because by default, the `Std` step would otherwise have a list of all std crates to check, but these would fail for no_std targets. What has to happen instead is to select the default set of packages to check/doc/build, which currently happens in the `std_cargo` function, but the `self.crates` list was overriding that.

In general, using `crates: Vec<String>` in the `Std` steps is quite fishy, because it's difficult to distinguish between all crates (either they are all enumerated or `crates` is empty) and the default (e.g. `x <kind> [library]`) vs a subset (e.g. `x <kind> core`). I wanted to improve that using an enum that would distinguish these situations, avoid passing `-p` for all of the crates explicitly, and unify the selection of packages to compile/check/... in `std_cargo`, based on this enum.

However, I found out from some other bootstrap comments that when you pass `-p` explicitly for all crates, cargo behaves differently (apparently for check it will also check targets/examples etc. with `-p`, but not without it). Furthermore, the doc step has a special case where it does not document the `sysroot` package. So as usually, unifying this logic would get into some edge cases... So instead I opted for a seemingly simpler solution, where I try to prefilter only two allowed crates (core and alloc) for no_std targets in the `std_crates_for_run_make` function.

It's not perfect, but I think it's better than the status quo (words to live by when working on bootstrap...).

Fixes [this Zulip topic](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/docs.20for.20non-host.20targets.3F).

r? `@jieyouxu`
@Kobzol Kobzol deleted the doc-std branch July 11, 2025 05:11
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Jul 11, 2025
Rollup of 9 pull requests

Successful merges:

 - rust-lang/rust#143446 (use `--dynamic-list` for exporting executable symbols)
 - rust-lang/rust#143590 (Fix weird rustdoc output when single and glob reexport conflict on a name)
 - rust-lang/rust#143599 (emit `.att_syntax` when global/naked asm use that option)
 - rust-lang/rust#143615 (Fix handling of no_std targets in `doc::Std` step)
 - rust-lang/rust#143632 (fix: correct parameter names in LLVMRustBuildMinNum and LLVMRustBuildMaxNum FFI declarations)
 - rust-lang/rust#143640 (Constify `Fn*` traits)
 - rust-lang/rust#143651 (Win: Use exceptions with empty data for SEH panic exception copies instead of a new panic)
 - rust-lang/rust#143660 (Disable docs for `compiler-builtins` and `sysroot`)
 - rust-lang/rust#143665 ([rustdoc-json] Add tests for `#[doc(hidden)]` handling of items.)

r? `@ghost`
`@rustbot` modify labels: rollup
github-actions bot pushed a commit to rust-lang/compiler-builtins that referenced this pull request Jul 12, 2025
Rollup of 9 pull requests

Successful merges:

 - rust-lang/rust#143446 (use `--dynamic-list` for exporting executable symbols)
 - rust-lang/rust#143590 (Fix weird rustdoc output when single and glob reexport conflict on a name)
 - rust-lang/rust#143599 (emit `.att_syntax` when global/naked asm use that option)
 - rust-lang/rust#143615 (Fix handling of no_std targets in `doc::Std` step)
 - rust-lang/rust#143632 (fix: correct parameter names in LLVMRustBuildMinNum and LLVMRustBuildMaxNum FFI declarations)
 - rust-lang/rust#143640 (Constify `Fn*` traits)
 - rust-lang/rust#143651 (Win: Use exceptions with empty data for SEH panic exception copies instead of a new panic)
 - rust-lang/rust#143660 (Disable docs for `compiler-builtins` and `sysroot`)
 - rust-lang/rust#143665 ([rustdoc-json] Add tests for `#[doc(hidden)]` handling of items.)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants