Skip to content

Commit

Permalink
Change Step to be invoked with a path when in default mode.
Browse files Browse the repository at this point in the history
Previously, a Step would be able to tell on its own when it was invoked
"by-default" (that is, `./x.py test` was called instead of `./x.py test
some/path`). This commit replaces that functionality, invoking each Step
with each of the paths it has specified as "should be invoked by."

For example, if a step calls `path("src/tools/cargo")` and
`path("src/doc/cargo")` then it's make_run will be called twice, with
"src/tools/cargo" and "src/doc/cargo." This makes it so that default
handling logic is in builder, instead of spread across various Steps.

However, this meant that some Step specifications needed to be updated,
since for example `rustdoc` can be built by `./x.py build
src/librustdoc` or `./x.py build src/tools/rustdoc`. A `PathSet`
abstraction is added that handles this: now, each Step can not only list
`path(...)` but also `paths(&[a, b, ...])` which will make it so that we
don't invoke it with each of the individual paths, instead invoking it
with the first path in the list (though this shouldn't be depended on).

Future work likely consists of implementing a better/easier way for a
given Step to work with "any" crate in-tree, especially those that want
to run tests, build, or check crates in the std, test, or rustc crate
trees. Currently this is rather painful to do as most of the logic is
duplicated across should_run and make_run. It seems likely this can be
abstracted away into builder somehow.
  • Loading branch information
Mark-Simulacrum committed Feb 11, 2018
1 parent 55c36e3 commit f104b12
Show file tree
Hide file tree
Showing 7 changed files with 415 additions and 312 deletions.
125 changes: 85 additions & 40 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub struct RunConfig<'a> {
pub builder: &'a Builder<'a>,
pub host: Interned<String>,
pub target: Interned<String>,
pub path: Option<&'a Path>,
pub path: &'a Path,
}

