Rollup merge of #73352 - ehuss:bootstrap-metadata, r=Mark-Simulacrum

Speed up bootstrap a little.

The bootstrap script was calling `cargo metadata` 3 times (or 6 with `-v`). This is a very expensive operation, and this attempts to avoid the extra calls. On my system, a simple command like `./x.py test -h -v` goes from about 3 seconds to 0.4.

An overview of the changes:

- Call `cargo metadata` only once with `--no-deps`. Optional dependencies are filtered in `in_tree_crates` (handling `profiler_builtins` and `rustc_codegen_llvm` which are driven by the config).
- Remove a duplicate call to `metadata::build` when using `-v`. I'm not sure why it was there, it looks like a mistake or vestigial from previous behavior.
- Remove check for `_shim`, I believe all the `_shim` crates are now gone.
- Remove check for `rustc_` and `*san` for `test::Crate::should_run`, these are no longer dependencies in the `test` tree.
- Use relative paths in `./x.py test -h -v` output.
- Some code cleanup (remove unnecessary `find_compiler_crates`, etc.).
- Show suite paths (`src/test/ui/...`) in `./x.py test -h -v` output.
- Some doc comments.

Author: RalfJung

Diff for: src/bootstrap/builder.rs

@@ -99,9 +99,21 @@ struct StepDescription {
     name: &'static str,
 }
 
+/// Collection of paths used to match a task rule.
 #[derive(Debug, Clone, PartialOrd, Ord, PartialEq, Eq)]
 pub enum PathSet {
+    /// A collection of individual paths.
+    ///
+    /// These are generally matched as a path suffix. For example, a
+    /// command-line value of `libstd` will match if `src/libstd` is in the
+    /// set.
     Set(BTreeSet<PathBuf>),
+    /// A "suite" of paths.
+    ///
+    /// These can match as a path suffix (like `Set`), or as a prefix. For
+    /// example, a command-line value of `src/test/ui/abi/variadic-ffi.rs`
+    /// will match `src/test/ui`. A command-line value of `ui` would also
+    /// match `src/test/ui`.
     Suite(PathBuf),
 }
 
@@ -251,21 +263,33 @@ 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.
+    /// Indicates it should run if the command-line selects the given crate or
+    /// any of its (local) dependencies.
+    ///
+    /// Compared to `krate`, this treats the dependencies as aliases for the
+    /// same job. Generally it is preferred to use `krate`, and treat each
+    /// individual path separately. For example `./x.py test src/liballoc`
+    /// (which uses `krate`) will test just `liballoc`. However, `./x.py check
+    /// src/liballoc` (which uses `all_krates`) will check all of `libtest`.
+    /// `all_krates` should probably be removed at some point.
     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));
+            let path = krate.local_path(self.builder);
+            set.insert(path);
         }
         self.paths.insert(PathSet::Set(set));
         self
     }
 
+    /// Indicates it should run if the command-line selects the given crate or
+    /// any of its (local) dependencies.
+    ///
+    /// `make_run` will be called separately for each matching command-line path.
     pub fn krate(mut self, name: &str) -> Self {
         for krate in self.builder.in_tree_crates(name) {
-            self.paths.insert(PathSet::one(&krate.path));
+            let path = krate.local_path(self.builder);
+            self.paths.insert(PathSet::one(path));
         }
         self
     }
@@ -488,13 +512,19 @@ impl<'a> Builder<'a> {
             should_run = (desc.should_run)(should_run);
         }
         let mut help = String::from("Available paths:\n");
