Skip to content

Commit

Permalink
Rollup merge of #82480 - jyn514:no-enable-constant, r=Mark-Simulacrum
Browse files Browse the repository at this point in the history
Remove `ENABLE_DOWNLOAD_RUSTC` constant

`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`
  • Loading branch information
Joshua Nelson committed Mar 1, 2021
2 parents 57d7c7e + 8fb272c commit 4f14f17
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 23 deletions.
18 changes: 0 additions & 18 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,6 @@ pub trait Step: 'static + Clone + Debug + PartialEq + Eq + Hash {
/// `true` here can still be overwritten by `should_run` calling `default_condition`.
const DEFAULT: bool = false;

/// Whether this step should be run even when `download-rustc` is set.
///
/// Most steps are not important when the compiler is downloaded, since they will be included in
/// the pre-compiled sysroot. Steps can set this to `true` to be built anyway.
///
/// When in doubt, set this to `false`.
const ENABLE_DOWNLOAD_RUSTC: bool = false;

/// If true, then this rule should be skipped if --target was specified, but --host was not
const ONLY_HOSTS: bool = false;

Expand Down Expand Up @@ -107,7 +99,6 @@ impl RunConfig<'_> {

struct StepDescription {
default: bool,
enable_download_rustc: bool,
only_hosts: bool,
should_run: fn(ShouldRun<'_>) -> ShouldRun<'_>,
make_run: fn(RunConfig<'_>),
Expand Down Expand Up @@ -162,7 +153,6 @@ impl StepDescription {
fn from<S: Step>() -> StepDescription {
StepDescription {
default: S::DEFAULT,
enable_download_rustc: S::ENABLE_DOWNLOAD_RUSTC,
only_hosts: S::ONLY_HOSTS,
should_run: S::should_run,
make_run: S::make_run,
Expand All @@ -179,14 +169,6 @@ impl StepDescription {
"{:?} not skipped for {:?} -- not in {:?}",
pathset, self.name, builder.config.exclude
);
} else if builder.config.download_rustc && !self.enable_download_rustc {
if !builder.config.dry_run {
eprintln!(
"Not running {} because its artifacts have been downloaded from CI (`download-rustc` is set)",
self.name
);
}
return;
}

// Determine the targets participating in this rule.
Expand Down
4 changes: 0 additions & 4 deletions src/bootstrap/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ fn cargo_subcommand(kind: Kind) -> &'static str {
impl Step for Std {
type Output = ();
const DEFAULT: bool = true;
const ENABLE_DOWNLOAD_RUSTC: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.all_krates("test")
Expand Down Expand Up @@ -156,7 +155,6 @@ impl Step for Rustc {
type Output = ();
const ONLY_HOSTS: bool = true;
const DEFAULT: bool = true;
const ENABLE_DOWNLOAD_RUSTC: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.all_krates("rustc-main")
Expand Down Expand Up @@ -235,7 +233,6 @@ impl Step for CodegenBackend {
type Output = ();
const ONLY_HOSTS: bool = true;
const DEFAULT: bool = true;
const ENABLE_DOWNLOAD_RUSTC: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.paths(&["compiler/rustc_codegen_cranelift", "rustc_codegen_cranelift"])
Expand Down Expand Up @@ -293,7 +290,6 @@ macro_rules! tool_check_step {
type Output = ();
const ONLY_HOSTS: bool = true;
const DEFAULT: bool = true;
const ENABLE_DOWNLOAD_RUSTC: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path($path)
Expand Down
13 changes: 13 additions & 0 deletions src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ impl Step for Std {
let target = self.target;
let compiler = self.compiler;

// These artifacts were already copied (in `impl Step for Sysroot`).
// Don't recompile them.
if builder.config.download_rustc {
return;
}

if builder.config.keep_stage.contains(&compiler.stage)
|| builder.config.keep_stage_std.contains(&compiler.stage)
{
Expand Down Expand Up @@ -507,6 +513,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.
builder.ensure(Sysroot { compiler });
return;
}

builder.ensure(Std { compiler, target });

if builder.config.keep_stage.contains(&compiler.stage) {
Expand Down
1 change: 0 additions & 1 deletion src/bootstrap/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,6 @@ pub struct Rustdoc {
impl Step for Rustdoc {
type Output = PathBuf;
const DEFAULT: bool = true;
const ENABLE_DOWNLOAD_RUSTC: bool = true;
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
Expand Down

0 comments on commit 4f14f17

Please sign in to comment.