From 8fb272c8e34cc3b2f7426743c6db398daba7645d Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 24 Feb 2021 10:14:19 -0500 Subject: [PATCH] Remove `ENABLE_DOWNLOAD_RUSTC` constant 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 https://github.com/rust-lang/rust/pull/79540#discussion_r563350075 and https://github.com/rust-lang/rust/issues/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. --- src/bootstrap/builder.rs | 18 ------------------ src/bootstrap/check.rs | 4 ---- src/bootstrap/compile.rs | 13 +++++++++++++ src/bootstrap/tool.rs | 1 - 4 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index 61554a316d0..c5cc2dfe3ff 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -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; @@ -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<'_>), @@ -162,7 +153,6 @@ impl StepDescription { fn from() -> 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, @@ -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. diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 9b80f1cf9fc..6626fead774 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -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") @@ -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") @@ -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"]) @@ -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) diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 7d5e3d05b11..582dee2ea0e 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -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) { @@ -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. + builder.ensure(Sysroot { compiler }); + return; + } + builder.ensure(Std { compiler, target }); if builder.config.keep_stage.contains(&compiler.stage) { diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs index 5c874f69bd9..bf6bea539e5 100644 --- a/src/bootstrap/tool.rs +++ b/src/bootstrap/tool.rs @@ -477,7 +477,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<'_> {