Skip to content

Require all paths passed to ShouldRun::paths to exist on disk #95906

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 18, 2022
Merged

Require all paths passed to ShouldRun::paths to exist on disk #95906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 24 additions & 1 deletion src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,19 @@ impl<'a> ShouldRun<'a> {
self
}

// single alias, which does not correspond to any on-disk path
pub fn alias(mut self, alias: &str) -> Self {
assert!(
!self.builder.src.join(alias).exists(),
"use `builder.path()` for real paths: {}",
alias
);
self.paths.insert(PathSet::Set(
std::iter::once(TaskPath { path: alias.into(), kind: Some(self.kind) }).collect(),
));
self
}

// single, non-aliased path
pub fn path(self, path: &str) -> Self {
self.paths(&[path])
Expand All @@ -372,7 +385,17 @@ impl<'a> ShouldRun<'a> {
// multiple aliases for the same job
pub fn paths(mut self, paths: &[&str]) -> Self {
self.paths.insert(PathSet::Set(
paths.iter().map(|p| TaskPath { path: p.into(), kind: Some(self.kind) }).collect(),
paths
.iter()
.map(|p| {
assert!(
self.builder.src.join(p).exists(),
"`should_run.paths` should correspond to real on-disk paths - use `alias` if there is no relevant path: {}",
p
);
TaskPath { path: p.into(), kind: Some(self.kind) }
})
.collect(),
));
self
}
Expand Down
45 changes: 23 additions & 22 deletions src/bootstrap/dist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ impl Step for Docs {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = run.builder.config.docs;
run.path("rust-docs").default_condition(default)
run.alias("rust-docs").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -94,7 +94,7 @@ impl Step for RustcDocs {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let builder = run.builder;
run.path("rustc-docs").default_condition(builder.config.compiler_docs)
run.alias("rustc-docs").default_condition(builder.config.compiler_docs)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -272,7 +272,7 @@ impl Step for Mingw {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("rust-mingw")
run.alias("rust-mingw")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -313,7 +313,7 @@ impl Step for Rustc {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("rustc")
run.alias("rustc")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -456,7 +456,7 @@ impl Step for DebuggerScripts {
type Output = ();

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("src/lldb_batchmode.py")
run.path("src/etc/lldb_batchmode.py")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -547,7 +547,7 @@ impl Step for Std {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("rust-std")
run.alias("rust-std")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -594,7 +594,7 @@ impl Step for RustcDev {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("rustc-dev")
run.alias("rustc-dev")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -653,7 +653,7 @@ impl Step for Analysis {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "analysis");
run.path("rust-analysis").default_condition(default)
run.alias("rust-analysis").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -790,7 +790,7 @@ impl Step for Src {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("rust-src")
run.alias("rust-src")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -848,7 +848,7 @@ impl Step for PlainSourceTarball {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let builder = run.builder;
run.path("rustc-src").default_condition(builder.config.rust_dist_src)
run.alias("rustc-src").default_condition(builder.config.rust_dist_src)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -942,7 +942,7 @@ impl Step for Cargo {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "cargo");
run.path("cargo").default_condition(default)
run.alias("cargo").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -998,7 +998,7 @@ impl Step for Rls {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "rls");
run.path("rls").default_condition(default)
run.alias("rls").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1045,7 +1045,7 @@ impl Step for RustAnalyzer {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "rust-analyzer");
run.path("rust-analyzer").default_condition(default)
run.alias("rust-analyzer").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1101,7 +1101,7 @@ impl Step for Clippy {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "clippy");
run.path("clippy").default_condition(default)
run.alias("clippy").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1152,7 +1152,7 @@ impl Step for Miri {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "miri");
run.path("miri").default_condition(default)
run.alias("miri").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1212,7 +1212,7 @@ impl Step for Rustfmt {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "rustfmt");
run.path("rustfmt").default_condition(default)
run.alias("rustfmt").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1271,7 +1271,7 @@ impl Step for RustDemangler {
// we run the step by default when only `extended = true`, and decide whether to actually
// run it or not later.
let default = run.builder.config.extended;
run.path("rust-demangler").default_condition(default)
run.alias("rust-demangler").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1324,7 +1324,7 @@ impl Step for Extended {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let builder = run.builder;
run.path("extended").default_condition(builder.config.extended)
run.alias("extended").default_condition(builder.config.extended)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -1968,7 +1968,8 @@ impl Step for LlvmTools {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let default = should_build_extended_tool(&run.builder, "llvm-tools");
run.path("llvm-tools").default_condition(default)
// FIXME: allow using the names of the tools themselves?
run.alias("llvm-tools").default_condition(default)
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -2022,7 +2023,7 @@ impl Step for RustDev {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("rust-dev")
run.alias("rust-dev")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -2098,7 +2099,7 @@ impl Step for BuildManifest {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("build-manifest")
run.alias("build-manifest")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -2130,7 +2131,7 @@ impl Step for ReproducibleArtifacts {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("reproducible-artifacts")
run.alias("reproducible-artifacts")
}

fn make_run(run: RunConfig<'_>) {
Expand Down
28 changes: 14 additions & 14 deletions src/bootstrap/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ fn prepare_dir(mut path: PathBuf) -> String {
macro_rules! install {
(($sel:ident, $builder:ident, $_config:ident),
$($name:ident,
$path:expr,
$condition_name: ident = $path_or_alias: literal,
$default_cond:expr,
only_hosts: $only_hosts:expr,
$run_item:block $(, $c:ident)*;)+) => {
Expand All @@ -108,7 +108,7 @@ macro_rules! install {
#[allow(dead_code)]
fn should_build(config: &Config) -> bool {
config.extended && config.tools.as_ref()
.map_or(true, |t| t.contains($path))
.map_or(true, |t| t.contains($path_or_alias))
}
}

Expand All @@ -120,7 +120,7 @@ macro_rules! install {

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
let $_config = &run.builder.config;
run.path($path).default_condition($default_cond)
run.$condition_name($path_or_alias).default_condition($default_cond)
}

fn make_run(run: RunConfig<'_>) {
Expand All @@ -138,11 +138,11 @@ macro_rules! install {
}

install!((self, builder, _config),
Docs, "src/doc", _config.docs, only_hosts: false, {
Docs, path = "src/doc", _config.docs, only_hosts: false, {
let tarball = builder.ensure(dist::Docs { host: self.target }).expect("missing docs");
install_sh(builder, "docs", self.compiler.stage, Some(self.target), &tarball);
};
Std, "library/std", true, only_hosts: false, {
Std, path = "library/std", true, only_hosts: false, {
for target in &builder.targets {
// `expect` should be safe, only None when host != build, but this
// only runs when host == build
Expand All @@ -153,13 +153,13 @@ install!((self, builder, _config),
install_sh(builder, "std", self.compiler.stage, Some(*target), &tarball);
}
};
Cargo, "cargo", Self::should_build(_config), only_hosts: true, {
Cargo, alias = "cargo", Self::should_build(_config), only_hosts: true, {
let tarball = builder
.ensure(dist::Cargo { compiler: self.compiler, target: self.target })
.expect("missing cargo");
install_sh(builder, "cargo", self.compiler.stage, Some(self.target), &tarball);
};
Rls, "rls", Self::should_build(_config), only_hosts: true, {
Rls, alias = "rls", Self::should_build(_config), only_hosts: true, {
if let Some(tarball) = builder.ensure(dist::Rls { compiler: self.compiler, target: self.target }) {
install_sh(builder, "rls", self.compiler.stage, Some(self.target), &tarball);
} else {
Expand All @@ -168,7 +168,7 @@ install!((self, builder, _config),
);
}
};
RustAnalyzer, "rust-analyzer", Self::should_build(_config), only_hosts: true, {
RustAnalyzer, alias = "rust-analyzer", Self::should_build(_config), only_hosts: true, {
if let Some(tarball) =
builder.ensure(dist::RustAnalyzer { compiler: self.compiler, target: self.target })
{
Expand All @@ -179,13 +179,13 @@ install!((self, builder, _config),
);
}
};
Clippy, "clippy", Self::should_build(_config), only_hosts: true, {
Clippy, alias = "clippy", Self::should_build(_config), only_hosts: true, {
let tarball = builder
.ensure(dist::Clippy { compiler: self.compiler, target: self.target })
.expect("missing clippy");
install_sh(builder, "clippy", self.compiler.stage, Some(self.target), &tarball);
};
Miri, "miri", Self::should_build(_config), only_hosts: true, {
Miri, alias = "miri", Self::should_build(_config), only_hosts: true, {
if let Some(tarball) = builder.ensure(dist::Miri { compiler: self.compiler, target: self.target }) {
install_sh(builder, "miri", self.compiler.stage, Some(self.target), &tarball);
} else {
Expand All @@ -194,7 +194,7 @@ install!((self, builder, _config),
);
}
};
Rustfmt, "rustfmt", Self::should_build(_config), only_hosts: true, {
Rustfmt, alias = "rustfmt", Self::should_build(_config), only_hosts: true, {
if let Some(tarball) = builder.ensure(dist::Rustfmt {
compiler: self.compiler,
target: self.target
Expand All @@ -206,7 +206,7 @@ install!((self, builder, _config),
);
}
};
RustDemangler, "rust-demangler", Self::should_build(_config), only_hosts: true, {
RustDemangler, alias = "rust-demangler", Self::should_build(_config), only_hosts: true, {
// Note: Even though `should_build` may return true for `extended` default tools,
// dist::RustDemangler may still return None, unless the target-dependent `profiler` config
// is also true, or the `tools` array explicitly includes "rust-demangler".
Expand All @@ -222,7 +222,7 @@ install!((self, builder, _config),
);
}
};
Analysis, "analysis", Self::should_build(_config), only_hosts: false, {
Analysis, alias = "analysis", Self::should_build(_config), only_hosts: false, {
// `expect` should be safe, only None with host != build, but this
// only uses the `build` compiler
let tarball = builder.ensure(dist::Analysis {
Expand All @@ -234,7 +234,7 @@ install!((self, builder, _config),
}).expect("missing analysis");
install_sh(builder, "analysis", self.compiler.stage, Some(self.target), &tarball);
};
Rustc, "src/librustc", true, only_hosts: true, {
Rustc, path = "compiler/rustc", true, only_hosts: true, {
Copy link
Member

Choose a reason for hiding this comment

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

x.py install is a bit of a mess I guess between paths and aliases. My tendency is to move towards the dist tarball names for simplicity (and maybe just universally support any of them; I suspect it might not be that hard with the relatively uniform tarball generation work @pietroalbini did a while back).

Can be a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my impression is that x.py install isn't a super well trodden path in general - not sure who's using it. I can look into cleaning it up sometime in the next few weeks, I've been meaning to write up instructions for how to port a new architecture anyway and I suspect this will come up as part of the work. Leaving it be for now.

let tarball = builder.ensure(dist::Rustc {
compiler: builder.compiler(builder.top_stage, self.target),
});
Expand Down
6 changes: 3 additions & 3 deletions src/bootstrap/native.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl Step for Llvm {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("src/llvm-project").path("src/llvm-project/llvm").path("src/llvm")
run.path("src/llvm-project").path("src/llvm-project/llvm")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -605,7 +605,7 @@ impl Step for Lld {
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("src/llvm-project/lld").path("src/tools/lld")
run.path("src/llvm-project/lld")
}

fn make_run(run: RunConfig<'_>) {
Expand Down Expand Up @@ -771,7 +771,7 @@ impl Step for Sanitizers {
type Output = Vec<SanitizerRuntime>;

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("src/llvm-project/compiler-rt").path("src/sanitizers")
run.alias("sanitizers")
}

fn make_run(run: RunConfig<'_>) {
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2295,7 +2295,7 @@ impl Step for Distcheck {
type Output = ();

fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
run.path("distcheck")
run.alias("distcheck")
}

fn make_run(run: RunConfig<'_>) {
Expand Down
Loading