+        let mut add_path = |path: &Path| {
+            help.push_str(&format!("    ./x.py {} {}\n", subcommand, path.display()));
+        };
         for pathset in should_run.paths {
-            if let PathSet::Set(set) = pathset {
-                set.iter().for_each(|path| {
-                    help.push_str(
-                        format!("    ./x.py {} {}\n", subcommand, path.display()).as_str(),
-                    )
-                })
+            match pathset {
+                PathSet::Set(set) => {
+                    for path in set {
+                        add_path(&path);
+                    }
+                }
+                PathSet::Suite(path) => {
+                    add_path(&path.join("..."));
+                }
             }
         }
         Some(help)

Diff for: src/bootstrap/doc.rs

@@ -548,8 +548,8 @@ impl Step for Rustc {
         // Find dependencies for top level crates.
         let mut compiler_crates = HashSet::new();
         for root_crate in &["rustc_driver", "rustc_codegen_llvm", "rustc_codegen_ssa"] {
-            let interned_root_crate = INTERNER.intern_str(root_crate);
-            find_compiler_crates(builder, &interned_root_crate, &mut compiler_crates);
+            compiler_crates
+                .extend(builder.in_tree_crates(root_crate).into_iter().map(|krate| krate.name));
         }
 
         for krate in &compiler_crates {
@@ -564,22 +564,6 @@ impl Step for Rustc {
     }
 }
 
-fn find_compiler_crates(
-    builder: &Builder<'_>,
-    name: &Interned<String>,
-    crates: &mut HashSet<Interned<String>>,
-) {
-    // Add current crate.
-    crates.insert(*name);
-
-    // Look for dependencies.
-    for dep in builder.crates.get(name).unwrap().deps.iter() {
-        if builder.crates.get(dep).unwrap().is_local(builder) {
-            find_compiler_crates(builder, dep, crates);
-        }
-    }
-}
-
 #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
 pub struct Rustdoc {
     stage: u32,

Diff for: src/bootstrap/flags.rs

@@ -10,12 +10,10 @@ use std::process;
 use getopts::Options;
 
 use crate::builder::Builder;
+use crate::cache::{Interned, INTERNER};
 use crate::config::Config;
-use crate::metadata;
 use crate::{Build, DocTests};
 
-use crate::cache::{Interned, INTERNER};
-
 /// Deserialized version of all flags for this compile.
 pub struct Flags {
     pub verbose: usize, // number of -v args; each extra -v after the first is passed to Cargo
@@ -443,8 +441,7 @@ Arguments:
         // All subcommands except `clean` can have an optional "Available paths" section
         if matches.opt_present("verbose") {
             let config = Config::parse(&["build".to_string()]);
-            let mut build = Build::new(config);
-            metadata::build(&mut build);
+            let build = Build::new(config);
 
             let maybe_rules_help = Builder::get_help(&build, subcommand.as_str());
             extra_help.push_str(maybe_rules_help.unwrap_or_default().as_str());

Diff for: src/bootstrap/lib.rs

@@ -270,12 +270,7 @@ struct Crate {
 }
 
 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()
     }
 }
@@ -1090,17 +1085,29 @@ impl Build {
         }
     }
 
+    /// Returns a Vec of all the dependencies of the given root crate,
+    /// including transitive dependencies and the root itself. Only includes
+    /// "local" crates (those in the local source tree, not from a registry).
     fn in_tree_crates(&self, root: &str) -> Vec<&Crate> {
         let mut ret = Vec::new();
         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 krate.is_local(self) {
-                ret.push(krate);
-            }
+            ret.push(krate);
             for dep in &krate.deps {
-                if visited.insert(dep) && dep != "build_helper" {
+                // Don't include optional deps if their features are not
+                // enabled. Ideally this would be computed from `cargo
+                // metadata --features …`, but that is somewhat slow. Just
+                // skip `build_helper` since there aren't any operations we
+                // want to perform on it. In the future, we may want to
+                // consider just filtering all build and dev dependencies in
+                // metadata::build.
+                if visited.insert(dep)
+                    && dep != "build_helper"
+                    && (dep != "profiler_builtins" || self.config.profiler)
+                    && (dep != "rustc_codegen_llvm" || self.config.llvm_enabled())
+                {
                     list.push(*dep);
                 }
             }

Diff for: src/bootstrap/metadata.rs

@@ -1,5 +1,3 @@
-use std::collections::HashMap;
-use std::collections::HashSet;
 use std::path::PathBuf;
 use std::process::Command;
 
@@ -12,7 +10,6 @@ use crate::{Build, Crate};
 #[derive(Deserialize)]
 struct Output {
     packages: Vec<Package>,
-    resolve: Resolve,
 }
 
 #[derive(Deserialize)]
@@ -21,72 +18,39 @@ struct Package {
     name: String,
     source: Option<String>,
     manifest_path: String,
+    dependencies: Vec<Dependency>,
 }
 
 #[derive(Deserialize)]
-struct Resolve {
-    nodes: Vec<ResolveNode>,
-}
-
-#[derive(Deserialize)]
-struct ResolveNode {
-    id: String,
-    dependencies: Vec<String>,
+struct Dependency {
+    name: String,
+    source: Option<String>,
 }
 
 pub fn build(build: &mut Build) {
-    let mut resolves = Vec::new();
-    build_krate(&build.std_features(), build, &mut resolves, "src/libstd");
-    build_krate("", build, &mut resolves, "src/libtest");
-    build_krate(&build.rustc_features(), build, &mut resolves, "src/rustc");
-
-    let mut id2name = HashMap::with_capacity(build.crates.len());
-    for (name, krate) in build.crates.iter() {
-        id2name.insert(krate.id.clone(), name.clone());
-    }
-
-    for node in resolves {
-        let name = match id2name.get(&node.id) {
-            Some(name) => name,
-            None => continue,
-        };
-
-        let krate = build.crates.get_mut(name).unwrap();
-        for dep in node.dependencies.iter() {
-            let dep = match id2name.get(dep) {
-                Some(dep) => dep,
-                None => continue,
-            };
-            krate.deps.insert(*dep);
-        }
-    }
-}
-
-fn build_krate(features: &str, build: &mut Build, resolves: &mut Vec<ResolveNode>, krate: &str) {
     // Run `cargo metadata` to figure out what crates we're testing.
-    //
-    // Down below we're going to call `cargo test`, but to test the right set
-    // of packages we're going to have to know what `-p` arguments to pass it
-    // to know what crates to test. Here we run `cargo metadata` to learn about
-    // the dependency graph and what `-p` arguments there are.
     let mut cargo = Command::new(&build.initial_cargo);
     cargo
         .arg("metadata")
         .arg("--format-version")
         .arg("1")
-        .arg("--features")
-        .arg(features)
+        .arg("--no-deps")
         .arg("--manifest-path")
-        .arg(build.src.join(krate).join("Cargo.toml"));
+        .arg(build.src.join("Cargo.toml"));
     let output = output(&mut cargo);
     let output: Output = serde_json::from_str(&output).unwrap();
     for package in output.packages {
         if package.source.is_none() {
             let name = INTERNER.intern_string(package.name);
             let mut path = PathBuf::from(package.manifest_path);
             path.pop();
-            build.crates.insert(name, Crate { name, id: package.id, deps: HashSet::new(), path });
+            let deps = package
+                .dependencies
+                .into_iter()
+                .filter(|dep| dep.source.is_none())
+                .map(|dep| INTERNER.intern_string(dep.name))
+                .collect();
+            build.crates.insert(name, Crate { name, id: package.id, deps, path });
         }
     }
-    resolves.extend(output.resolve.nodes);
 }

Diff for: src/bootstrap/test.rs

@@ -1648,14 +1648,8 @@ impl Step for Crate {
     type Output = ();
     const DEFAULT: bool = true;
 
-    fn should_run(mut run: ShouldRun<'_>) -> ShouldRun<'_> {
-        let builder = run.builder;
-        for krate in run.builder.in_tree_crates("test") {
-            if !(krate.name.starts_with("rustc_") && krate.name.ends_with("san")) {
-                run = run.path(krate.local_path(&builder).to_str().unwrap());
-            }
-        }
-        run
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.krate("test")
     }
 
     fn make_run(run: RunConfig<'_>) {