bootstrap: download-rustc support for ferrocene s3 bucket and coverage#2048
Merged
bors-ferrocene[bot] merged 7 commits intomainfrom Apr 7, 2026
Merged
bootstrap: download-rustc support for ferrocene s3 bucket and coverage#2048bors-ferrocene[bot] merged 7 commits intomainfrom
bors-ferrocene[bot] merged 7 commits intomainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
tshepang
reviewed
Dec 23, 2025
| if let Some(config_path) = &self.config { | ||
| // Ferrocene addition: our CI builders don't currently upload builder-config. | ||
| // For now, just assume it matches the local settings. | ||
| if false && let Some(config_path) = &self.config { |
Member
There was a problem hiding this comment.
maybe left-over from testing
Suggested change
| if false && let Some(config_path) = &self.config { | |
| if let Some(config_path) = &self.config { |
Contributor
Author
There was a problem hiding this comment.
no, that is intentional. see the comment above.
Contributor
Author
There was a problem hiding this comment.
I didn't remove the code altogether because I don't want to cause merge conflicts with upstream.
Contributor
Author
oh, i got very close, but it didn't work because of the caching issue in https://github.com/ferrocene/ferrocene/blob/main/src/bootstrap/src/ferrocene/code_coverage.rs#L32-L47 (the one with an open ticket) |
jyn514
force-pushed
the
jyn/download-rustc-ferrocene
branch
from
09515fc to
3f016db
Compare
jyn514
changed the title
Hoverbear
reviewed
Jan 2, 2026
Member
Hoverbear
left a comment
Hoverbear
left a comment
There was a problem hiding this comment.
I believe you're adding some safeguards to stage related flags. Let's do that. :)
Going to hold this until backports are cozy for 1.92, just in case.
Contributor
Author
yup, and i'm also going to delete |
Member
|
bors try |
bors-ferrocene Bot
added a commit
that referenced
this pull request
Jan 20, 2026
2048: bootstrap: download-rustc support for ferrocene s3 bucket and coverage r=Hoverbear a=jyn514 For details about the changes, see the individual commit messages. For more background on download-rustc and why it's useful, see https://rustc-dev-guide.rust-lang.org/building/suggested.html?highlight=download-rustc#faster-builds-with-ci-rustc. To test this, set `rust.download-rustc = true` and `rust.debug-assertions = false` in config.toml, then run `x t --coverage=library library/core library/alloc --no-doc`. Co-authored-by: Jynn Nelson <[email protected]>
Member
|
bors cancel |
Contributor
|
Canceled. |
Contributor
tryBuild failed: |
Member
|
bors try |
Contributor
tryBuild succeeded: |
jyn514
added
waiting-on-author
jyn514
force-pushed
the
jyn/download-rustc-ferrocene
branch
from
3f016db to
e5c29c9
Compare
Contributor
Author
|
i think this might be using the wrong core when i run coverage tests ... |
jyn514
force-pushed
the
jyn/download-rustc-ferrocene
branch
from
e5c29c9 to
bc47be4
Compare
Contributor
Author
|
I think this is a strict improvement over the current situation, even though it's not really usable for coverage yet. I think I'd like to merge it as-is, maybe with a note in bootstrap.example.toml and in the docs that it's experimental and may break. |
Contributor
Author
|
This makes three changes: 1. Pass `config` into `download_file` more consistently. Unlike upstream, we need a config in order to download artifacts. 2. Adjust to our incompatible dist artifact URL format. This is not great; ideally we would just use the same naming scheme as upstream. But that requires coordinated changes across CI, bootstrap, and criticalup. For now, just use our new names. 3. Don't try to compare the builder's config settings to the local config. Our artifacts don't currently package their settings, so this always fails. This should be fairly simple to fix, but I don't think it's necessary to block on.
There were three main issues here. 1. The pre-existing caching issue, where profiler_builtins was passed multiple times to `x test --coverage` consistently happened *every* time when download-rustc was enabled (it was loaded once from ci-rustc and once from stage1-std). 2. When std is downloaded from CI, the staging gets *very* complicated. In particular, `x test --stage 2 core` will build the *sysroot* with stage 1 but the *tests* with stage 2. The logic for this lives only in `compile::Std`, and the various other parts of the codebase that were looking at the stage didn't account for it. 3. When instrumenting core, `force_rebuild` needs to always be set or bootstrap will make incorrect caching decisions. Fix 1. by building `profiler_builtins` explicitly, separate from the rest of the standard library, and passing it explicitly to rustc with `--extern profiler_builtins=...`. I did this with a new `ProfilerBuiltins` step. Fix 2. by moving all relevant logic to `instrument_coverage` and out of `std_cargo`. Fix 3. by enabling `force_rebuild` whenever `should_instrument_coverage` is set. I removed the `ferrocene_inject_profiler_builtins` feature; it's not necessary now that we pass it explicitly, and it will break the build if cargo tries to rebuild it with a different metadata hash or in a different stage. Finally, I removed the `-L` flag that was originally causing all the caching trouble. To test this PR, you can set `rust.download-rustc = true`, `rust.debug-assertions = false`, and then run `x test --coverage=library library/core library/alloc --no-doc` like normal.
Contributor
Author
|
Added some short docs and warnings. I haven't switched this on by default for the library profile, partly because upstream looks unlikely to and I want to match them, and partly because this is still experimental. |
jyn514
force-pushed
the
jyn/download-rustc-ferrocene
branch
from
abd53bf to
5b6ebc7
Compare
jyn514
added
waiting-on-review
and removed
waiting-on-author
This comment was marked as resolved.
This comment was marked as resolved.
Only stage 2 is currently supported.
Member
|
Hmmm following the recommended test procedure from the PR description (with |
Contributor
Author
|
@Hoverbear you can't just remove --tests altogether, you need to replace it with |
Member
|
That works better, is this the expected result? |
Contributor
Author
|
yeah that looks broken in the way I expect, it's getting the spans for the standard library wrong. this isn't currently usable for coverage but it should be useful for most other things. |
Member
|
Excellent, yes, it was notably faster. |
Hoverbear
reviewed
Apr 3, 2026
| use crate::{BootstrapCommand, Compiler, Mode}; | ||
|
|
||
| pub(crate) fn instrument_coverage(builder: &Builder<'_>, cargo: &mut Cargo, compiler: Compiler) { | ||
| pub(crate) fn instrument_coverage( |
Member
There was a problem hiding this comment.
Do you think we could make the --coverage=library arg explicitly crash with download-rustc, since it's known bad? I'd like to avoid a situation where we get confused and chase a wild goose around.
Contributor
Author
There was a problem hiding this comment.
I feel like Nami would love chasing a goose actually.
seems like a good idea, done.
Contributor
Author
There was a problem hiding this comment.
(It doesn't crash but it does disable download-rustc, which means you'll get the right results. You the user can choose to kill the build if you don't want to spend that much time.)
Hoverbear
approved these changes
Apr 3, 2026
Member
Hoverbear
left a comment
Hoverbear
left a comment
There was a problem hiding this comment.
Looks overall good, mildly concerned we might not realize we have this set when measuring coverage though. Recommend a guard rail, so holding merge until you decide if you agree.
Example output:
```
$ x b compiler
Building bootstrap
Finished `dev` profile [unoptimized] target(s) in 0.02s
NOTE: detected 3 modifications that could affect a build of rustc
- src/bootstrap/src/core/config/config.rs
- src/bootstrap/src/core/download.rs
- src/build_helper/src/git.rs
skipping rustc download due to `download-rustc = 'if-unchanged'`
Building stage1 compiler artifacts (stage0 -> stage1, aarch64-apple-darwin)
Finished `release` profile [optimized] target(s) in 0.72s
Creating a sysroot for stage1 compiler (use `rustup toolchain link 'name' build/host/stage1`)
Build completed successfully in 0:00:05
```
Contributor
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
For details about the changes, see the individual commit messages.
For more background on download-rustc and why it's useful, see https://rustc-dev-guide.rust-lang.org/building/suggested.html?highlight=download-rustc#faster-builds-with-ci-rustc.
To test this, set
rust.download-rustc = trueandrust.debug-assertions = falsein config.toml, then runx t --coverage=library library/core library/alloc --tests.This doesn't report the same coverage outcomes as
download-rustc = falsecurrently, there's some bug in how spans are reported. I think this is a strict improvement over the current situation, even though it's not really usable for coverage yet. I'd like to merge it as-is. There's a note in bootstrap.example.toml and in the docs that it's experimental and may break.