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

Remove ENABLE_DOWNLOAD_RUSTC constant #82480

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 24, 2021

ENABLE_DOWNLOAD_RUSTC was introduced as part of the MVP for download-rustc as a way not to rebuild artifacts that have already been downloaded. Unfortunately, it doesn't work very well:

  • Steps are ignored by default, which makes it easy to leave out a step
    that should be built. For example, the MVP forgot to enable any tests,
    so it was only possible to build locally.
  • It didn't work correctly even when it was enabled: calling
    builder.ensure() would completely ignore the constant and rebuild the
    step anyway. This has no obvious fix since ensure() has to return a
    Step::Output.

Instead, this handles download-rustc in impl Step for Rustc and
impl Step for Std, which to my knowledge are the only build steps that
don't first go through impl Step for Sysroot (Rustc is used for
the rustc-dev component).

See #79540 (comment) and #81930 for further context.

Here are some example runs with these changes and download-rustc
enabled:

$ x.py build src/tools/clippy
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 1m 09s
Building stage1 tool cargo-clippy (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.11s
$ x.py test src/tools/clippy
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.09s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.28s
    Finished release [optimized] target(s) in 15.26s
     Running build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/clippy_driver-8b407b140e0aa91c
test result: ok. 592 passed; 0 failed; 3 ignored; 0 measured; 0 filtered out
$ x.py build src/tools/rustdoc
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 41.28s
Build completed successfully in 0:00:41
$ x.py test src/test/rustdoc-ui
Building stage0 tool compiletest (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.12s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.10s
test result: ok. 105 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.15s
$ x.py build compiler/rustc
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Build completed successfully in 0:00:00

Note a few things:

  • Clippy depends on stage1 rustc-dev artifacts, but rustc didn't have to
    be recompiled. Instead, the artifacts were copied automatically.
  • All steps are always enabled. There is no danger of forgetting a step,
    since only the entrypoints have to handle download-rustc.
  • Building the compiler (compiler/rustc) automatically does no work.

Helps with #81930.

r? @Mark-Simulacrum

This was introduced as part of the MVP for `download-rustc`.
Unfortunately, it doesn't work very well:

- Steps are ignored by default, which makes it easy to leave out a step
that should be built. For example, the MVP forgot to enable any tests,
so it was *only* possible to build locally.
- It didn't work correctly even when it was enabled: calling
  `builder.ensure()` would completely ignore the constant and rebuild the
  step anyway. This has no obvious fix since `ensure()` has to return a
  `Step::Output`.

Instead, this handles `download-rustc` in `impl Step for Rustc` and
`impl Step for Std`, which to my knowledge are the only build steps that
don't first go through `impl Step for Sysroot` (`Rustc` is used for
the `rustc-dev` component).

See rust-lang#79540 (comment)
and rust-lang#81930 for further context.

Here are some example runs with these changes and `download-rustc`
enabled:

```
$ x.py build src/tools/clippy
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 1m 09s
Building stage1 tool cargo-clippy (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.11s
$ x.py test src/tools/clippy
Updating only changed submodules
Submodules updated in 0.01 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Building stage1 tool clippy-driver (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.09s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.28s
    Finished release [optimized] target(s) in 15.26s
     Running build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/clippy_driver-8b407b140e0aa91c
test result: ok. 592 passed; 0 failed; 3 ignored; 0 measured; 0 filtered out
$ x.py build src/tools/rustdoc
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 41.28s
Build completed successfully in 0:00:41
$ x.py test src/test/rustdoc-ui
Building stage0 tool compiletest (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.12s
Building rustdoc for stage1 (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.10s
test result: ok. 105 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 8.15s
$ x.py build compiler/rustc
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Build completed successfully in 0:00:00
```

Note a few things:

- Clippy depends on stage1 rustc-dev artifacts, but rustc didn't have to
  be recompiled. Instead, the artifacts were copied automatically.
- All steps are always enabled. There is no danger of forgetting a step,
  since only the entrypoints have to handle `download-rustc`.
- Building the compiler (`compiler/rustc`) automatically does no work.
@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Feb 24, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 24, 2021

In addition to the things I mentioned in the description, I hope this also addresses the concern you had in the MVP:

I am unconvinced that the new Step constant is actually useful in practice and will work well, but it shouldn't affect people not enabling this option and we can iterate on that over time as needed. In particular, I am worried it excludes e.g. clippy or cargo work today; it seems like both of those should in theory support download-rustc as the interaction mode, but with this PR I believe wouldn't.

@@ -494,6 +500,13 @@ impl Step for Rustc {
let compiler = self.compiler;
let target = self.target;

if builder.config.download_rustc {
// Copy the existing artifacts instead of rebuilding them.
// NOTE: this path is only taken for tools linking to rustc-dev.
Copy link
Member

Choose a reason for hiding this comment

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

Hm not sure I really follow this comment, there's nothing gating this is there? I think it'll always be the case that we skip this compile with download-rustc set?

Copy link
Member Author

Choose a reason for hiding this comment

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

download-rustc works for everything besides tools without this block because it's handled in impl Step for Sysroot:

// If we're downloading a compiler from CI, we can use the same compiler for all stages other than 0.
if builder.config.download_rustc {
assert_eq!(
builder.config.build, compiler.host,
"Cross-compiling is not yet supported with `download-rustc`",
);
// Copy the compiler into the correct sysroot.
let stage0_dir = builder.config.out.join(&*builder.config.build.triple).join("stage0");
builder.cp_r(&stage0_dir, &sysroot);
return INTERNER.intern_path(sysroot);
}

Only building rustc-dev skips the sysroot and builds rustc directly.

@Mark-Simulacrum
Copy link
Member

Seems good to me, left a question; with that resolved r=me (happy to take another look or feel free to approve if you think there's a good answer/clarification update)

@Mark-Simulacrum Mark-Simulacrum 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 Mar 1, 2021
@jyn514
Copy link
Member Author

jyn514 commented Mar 1, 2021

Seems good to me, left a question; with that resolved r=me (happy to take another look or feel free to approve if you think there's a good answer/clarification update)

Thanks! I left an explanation which hopefully makes sense. I'm going to approve this in the meantime so people can start using download-rustc for tests, but I'm happy to improve the comment or make changes in a follow-up.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Mar 1, 2021

📌 Commit 8fb272c has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81210 (BTreeMap: correct node size test case for choices of B)
 - rust-lang#82360 (config.toml parsing error improvements)
 - rust-lang#82428 (Update mdbook)
 - rust-lang#82480 (Remove `ENABLE_DOWNLOAD_RUSTC` constant)
 - rust-lang#82578 (Add some diagnostic items for Clippy)
 - rust-lang#82620 (Apply lint restrictions from renamed lints)
 - rust-lang#82635 (Fix typos in rustc_infer::infer::nll_relate)
 - rust-lang#82645 (Clarify that SyncOnceCell::set blocks.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4f14f17 into rust-lang:master Mar 1, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 1, 2021
@jyn514 jyn514 deleted the no-enable-constant branch March 1, 2021 23:53
@jyn514 jyn514 mentioned this pull request Mar 3, 2021
13 tasks
@jyn514 jyn514 added A-download-rustc Area: Related to the `rust.download-rustc` build option and removed A-download-rustc Area: Related to the `rust.download-rustc` build option labels Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-download-rustc Area: Related to the `rust.download-rustc` build option 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-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants