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.

Sign up for GitHub

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

Clean up dependency tracking in Rustbuild [2/2] #52036

Merged
merged 8 commits into from
Sep 17, 2018
+90 −109
Merged

Clean up dependency tracking in Rustbuild [2/2] #52036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c0af0b0
clear_if_dirty in Builder::cargo with passed mode
collin5 Jul 3, 2018
c22c709
use stage 1 compiler only for stamps
collin5 Jul 6, 2018
86e34cd
clear_if_dirty with self.rustc(compiler) for Mode::Rustc
collin5 Jul 10, 2018
de3ec8d
nit: rearrange and make match exhaustive
collin5 Jul 10, 2018
e792d1d
remove struct CleanTools
collin5 Jul 13, 2018
b585893
Do not print checking messages in dry run mode
Mark-Simulacrum Aug 2, 2018
9681e13
Add check build dependencies
Mark-Simulacrum Aug 2, 2018
5ae40be
refactor Builder::cargo, clean deps for cmd!=test
collin5 Sep 17, 2018
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
74 changes: 74 additions & 0 deletions src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,80 @@ impl<'a> Builder<'a> {
) -> Command {
let mut cargo = Command::new(&self.initial_cargo);
let out_dir = self.stage_out(compiler, mode);

// command specific path, we call clear_if_dirty with this
let mut my_out = match cmd {
"build" => self.cargo_out(compiler, mode, target),

// This is the intended out directory for crate documentation.
"doc" => self.crate_doc_out(target),

_ => self.stage_out(compiler, mode),
};

// This is for the original compiler, but if we're forced to use stage 1, then
// std/test/rustc stamps won't exist in stage 2, so we need to get those from stage 1, since
// we copy the libs forward.
let cmp = if self.force_use_stage1(compiler, target) {
self.compiler(1, compiler.host)
} else {
compiler
};

let libstd_stamp = match cmd {
"check" => check::libstd_stamp(self, cmp, target),
_ => compile::libstd_stamp(self, cmp, target),
};

let libtest_stamp = match cmd {
"check" => check::libtest_stamp(self, cmp, target),
_ => compile::libstd_stamp(self, cmp, target),
};

let librustc_stamp = match cmd {
"check" => check::librustc_stamp(self, cmp, target),
_ => compile::librustc_stamp(self, cmp, target),
};

if cmd == "doc" {
if mode == Mode::Rustc || mode == Mode::ToolRustc || mode == Mode::Codegen {
// This is the intended out directory for compiler documentation.
my_out = self.compiler_doc_out(target);
}
let rustdoc = self.rustdoc(compiler.host);
self.clear_if_dirty(&my_out, &rustdoc);
} else if cmd != "test" {
match mode {
Mode::Std => {
self.clear_if_dirty(&my_out, &self.rustc(compiler));
},
Mode::Test => {
self.clear_if_dirty(&my_out, &libstd_stamp);
},
Mode::Rustc => {
self.clear_if_dirty(&my_out, &self.rustc(compiler));
self.clear_if_dirty(&my_out, &libstd_stamp);
self.clear_if_dirty(&my_out, &libtest_stamp);
},
Mode::Codegen => {
self.clear_if_dirty(&my_out, &librustc_stamp);
},
Mode::ToolBootstrap => { },
Mode::ToolStd => {
self.clear_if_dirty(&my_out, &libstd_stamp);
},
Mode::ToolTest => {
self.clear_if_dirty(&my_out, &libstd_stamp);
self.clear_if_dirty(&my_out, &libtest_stamp);
},
Mode::ToolRustc => {
self.clear_if_dirty(&my_out, &libstd_stamp);
self.clear_if_dirty(&my_out, &libtest_stamp);
self.clear_if_dirty(&my_out, &librustc_stamp);
},
}
}

cargo
.env("CARGO_TARGET_DIR", out_dir)
.arg(cmd);
Expand Down
33 changes: 9 additions & 24 deletions src/bootstrap/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

use compile::{run_cargo, std_cargo, test_cargo, rustc_cargo, rustc_cargo_env, add_to_sysroot};
use builder::{RunConfig, Builder, ShouldRun, Step};
use tool::{self, prepare_tool_cargo, SourceType};
use tool::{prepare_tool_cargo, SourceType};
use {Compiler, Mode};
use cache::{INTERNER, Interned};
use std::path::PathBuf;
Expand Down Expand Up @@ -40,14 +40,11 @@ impl Step for Std {
let target = self.target;
let compiler = builder.compiler(0, builder.config.build);

let out_dir = builder.stage_out(compiler, Mode::Std);
builder.clear_if_dirty(&out_dir, &builder.rustc(compiler));

let mut cargo = builder.cargo(compiler, Mode::Std, target, "check");
std_cargo(builder, &compiler, target, &mut cargo);

let _folder = builder.fold_output(|| format!("stage{}-std", compiler.stage));
println!("Checking std artifacts ({} -> {})", &compiler.host, target);
builder.info(&format!("Checking std artifacts ({} -> {})", &compiler.host, target));
run_cargo(builder,
&mut cargo,
vec![],
Expand Down Expand Up @@ -88,15 +85,13 @@ impl Step for Rustc {
let compiler = builder.compiler(0, builder.config.build);
let target = self.target;

let stage_out = builder.stage_out(compiler, Mode::Rustc);
builder.clear_if_dirty(&stage_out, &libstd_stamp(builder, compiler, target));
builder.clear_if_dirty(&stage_out, &libtest_stamp(builder, compiler, target));
builder.ensure(Test { target });

let mut cargo = builder.cargo(compiler, Mode::Rustc, target, "check");
rustc_cargo(builder, &mut cargo);

let _folder = builder.fold_output(|| format!("stage{}-rustc", compiler.stage));
println!("Checking compiler artifacts ({} -> {})", &compiler.host, target);
builder.info(&format!("Checking compiler artifacts ({} -> {})", &compiler.host, target));
run_cargo(builder,
&mut cargo,
vec![],
Expand Down Expand Up @@ -139,8 +134,7 @@ impl Step for CodegenBackend {
let target = self.target;
let backend = self.backend;

let out_dir = builder.cargo_out(compiler, Mode::Codegen, target);
builder.clear_if_dirty(&out_dir, &librustc_stamp(builder, compiler, target));
builder.ensure(Rustc { target });

let mut cargo = builder.cargo(compiler, Mode::Codegen, target, "check");
cargo.arg("--manifest-path").arg(builder.src.join("src/librustc_codegen_llvm/Cargo.toml"));
Expand Down Expand Up @@ -180,14 +174,13 @@ impl Step for Test {
let compiler = builder.compiler(0, builder.config.build);
let target = self.target;

let out_dir = builder.stage_out(compiler, Mode::Test);
builder.clear_if_dirty(&out_dir, &libstd_stamp(builder, compiler, target));
builder.ensure(Std { target });

let mut cargo = builder.cargo(compiler, Mode::Test, target, "check");
test_cargo(builder, &compiler, target, &mut cargo);

let _folder = builder.fold_output(|| format!("stage{}-test", compiler.stage));
println!("Checking test artifacts ({} -> {})", &compiler.host, target);
builder.info(&format!("Checking test artifacts ({} -> {})", &compiler.host, target));
run_cargo(builder,
&mut cargo,
vec![],
Expand Down Expand Up @@ -223,10 +216,7 @@ impl Step for Rustdoc {
let compiler = builder.compiler(0, builder.config.build);
let target = self.target;

let stage_out = builder.stage_out(compiler, Mode::ToolRustc);
builder.clear_if_dirty(&stage_out, &libstd_stamp(builder, compiler, target));
builder.clear_if_dirty(&stage_out, &libtest_stamp(builder, compiler, target));
builder.clear_if_dirty(&stage_out, &librustc_stamp(builder, compiler, target));
builder.ensure(Rustc { target });

let mut cargo = prepare_tool_cargo(builder,
compiler,
Expand All @@ -246,12 +236,7 @@ impl Step for Rustdoc {

let libdir = builder.sysroot_libdir(compiler, target);
add_to_sysroot(&builder, &libdir, &rustdoc_stamp(builder, compiler, target));

builder.ensure(tool::CleanTools {
compiler,
target,
cause: Mode::Rustc,
});
builder.cargo(compiler, Mode::ToolRustc, target, "clean");
}
}

Expand Down
28 changes: 4 additions & 24 deletions src/bootstrap/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ use serde_json;
use util::{exe, libdir, is_dylib, CiEnv};
use {Compiler, Mode};
use native;
use tool;

use cache::{INTERNER, Interned};
use builder::{Step, RunConfig, ShouldRun, Builder};
Expand Down Expand Up @@ -107,8 +106,6 @@ impl Step for Std {
copy_musl_third_party_objects(builder, target, &libdir);
}

let out_dir = builder.cargo_out(compiler, Mode::Std, target);
builder.clear_if_dirty(&out_dir, &builder.rustc(compiler));
let mut cargo = builder.cargo(compiler, Mode::Std, target, "build");
std_cargo(builder, &compiler, target, &mut cargo);

Expand Down Expand Up @@ -246,11 +243,7 @@ impl Step for StdLink {
copy_apple_sanitizer_dylibs(builder, &builder.native_dir(target), "osx", &libdir);
}

builder.ensure(tool::CleanTools {
compiler: target_compiler,
target,
cause: Mode::Std,
});
builder.cargo(target_compiler, Mode::ToolStd, target, "clean");
Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this (and similar statements below) actually necessary? I would've hoped this would happen on its own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. It looks like some steps are missed when we don't explicitly call Builder::cargo here. i.e running ./x.py test src/bootstrap without doing this shows that some steps were actually not called. I'm not completely sure about this though, maybe I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

Step resolution shouldn't depend on files existing on the system - do you happen to have a concrete example? If we actually do need this then can we factor out the cleaning logic from Builder::cargo and call that separately here? It feels a little odd to be creating the full command and then not running it (i.e., using implicit side-effects).

Copy link
Member

Choose a reason for hiding this comment

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

If you don't feel like dealing with this though feel free to let me know -- I'm fine with leaving these in for now

Copy link
Contributor Author

@collin5 collin5 Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, i.e removing both Builder::cargo https://github.com/rust-lang/rust/pull/52036/files#diff-fa9668c926a2788f7849514fe3ed66a9R248 for StdLink and https://github.com/rust-lang/rust/pull/52036/files#diff-fa9668c926a2788f7849514fe3ed66a9R446 for TestLink then doing ./x.py test src/bootstrap, Assertion

rust/src/bootstrap/builder.rs

Line 1660 in 1ecf692

assert_eq!(
will fail and looking at diff shows one of steps was missed. Perhaps we could factor out cleaning logic as you have suggested above.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's leave this as is for now and look into the details later -- it seems fine to leave this somewhat half done pending a more thorough investigation as to how exactly the dependency edge is created.

}
}

Expand Down Expand Up @@ -387,8 +380,6 @@ impl Step for Test {
return;
}

let out_dir = builder.cargo_out(compiler, Mode::Test, target);
builder.clear_if_dirty(&out_dir, &libstd_stamp(builder, compiler, target));
let mut cargo = builder.cargo(compiler, Mode::Test, target, "build");
test_cargo(builder, &compiler, target, &mut cargo);

Expand Down Expand Up @@ -448,11 +439,8 @@ impl Step for TestLink {
target));
add_to_sysroot(builder, &builder.sysroot_libdir(target_compiler, target),
&libtest_stamp(builder, compiler, target));
builder.ensure(tool::CleanTools {
compiler: target_compiler,
target,
cause: Mode::Test,
});

builder.cargo(target_compiler, Mode::ToolTest, target, "clean");
}
}

Expand Down Expand Up @@ -519,9 +507,6 @@ impl Step for Rustc {
compiler: builder.compiler(self.compiler.stage, builder.config.build),
target: builder.config.build,
});
let cargo_out = builder.cargo_out(compiler, Mode::Rustc, target);
builder.clear_if_dirty(&cargo_out, &libstd_stamp(builder, compiler, target));
builder.clear_if_dirty(&cargo_out, &libtest_stamp(builder, compiler, target));

let mut cargo = builder.cargo(compiler, Mode::Rustc, target, "build");
rustc_cargo(builder, &mut cargo);
Expand Down Expand Up @@ -613,11 +598,7 @@ impl Step for RustcLink {
target));
add_to_sysroot(builder, &builder.sysroot_libdir(target_compiler, target),
&librustc_stamp(builder, compiler, target));
builder.ensure(tool::CleanTools {
compiler: target_compiler,
target,
cause: Mode::Rustc,
});
builder.cargo(target_compiler, Mode::ToolRustc, target, "clean");
}
}

Expand Down Expand Up @@ -674,7 +655,6 @@ impl Step for CodegenBackend {
}

let out_dir = builder.cargo_out(compiler, Mode::Codegen, target);
builder.clear_if_dirty(&out_dir, &librustc_stamp(builder, compiler, target));
Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this change required? I'd expect that if we change rustc crates we need this in order for the codegen backends to be properly rebuilt.

Copy link
Contributor Author

@collin5 collin5 Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I guess this is being handled by builder::cargo (here -> https://github.com/rust-lang/rust/pull/52036/files#diff-4bc384d35e57dc577b9378d4d733d7c3R758) since we are calling it just below.


let mut cargo = builder.cargo(compiler, Mode::Codegen, target, "rustc");
cargo.arg("--manifest-path")
Expand Down
10 changes: 0 additions & 10 deletions src/bootstrap/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,6 @@ impl Step for Std {
let out = builder.doc_out(target);
t!(fs::create_dir_all(&out));
let compiler = builder.compiler(stage, builder.config.build);
let rustdoc = builder.rustdoc(compiler.host);
let compiler = if builder.force_use_stage1(compiler, target) {
builder.compiler(1, compiler.host)
} else {
Expand All @@ -480,7 +479,6 @@ impl Step for Std {
// This way rustdoc generates output directly into the output, and rustdoc
// will also directly handle merging.
let my_out = builder.crate_doc_out(target);
builder.clear_if_dirty(&my_out, &rustdoc);
t!(symlink_dir_force(&builder.config, &my_out, &out_dir));

let mut cargo = builder.cargo(compiler, Mode::Std, target, "doc");
Expand Down Expand Up @@ -535,7 +533,6 @@ impl Step for Test {
let out = builder.doc_out(target);
t!(fs::create_dir_all(&out));
let compiler = builder.compiler(stage, builder.config.build);
let rustdoc = builder.rustdoc(compiler.host);
let compiler = if builder.force_use_stage1(compiler, target) {
builder.compiler(1, compiler.host)
} else {
Expand All @@ -551,7 +548,6 @@ impl Step for Test {

// See docs in std above for why we symlink
let my_out = builder.crate_doc_out(target);
builder.clear_if_dirty(&my_out, &rustdoc);
t!(symlink_dir_force(&builder.config, &my_out, &out_dir));

let mut cargo = builder.cargo(compiler, Mode::Test, target, "doc");
Expand Down Expand Up @@ -603,7 +599,6 @@ impl Step for WhitelistedRustc {
let out = builder.doc_out(target);
t!(fs::create_dir_all(&out));
let compiler = builder.compiler(stage, builder.config.build);
let rustdoc = builder.rustdoc(compiler.host);
let compiler = if builder.force_use_stage1(compiler, target) {
builder.compiler(1, compiler.host)
} else {
Expand All @@ -619,7 +614,6 @@ impl Step for WhitelistedRustc {

// See docs in std above for why we symlink
let my_out = builder.crate_doc_out(target);
builder.clear_if_dirty(&my_out, &rustdoc);
t!(symlink_dir_force(&builder.config, &my_out, &out_dir));

let mut cargo = builder.cargo(compiler, Mode::Rustc, target, "doc");
Expand Down Expand Up @@ -678,7 +672,6 @@ impl Step for Rustc {

// Get the correct compiler for this stage.
let compiler = builder.compiler(stage, builder.config.build);
let rustdoc = builder.rustdoc(compiler.host);
let compiler = if builder.force_use_stage1(compiler, target) {
builder.compiler(1, compiler.host)
} else {
Expand All @@ -699,7 +692,6 @@ impl Step for Rustc {
// We do not symlink to the same shared folder that already contains std library
// documentation from previous steps as we do not want to include that.
let out_dir = builder.stage_out(compiler, Mode::Rustc).join(target).join("doc");
builder.clear_if_dirty(&out, &rustdoc);
t!(symlink_dir_force(&builder.config, &out, &out_dir));

// Build cargo command.
Expand Down Expand Up @@ -780,7 +772,6 @@ impl Step for Rustdoc {

// Get the correct compiler for this stage.
let compiler = builder.compiler(stage, builder.config.build);
let rustdoc = builder.rustdoc(compiler.host);
let compiler = if builder.force_use_stage1(compiler, target) {
builder.compiler(1, compiler.host)
} else {
Expand All @@ -803,7 +794,6 @@ impl Step for Rustdoc {
.join(target)
.join("doc");
t!(fs::create_dir_all(&out_dir));
builder.clear_if_dirty(&out, &rustdoc);
t!(symlink_dir_force(&builder.config, &out, &out_dir));

// Build cargo command.
Expand Down
2 changes: 2 additions & 0 deletions src/bootstrap/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ pub enum Mode {
/// Compile a tool which uses all libraries we compile (up to rustc).
/// Doesn't use the stage0 compiler libraries like "other", and includes
/// tools like rustdoc, cargo, rls, etc.
ToolTest,
ToolStd,
ToolRustc,
}
Expand Down Expand Up @@ -567,6 +568,7 @@ impl Build {
Mode::Codegen => "-codegen",
Mode::ToolBootstrap => "-bootstrap-tools",
Mode::ToolStd => "-tools",
Mode::ToolTest => "-tools",
Mode::ToolRustc => "-tools",
};
self.out.join(&*compiler.host)
Expand Down
Loading