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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 #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
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.
  • Loading branch information
jyn514 committed Feb 24, 2021
commit 8fb272c8e34cc3b2f7426743c6db398daba7645d
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 @@ -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.

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 @@ -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<'_> {
Expand Down