Don't condition RUSTDOC_LIBDIR on --no-doc
#147167
Merged
+3
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use |
This comment has been minimized.
This comment has been minimized.
In d94e7ff, `rustdoc_path` was changed to ignore `want_rustdoc` (which is just whether `--no-doc` was passed). But RUSTDOC_LIBDIR wasn't kept in sync. Rather than trying to keep `rustdoc_path` in sync with `RUSTDOC_LIBDIR`, just pass LIBDIR to the rustc shim unconditionally. This fix allows calling `ensure(doc::Step)` from a non-doc top-level Step, even if `--no-doc` was present in the command line.
jyn514
force-pushed
the
rustdoc-tests
branch
from
11f8023 to
73f6b08
Compare
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Sep 30, 2025
Rollup of 5 pull requests Successful merges: - #140916 (Fix unuseful span in type error in some format_args!() invocations) - #146011 (Point at fn bound that introduced lifetime obligation) - #146649 (cmse: fix 'region variables should not be hashed') - #147109 (Rename various "concrete opaque type" things to say "hidden type") - #147167 (Don't condition RUSTDOC_LIBDIR on `--no-doc`) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
that referenced
this pull request
Sep 30, 2025
Rollup merge of #147167 - jyn514:rustdoc-tests, r=Kobzol Don't condition RUSTDOC_LIBDIR on `--no-doc` In d94e7ff, `rustdoc_path` was changed to ignore `want_rustdoc` (which is just whether `--no-doc` was passed). But RUSTDOC_LIBDIR wasn't kept in sync. Rather than trying to keep `rustdoc_path` in sync with `RUSTDOC_LIBDIR`, just pass LIBDIR to the rustc shim unconditionally. This fix allows calling `ensure(doc::Step)` from a non-doc top-level Step, even if `--no-doc` was present in the command line.
bors-ferrocene bot
added a commit
to ferrocene/ferrocene
that referenced
this pull request
Oct 9, 2025
1787: Custom core coverage aggregator (codename: `blanket`) r=Hoverbear a=jyn514 Summary: We know which symbols we care about covering (those in https://public-docs.ferrocene.dev/main/certification/core/subset.html), and we want to ensure we cover exactly those rather than "whichever functions make it into the binary past LTO". 1. Write a rustc_driver which collects a list of those fully-qualified symbols. Emit them as JSON to `build/host/stage1-std/${certified-target}/release/symbol-report.json`. 2. Rather than using llvm-cov to turn our .profdata files into .info, use our new `blanket` tool to generate an in-memory structured data aggregation. 3. Rather than using grcov to display the aggregation, use the HTML reports in our new `blanket` tool to generate `build/${certified-target}/doc/certified-coverage-report.html. This also fixes the main original bug, which was that we were filtering out `libstd.so` when aggregating coverage reports. That is not correct; without that .so the coverage tool doesn't know how to map the binary info back to source info, and reports all the sources as uncovered. Before, using llvm-cov, it would also be reported as "ignored". Now, using blanket, it's correctly reported as uncovered. Doing things in this way allows us to customize the reporting as well as to add integration tests that we actually have all the coverage we expect. This PR does not currently modify `ferrocene/doc/core-certification/src/spreadsheets/subset.tsv`. Long term, I would like to delete that file, along with `ferrocene/tools/coverage-of-subset`, and use the generated HTML report in `blanket` for everything. --- Additionally, this extends bootstrap to support running std's tests and doctests with `-C instrument-coverage`. Unfortuately, `blanket` tends to crash while parsing the .profdata for these because of an upstream bug in `llvm-profparse`. I am going to investigate why that happens in the next few weeks. In the meantime, if you see "Parsing Failed" while running `blanket`, try again but with fewer tests. Doctests work in the following way: - Pass `--persist-doctest-binaries` to rustdoc for std tests whenever bootstrap is passed `--coverage=library` - Load those binaries when generating coverage, so that blanket can parse their DWARF info Std tests are mostly the same as before, except that passing `-L deps` for profiler_builtins has moved to `instrument_coverage` so that we're passing it consistently instead of only for compile::Std. --- Some things to note while reviewing: - This includes all commits from https://github.com/ferrocene/blanket/. Those commits were pulled in as a subtree, and the private repo will be deleted shortly. - This includes two bugfixes for pre-existing issues in bootstrap, which have upstream PRs: rust-lang/rust#147168 and rust-lang/rust#147167 - Upstream has no way to get a fully-qualified crate name, and when [I asked about upstreaming it](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/print.20a.20fully.20qualified.20path.20name.3F/with/541561323) they didn't seem interested. So 7bd8340 forks the compiler to add a way to do that. I don't *believe* this affects our spec, but Ana may know more. - In #1769 Ana and Christian added various changes for doctests and std. ed186f8 adds to those changes so that we can cross-compile std. That commit is not strictly part of symbol-report, but symbol-report needs it in order to not break. - As part of a previous approach, I tried use rustdoc-json in `symbol-report`. I ripped that out because using a rustc_driver was more reliable. But the changes to make it possible to run `x doc certified-api-docs --json` are still present in 265c76c, because that seems useful. I can rip them out if desired. We caught some pre-existing bugs while working on this: - `-C instrument-coverage` breaks most of the TCP and UDP tests, because they can't bind to the socket. It's unclear why this happens. We'll investigate this later. - We pass `-L deps/` to load profiler_builtins. This can cause caching bugs or incorrect behavior if there are multiple versions of profiler_builtins in the deps directory. We plan to switch to `--extern profiler_builtins=/exact/path.rlib` in the future to avoid this. --- To test this, you can run `x test --coverage=library --no-doc library/core --stage 1`, which should print the path to an HTML file that looks like this: <img width="1335" height="879" alt="image" src="https://github.com/user-attachments/assets/18e61df7-dcc6-467a-9832-3f318de784d8" /> Co-authored-by: Jynn Nelson <[email protected]> Co-authored-by: Ana Hobden <[email protected]> Co-authored-by: Christian Poveda <[email protected]>
bors-ferrocene bot
added a commit
to ferrocene/ferrocene
that referenced
this pull request
Oct 10, 2025
1787: Custom core coverage aggregator (codename: `blanket`) r=Hoverbear a=jyn514 Summary: We know which symbols we care about covering (those in https://public-docs.ferrocene.dev/main/certification/core/subset.html), and we want to ensure we cover exactly those rather than "whichever functions make it into the binary past LTO". 1. Write a rustc_driver which collects a list of those fully-qualified symbols. Emit them as JSON to `build/host/stage1-std/${certified-target}/release/symbol-report.json`. 2. Rather than using llvm-cov to turn our .profdata files into .info, use our new `blanket` tool to generate an in-memory structured data aggregation. 3. Rather than using grcov to display the aggregation, use the HTML reports in our new `blanket` tool to generate `build/${certified-target}/doc/certified-coverage-report.html. This also fixes the main original bug, which was that we were filtering out `libstd.so` when aggregating coverage reports. That is not correct; without that .so the coverage tool doesn't know how to map the binary info back to source info, and reports all the sources as uncovered. Before, using llvm-cov, it would also be reported as "ignored". Now, using blanket, it's correctly reported as uncovered. Doing things in this way allows us to customize the reporting as well as to add integration tests that we actually have all the coverage we expect. This PR does not currently modify `ferrocene/doc/core-certification/src/spreadsheets/subset.tsv`. Long term, I would like to delete that file, along with `ferrocene/tools/coverage-of-subset`, and use the generated HTML report in `blanket` for everything. --- Additionally, this extends bootstrap to support running std's tests and doctests with `-C instrument-coverage`. Unfortuately, `blanket` tends to crash while parsing the .profdata for these because of an upstream bug in `llvm-profparse`. I am going to investigate why that happens in the next few weeks. In the meantime, if you see "Parsing Failed" while running `blanket`, try again but with fewer tests. Doctests work in the following way: - Pass `--persist-doctest-binaries` to rustdoc for std tests whenever bootstrap is passed `--coverage=library` - Load those binaries when generating coverage, so that blanket can parse their DWARF info Std tests are mostly the same as before, except that passing `-L deps` for profiler_builtins has moved to `instrument_coverage` so that we're passing it consistently instead of only for compile::Std. --- Some things to note while reviewing: - This includes all commits from https://github.com/ferrocene/blanket/. Those commits were pulled in as a subtree, and the private repo will be deleted shortly. - This includes two bugfixes for pre-existing issues in bootstrap, which have upstream PRs: rust-lang/rust#147168 and rust-lang/rust#147167 - Upstream has no way to get a fully-qualified crate name, and when [I asked about upstreaming it](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/print.20a.20fully.20qualified.20path.20name.3F/with/541561323) they didn't seem interested. So 7bd8340 forks the compiler to add a way to do that. I don't *believe* this affects our spec, but Ana may know more. - In #1769 Ana and Christian added various changes for doctests and std. ed186f8 adds to those changes so that we can cross-compile std. That commit is not strictly part of symbol-report, but symbol-report needs it in order to not break. - As part of a previous approach, I tried use rustdoc-json in `symbol-report`. I ripped that out because using a rustc_driver was more reliable. But the changes to make it possible to run `x doc certified-api-docs --json` are still present in 265c76c, because that seems useful. I can rip them out if desired. We caught some pre-existing bugs while working on this: - `-C instrument-coverage` breaks most of the TCP and UDP tests, because they can't bind to the socket. It's unclear why this happens. We'll investigate this later. - We pass `-L deps/` to load profiler_builtins. This can cause caching bugs or incorrect behavior if there are multiple versions of profiler_builtins in the deps directory. We plan to switch to `--extern profiler_builtins=/exact/path.rlib` in the future to avoid this. --- To test this, you can run `x test --coverage=library --no-doc library/core --stage 1`, which should print the path to an HTML file that looks like this: <img width="1335" height="879" alt="image" src="https://github.com/user-attachments/assets/18e61df7-dcc6-467a-9832-3f318de784d8" /> Co-authored-by: Jynn Nelson <[email protected]> Co-authored-by: Ana Hobden <[email protected]> Co-authored-by: Christian Poveda <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In d94e7ff,
rustdoc_pathwas changed to ignorewant_rustdoc(which is just whether--no-docwas passed). But RUSTDOC_LIBDIR wasn't kept in sync. Rather than trying to keeprustdoc_pathin sync withRUSTDOC_LIBDIR, just pass LIBDIR to the rustc shim unconditionally.This fix allows calling
ensure(doc::Step)from a non-doc top-level Step, even if--no-docwas present in the command line.