struct StepDescription {
Expand All @@ -105,6 +105,28 @@ struct StepDescription {
only_build: bool,
should_run: fn(ShouldRun) -> ShouldRun,
make_run: fn(RunConfig),
name: &'static str,
}

#[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)]
struct PathSet {
set: BTreeSet<PathBuf>,
}

impl PathSet {
fn one<P: Into<PathBuf>>(path: P) -> PathSet {
let mut set = BTreeSet::new();
set.insert(path.into());
PathSet { set }
}

fn has(&self, needle: &Path) -> bool {
self.set.iter().any(|p| p.ends_with(needle))
}

fn path(&self) -> &Path {
self.set.iter().next().unwrap()
}
}

impl StepDescription {
Expand All @@ -116,30 +138,17 @@ impl StepDescription {
only_build: S::ONLY_BUILD,
should_run: S::should_run,
make_run: S::make_run,
name: unsafe { ::std::intrinsics::type_name::<S>() },
}
}

fn maybe_run(&self, builder: &Builder, should_run: &ShouldRun, path: Option<&Path>) {
if let Some(path) = path {
if builder.config.exclude.iter().any(|e| e == path) {
eprintln!("Skipping {:?} because this path is excluded", path);
return;
} else if !builder.config.exclude.is_empty() {
eprintln!("{:?} not skipped -- not in {:?}", path, builder.config.exclude);
}
} else {
if !should_run.paths.is_empty() {
if should_run.paths.iter().all(|p| builder.config.exclude.contains(&p)) {
eprintln!("Skipping because all of its paths ({:?}) are excluded",
should_run.paths);
return;
} else if should_run.paths.len() > 1 {
for path in &should_run.paths {
self.maybe_run(builder, should_run, Some(path));
}
return;
}
}
fn maybe_run(&self, builder: &Builder, pathset: &PathSet) {
if builder.config.exclude.iter().any(|e| pathset.has(e)) {
eprintln!("Skipping {:?} because it is excluded", pathset);
return;
} else if !builder.config.exclude.is_empty() {
eprintln!("{:?} not skipped for {:?} -- not in {:?}", pathset,
self.name, builder.config.exclude);
}
let build = builder.build;
let hosts = if self.only_build_targets || self.only_build {
Expand All @@ -165,7 +174,7 @@ impl StepDescription {
for target in targets {
let run = RunConfig {
builder,
path,
path: pathset.path(),
host: *host,
target: *target,
};
Expand All @@ -178,19 +187,28 @@ impl StepDescription {
let should_runs = v.iter().map(|desc| {
(desc.should_run)(ShouldRun::new(builder))
}).collect::<Vec<_>>();

// sanity checks on rules
for (desc, should_run) in v.iter().zip(&should_runs) {
assert!(!should_run.paths.is_empty(),
"{:?} should have at least one pathset", desc.name);
}

if paths.is_empty() {
for (desc, should_run) in v.iter().zip(should_runs) {
if desc.default && should_run.is_really_default {
desc.maybe_run(builder, &should_run, None);
for pathset in &should_run.paths {
desc.maybe_run(builder, pathset);
}
}
}
} else {
for path in paths {
let mut attempted_run = false;
for (desc, should_run) in v.iter().zip(&should_runs) {
if should_run.run(path) {
if let Some(pathset) = should_run.pathset_for_path(path) {
attempted_run = true;
desc.maybe_run(builder, &should_run, Some(path));
desc.maybe_run(builder, pathset);
}
}

Expand All @@ -206,7 +224,7 @@ impl StepDescription {
pub struct ShouldRun<'a> {
pub builder: &'a Builder<'a>,
// use a BTreeSet to maintain sort order
paths: BTreeSet<PathBuf>,
paths: BTreeSet<PathSet>,

// If this is a default rule, this is an additional constraint placed on
// it's run. Generally something like compiler docs being enabled.
Expand All @@ -227,15 +245,35 @@ impl<'a> ShouldRun<'a> {
self
}

// Unlike `krate` this will create just one pathset. As such, it probably shouldn't actually
// ever be used, but as we transition to having all rules properly handle passing krate(...) by
// actually doing something different for every crate passed.
pub fn all_krates(mut self, name: &str) -> Self {
let mut set = BTreeSet::new();
for krate in self.builder.in_tree_crates(name) {
set.insert(PathBuf::from(&krate.path));
}
self.paths.insert(PathSet { set });
self
}

pub fn krate(mut self, name: &str) -> Self {
for (_, krate_path) in self.builder.crates(name) {
self.paths.insert(t!(env::current_dir()).join(krate_path));
for krate in self.builder.in_tree_crates(name) {
self.paths.insert(PathSet::one(&krate.path));
}
self
}

pub fn path(mut self, path: &str) -> Self {
self.paths.insert(t!(env::current_dir()).join(path));
// single, non-aliased path
pub fn path(self, path: &str) -> Self {
self.paths(&[path])
}

// multiple aliases for the same job
pub fn paths(mut self, paths: &[&str]) -> Self {
self.paths.insert(PathSet {
set: paths.iter().map(PathBuf::from).collect(),
});
self
}

Expand All @@ -244,8 +282,8 @@ impl<'a> ShouldRun<'a> {
self
}

fn run(&self, path: &Path) -> bool {
self.paths.iter().any(|p| path.ends_with(p))
fn pathset_for_path(&self, path: &Path) -> Option<&PathSet> {
self.paths.iter().find(|pathset| pathset.has(path))
}
}

Expand Down Expand Up @@ -275,11 +313,16 @@ impl<'a> Builder<'a> {
tool::RustInstaller, tool::Cargo, tool::Rls, tool::Rustdoc, tool::Clippy,
native::Llvm, tool::Rustfmt, tool::Miri),
Kind::Check => describe!(check::Std, check::Test, check::Rustc),
Kind::Test => describe!(test::Tidy, test::Bootstrap, test::DefaultCompiletest,
test::HostCompiletest, test::Crate, test::CrateLibrustc, test::Rustdoc,
test::Linkcheck, test::Cargotest, test::Cargo, test::Rls, test::Docs,
test::ErrorIndex, test::Distcheck, test::Rustfmt, test::Miri, test::Clippy,
test::RustdocJS, test::RustdocTheme),
Kind::Test => describe!(test::Tidy, test::Bootstrap, test::Ui, test::RunPass,
test::CompileFail, test::ParseFail, test::RunFail, test::RunPassValgrind,
test::MirOpt, test::Codegen, test::CodegenUnits, test::Incremental, test::Debuginfo,
test::UiFullDeps, test::RunPassFullDeps, test::RunFailFullDeps,
test::CompileFailFullDeps, test::IncrementalFullDeps, test::Rustdoc, test::Pretty,
test::RunPassPretty, test::RunFailPretty, test::RunPassValgrindPretty,
test::RunPassFullDepsPretty, test::RunFailFullDepsPretty, test::RunMake,
test::Crate, test::CrateLibrustc, test::Rustdoc, test::Linkcheck, test::Cargotest,
test::Cargo, test::Rls, test::Docs, test::ErrorIndex, test::Distcheck,
test::Rustfmt, test::Miri, test::Clippy, test::RustdocJS, test::RustdocTheme),
Kind::Bench => describe!(test::Crate, test::CrateLibrustc),
Kind::Doc => describe!(doc::UnstableBook, doc::UnstableBookGen, doc::TheBook,
doc::Standalone, doc::Std, doc::Test, doc::Rustc, doc::ErrorIndex, doc::Nomicon,
Expand Down Expand Up @@ -317,8 +360,10 @@ impl<'a> Builder<'a> {
should_run = (desc.should_run)(should_run);
}
let mut help = String::from("Available paths:\n");
for path in should_run.paths {
help.push_str(format!(" ./x.py {} {}\n", subcommand, path.display()).as_str());
for pathset in should_run.paths {
for path in pathset.set {
help.push_str(format!(" ./x.py {} {}\n", subcommand, path.display()).as_str());
}
}
Some(help)
}
Expand Down
6 changes: 3 additions & 3 deletions src/bootstrap/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl Step for Std {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/libstd").krate("std")
run.all_krates("std")
}

fn make_run(run: RunConfig) {
Expand Down Expand Up @@ -67,7 +67,7 @@ impl Step for Rustc {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/librustc").krate("rustc-main")
run.all_krates("rustc-main")
}

fn make_run(run: RunConfig) {
Expand Down Expand Up @@ -114,7 +114,7 @@ impl Step for Test {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/libtest").krate("test")
run.all_krates("test")
}

fn make_run(run: RunConfig) {
Expand Down
10 changes: 5 additions & 5 deletions src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl Step for Std {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/libstd").krate("std")
run.all_krates("std")
}

fn make_run(run: RunConfig) {
Expand Down Expand Up @@ -320,7 +320,7 @@ impl Step for Test {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/libtest").krate("test")
run.all_krates("test")
}

fn make_run(run: RunConfig) {
Expand Down Expand Up @@ -436,7 +436,7 @@ impl Step for Rustc {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/librustc").krate("rustc-main")
run.all_krates("rustc-main")
}

fn make_run(run: RunConfig) {
Expand Down Expand Up @@ -593,7 +593,7 @@ impl Step for CodegenBackend {
const DEFAULT: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/librustc_trans")
run.all_krates("rustc_trans")
}

fn make_run(run: RunConfig) {
Expand Down Expand Up @@ -828,7 +828,7 @@ impl Step for Assemble {
type Output = Compiler;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/rustc")
run.all_krates("rustc-main")
}

/// Prepare a new compiler from the artifacts in `stage`
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ impl Step for Std {

fn should_run(run: ShouldRun) -> ShouldRun {
let builder = run.builder;
run.krate("std").default_condition(builder.build.config.docs)
run.all_krates("std").default_condition(builder.build.config.docs)
}

fn make_run(run: RunConfig) {
Expand Down
37 changes: 22 additions & 15 deletions src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,8 @@
//! More documentation can be found in each respective module below, and you can
//! also check out the `src/bootstrap/README.md` file for more information.

#![deny(warnings)]
#![allow(stable_features)]
#![feature(associated_consts)]
//#![deny(warnings)]
#![feature(core_intrinsics)]

#[macro_use]
extern crate build_helper;
Expand Down Expand Up @@ -267,6 +266,18 @@ struct Crate {
bench_step: String,
}

impl Crate {
fn is_local(&self, build: &Build) -> bool {
self.path.starts_with(&build.config.src) &&
!self.path.to_string_lossy().ends_with("_shim")
}

fn local_path(&self, build: &Build) -> PathBuf {
assert!(self.is_local(build));
self.path.strip_prefix(&build.config.src).unwrap().into()
}
}

/// The various "modes" of invoking Cargo.
///
/// These entries currently correspond to the various output directories of the
Expand Down Expand Up @@ -949,22 +960,18 @@ impl Build {
}
}

/// Get a list of crates from a root crate.
///
/// Returns Vec<(crate, path to crate, is_root_crate)>
fn crates(&self, root: &str) -> Vec<(Interned<String>, &Path)> {
let interned = INTERNER.intern_string(root.to_owned());
fn in_tree_crates(&self, root: &str) -> Vec<&Crate> {
let mut ret = Vec::new();
let mut list = vec![interned];
let mut list = vec![INTERNER.intern_str(root)];
let mut visited = HashSet::new();
while let Some(krate) = list.pop() {
let krate = &self.crates[&krate];
// If we can't strip prefix, then out-of-tree path
let path = krate.path.strip_prefix(&self.src).unwrap_or(&krate.path);
ret.push((krate.name, path));
for dep in &krate.deps {
if visited.insert(dep) && dep != "build_helper" {
list.push(*dep);
if krate.is_local(self) {
ret.push(krate);
for dep in &krate.deps {
if visited.insert(dep) && dep != "build_helper" {
list.push(*dep);
}
}
}
}
Expand Down
4 changes: 1 addition & 3 deletions src/bootstrap/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ impl Step for Llvm {
}

fn make_run(run: RunConfig) {
let emscripten = run.path.map(|p| {
p.ends_with("llvm-emscripten")
}).unwrap_or(false);
let emscripten = run.path.ends_with("llvm-emscripten");
run.builder.ensure(Llvm {
target: run.target,
emscripten,
Expand Down
Loading

0 comments on commit f104b12

Please sign in to comment.