Decouple build_unit_dependencies and Context.

Author: ehuss

src/cargo/core/compiler/context/mod.rs

@@ -1,5 +1,5 @@
 #![allow(deprecated)]
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap, HashSet};
 use std::ffi::OsStr;
 use std::fmt::Write;
 use std::path::PathBuf;
@@ -10,7 +10,7 @@ use jobserver::Client;
 
 use crate::core::compiler::compilation;
 use crate::core::compiler::Unit;
-use crate::core::{Package, PackageId, Resolve};
+use crate::core::{PackageId, Resolve};
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::{internal, profile, Config};
 
@@ -21,9 +21,6 @@ use super::job_queue::JobQueue;
 use super::layout::Layout;
 use super::{BuildContext, Compilation, CompileMode, Executor, FileFlavor, Kind};
 
-mod unit_dependencies;
-use self::unit_dependencies::build_unit_dependencies;
-
 mod compilation_files;
 use self::compilation_files::CompilationFiles;
 pub use self::compilation_files::{Metadata, OutputFile};
@@ -66,8 +63,6 @@ pub struct Context<'a, 'cfg> {
     /// the compilation. This is `None` until after `unit_dependencies` has
     /// been computed.
     files: Option<CompilationFiles<'a, 'cfg>>,
-    /// Cache of packages, populated when they are downloaded.
-    package_cache: HashMap<PackageId, &'a Package>,
 
     /// A flag indicating whether pipelining is enabled for this compilation
     /// session. Pipelining largely only affects the edges of the dependency
@@ -82,7 +77,11 @@ pub struct Context<'a, 'cfg> {
 }
 
 impl<'a, 'cfg> Context<'a, 'cfg> {
-    pub fn new(config: &'cfg Config, bcx: &'a BuildContext<'a, 'cfg>) -> CargoResult<Self> {
+    pub fn new(
+        config: &'cfg Config,
+        bcx: &'a BuildContext<'a, 'cfg>,
+        unit_dependencies: HashMap<Unit<'a>, Vec<Unit<'a>>>,
+    ) -> CargoResult<Self> {
         // Load up the jobserver that we'll use to manage our parallelism. This
         // is the same as the GNU make implementation of a jobserver, and
         // intentionally so! It's hoped that we can interact with GNU make and
@@ -115,9 +114,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             links: Links::new(),
             jobserver,
             primary_packages: HashSet::new(),
-            unit_dependencies: HashMap::new(),
+            unit_dependencies,
             files: None,
-            package_cache: HashMap::new(),
             rmeta_required: HashSet::new(),
             pipelining,
         })
@@ -305,7 +303,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.primary_packages
             .extend(units.iter().map(|u| u.pkg.package_id()));
 
-        build_unit_dependencies(self, units)?;
+        self.record_units_requiring_metadata();
+
         let files = CompilationFiles::new(
             units,
             host_layout,
@@ -367,28 +366,19 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         self.primary_packages.contains(&unit.pkg.package_id())
     }
 
-    /// Gets a package for the given package ID.
-    pub fn get_package(&self, id: PackageId) -> CargoResult<&'a Package> {
-        self.package_cache
-            .get(&id)
-            .cloned()
-            .ok_or_else(|| failure::format_err!("failed to find {}", id))
-    }
-
     /// Returns the list of filenames read by cargo to generate the `BuildContext`
     /// (all `Cargo.toml`, etc.).
     pub fn build_plan_inputs(&self) -> CargoResult<Vec<PathBuf>> {
-        let mut inputs = Vec::new();
+        let mut inputs = BTreeSet::new();
         // Note that we're using the `package_cache`, which should have been
         // populated by `build_unit_dependencies`, and only those packages are
         // considered as all the inputs.
         //
         // (Notably, we skip dev-deps here if they aren't present.)
-        for pkg in self.package_cache.values() {
-            inputs.push(pkg.manifest_path().to_path_buf());
+        for unit in self.unit_dependencies.keys() {
+            inputs.insert(unit.pkg.manifest_path().to_path_buf());
         }
-        inputs.sort();
-        Ok(inputs)
+        Ok(inputs.into_iter().collect())
     }
 
     fn check_collistions(&self) -> CargoResult<()> {
@@ -488,6 +478,20 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
         Ok(())
     }
 
+    /// Records the list of units which are required to emit metadata.
+    ///
+    /// Units which depend only on the metadata of others requires the others to
+    /// actually produce metadata, so we'll record that here.
+    fn record_units_requiring_metadata(&mut self) {
+        for (key, deps) in self.unit_dependencies.iter() {
+            for dep in deps {
+                if self.only_requires_rmeta(key, dep) {
+                    self.rmeta_required.insert(*dep);
+                }
+            }
+        }
+    }
+
     /// Returns whether when `parent` depends on `dep` if it only requires the
     /// metadata file from `dep`.
     pub fn only_requires_rmeta(&self, parent: &Unit<'a>, dep: &Unit<'a>) -> bool {

src/cargo/core/compiler/mod.rs

@@ -10,6 +10,7 @@ mod job_queue;
 mod layout;
 mod output_depinfo;
 mod unit;
+mod unit_dependencies;
 
 use std::env;
 use std::ffi::{OsStr, OsString};
@@ -35,6 +36,7 @@ use self::job::{Job, Work};
 use self::job_queue::{JobQueue, JobState};
 pub use self::layout::is_bad_artifact_name;
 use self::output_depinfo::output_depinfo;
+pub use self::unit_dependencies::build_unit_dependencies;
 pub use crate::core::compiler::unit::{Unit, UnitInterner};
 use crate::core::manifest::TargetSourcePath;
 use crate::core::profiles::{Lto, PanicStrategy, Profile};

src/cargo/core/compiler/context/unit_dependencies.rs renamed to src/cargo/core/compiler/unit_dependencies.rs

@@ -16,7 +16,7 @@
 //! graph of `Unit`s, which capture these properties.
 
 use crate::core::compiler::Unit;
-use crate::core::compiler::{BuildContext, CompileMode, Context, Kind};
+use crate::core::compiler::{BuildContext, CompileMode, Kind};
 use crate::core::dependency::Kind as DepKind;
 use crate::core::package::Downloads;
 use crate::core::profiles::UnitFor;
@@ -25,25 +25,24 @@ use crate::CargoResult;
 use log::trace;
 use std::collections::{HashMap, HashSet};
 
-struct State<'a, 'cfg, 'tmp> {
-    cx: &'tmp mut Context<'a, 'cfg>,
+struct State<'a, 'cfg> {
+    bcx: &'a BuildContext<'a, 'cfg>,
     waiting_on_download: HashSet<PackageId>,
     downloads: Downloads<'a, 'cfg>,
+    unit_dependencies: HashMap<Unit<'a>, Vec<Unit<'a>>>,
+    package_cache: HashMap<PackageId, &'a Package>,
 }
 
 pub fn build_unit_dependencies<'a, 'cfg>(
-    cx: &mut Context<'a, 'cfg>,
+    bcx: &'a BuildContext<'a, 'cfg>,
     roots: &[Unit<'a>],
-) -> CargoResult<()> {
-    assert!(
-        cx.unit_dependencies.is_empty(),
-        "can only build unit deps once"
-    );
-
+) -> CargoResult<HashMap<Unit<'a>, Vec<Unit<'a>>>> {
     let mut state = State {
-        downloads: cx.bcx.packages.enable_download()?,
-        cx,
+        bcx,
+        downloads: bcx.packages.enable_download()?,
         waiting_on_download: HashSet::new(),
+        unit_dependencies: HashMap::new(),
+        package_cache: HashMap::new(),
     };
 
     loop {
@@ -56,7 +55,7 @@ pub fn build_unit_dependencies<'a, 'cfg>(
             // cleared, and avoid building the lib thrice (once with `panic`, once
             // without, once for `--test`). In particular, the lib included for
             // Doc tests and examples are `Build` mode here.
-            let unit_for = if unit.mode.is_any_test() || state.cx.bcx.build_config.test() {
+            let unit_for = if unit.mode.is_any_test() || state.bcx.build_config.test() {
                 UnitFor::new_test()
             } else if unit.target.is_custom_build() {
                 // This normally doesn't happen, except `clean` aggressively
@@ -73,32 +72,30 @@ pub fn build_unit_dependencies<'a, 'cfg>(
 
         if !state.waiting_on_download.is_empty() {
             state.finish_some_downloads()?;
-            state.cx.unit_dependencies.clear();
+            state.unit_dependencies.clear();
         } else {
             break;
         }
     }
 
     connect_run_custom_build_deps(&mut state);
 
-    trace!("ALL UNIT DEPENDENCIES {:#?}", state.cx.unit_dependencies);
-
-    record_units_requiring_metadata(state.cx);
+    trace!("ALL UNIT DEPENDENCIES {:#?}", state.unit_dependencies);
 
     // Dependencies are used in tons of places throughout the backend, many of
     // which affect the determinism of the build itself. As a result be sure
     // that dependency lists are always sorted to ensure we've always got a
     // deterministic output.
-    for list in state.cx.unit_dependencies.values_mut() {
+    for list in state.unit_dependencies.values_mut() {
         list.sort();
     }
 
-    Ok(())
+    Ok(state.unit_dependencies)
 }
 
-fn deps_of<'a, 'cfg, 'tmp>(
+fn deps_of<'a, 'cfg>(
     unit: &Unit<'a>,
-    state: &mut State<'a, 'cfg, 'tmp>,
+    state: &mut State<'a, 'cfg>,
     unit_for: UnitFor,
 ) -> CargoResult<()> {
     // Currently the `unit_dependencies` map does not include `unit_for`. This should
@@ -107,10 +104,10 @@ fn deps_of<'a, 'cfg, 'tmp>(
     // `TestDependency`. `CustomBuild` should also be fine since if the
     // requested unit's settings are the same as `Any`, `CustomBuild` can't
     // affect anything else in the hierarchy.
-    if !state.cx.unit_dependencies.contains_key(unit) {
+    if !state.unit_dependencies.contains_key(unit) {
         let unit_deps = compute_deps(unit, state, unit_for)?;
         let to_insert: Vec<_> = unit_deps.iter().map(|&(unit, _)| unit).collect();
-        state.cx.unit_dependencies.insert(*unit, to_insert);
+        state.unit_dependencies.insert(*unit, to_insert);
         for (unit, unit_for) in unit_deps {
             deps_of(&unit, state, unit_for)?;
         }
@@ -122,19 +119,19 @@ fn deps_of<'a, 'cfg, 'tmp>(
 /// for that package.
 /// This returns a `Vec` of `(Unit, UnitFor)` pairs. The `UnitFor`
 /// is the profile type that should be used for dependencies of the unit.
-fn compute_deps<'a, 'cfg, 'tmp>(
+fn compute_deps<'a, 'cfg>(
     unit: &Unit<'a>,
-    state: &mut State<'a, 'cfg, 'tmp>,
+    state: &mut State<'a, 'cfg>,
     unit_for: UnitFor,
 ) -> CargoResult<Vec<(Unit<'a>, UnitFor)>> {
     if unit.mode.is_run_custom_build() {
-        return compute_deps_custom_build(unit, state.cx.bcx);
+        return compute_deps_custom_build(unit, state.bcx);
     } else if unit.mode.is_doc() {
         // Note: this does not include doc test.
         return compute_deps_doc(unit, state);
     }
 
-    let bcx = state.cx.bcx;
+    let bcx = state.bcx;
     let id = unit.pkg.package_id();
     let deps = bcx.resolve.deps(id).filter(|&(_id, deps)| {
         assert!(!deps.is_empty());
@@ -289,11 +286,11 @@ fn compute_deps_custom_build<'a, 'cfg>(
 }
 
 /// Returns the dependencies necessary to document a package.
-fn compute_deps_doc<'a, 'cfg, 'tmp>(
+fn compute_deps_doc<'a, 'cfg>(
     unit: &Unit<'a>,
-    state: &mut State<'a, 'cfg, 'tmp>,
+    state: &mut State<'a, 'cfg>,
 ) -> CargoResult<Vec<(Unit<'a>, UnitFor)>> {
-    let bcx = state.cx.bcx;
+    let bcx = state.bcx;
     let deps = bcx
         .resolve
         .deps(unit.pkg.package_id())
@@ -438,7 +435,7 @@ fn new_unit<'a>(
 ///
 /// Here we take the entire `deps` map and add more dependencies from execution
 /// of one build script to execution of another build script.
-fn connect_run_custom_build_deps(state: &mut State<'_, '_, '_>) {
+fn connect_run_custom_build_deps(state: &mut State<'_, '_>) {
     let mut new_deps = Vec::new();
 
     {
@@ -448,7 +445,7 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_, '_>) {
         // have the build script as the key and the library would be in the
         // value's set.
         let mut reverse_deps = HashMap::new();
-        for (unit, deps) in state.cx.unit_dependencies.iter() {
+        for (unit, deps) in state.unit_dependencies.iter() {
             for dep in deps {
                 if dep.mode == CompileMode::RunCustomBuild {
                     reverse_deps
@@ -469,7 +466,6 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_, '_>) {
         // `dep_build_script` to manufacture an appropriate build script unit to
         // depend on.
         for unit in state
-            .cx
             .unit_dependencies
             .keys()
             .filter(|k| k.mode == CompileMode::RunCustomBuild)
@@ -481,13 +477,13 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_, '_>) {
 
             let to_add = reverse_deps
                 .iter()
-                .flat_map(|reverse_dep| state.cx.unit_dependencies[reverse_dep].iter())
+                .flat_map(|reverse_dep| state.unit_dependencies[reverse_dep].iter())
                 .filter(|other| {
                     other.pkg != unit.pkg
                         && other.target.linkable()
                         && other.pkg.manifest().links().is_some()
                 })
-                .filter_map(|other| dep_build_script(other, state.cx.bcx).map(|p| p.0))
+                .filter_map(|other| dep_build_script(other, state.bcx).map(|p| p.0))
                 .collect::<HashSet<_>>();
 
             if !to_add.is_empty() {
@@ -499,38 +495,23 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_, '_>) {
     // And finally, add in all the missing dependencies!
     for (unit, new_deps) in new_deps {
         state
-            .cx
             .unit_dependencies
             .get_mut(&unit)
             .unwrap()
             .extend(new_deps);
     }
 }
 
-/// Records the list of units which are required to emit metadata.
-///
-/// Units which depend only on the metadata of others requires the others to
-/// actually produce metadata, so we'll record that here.
-fn record_units_requiring_metadata(cx: &mut Context<'_, '_>) {
-    for (key, deps) in cx.unit_dependencies.iter() {
-        for dep in deps {
-            if cx.only_requires_rmeta(key, dep) {
-                cx.rmeta_required.insert(*dep);
-            }
-        }
-    }
-}
-
-impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> {
+impl<'a, 'cfg> State<'a, 'cfg> {
     fn get(&mut self, id: PackageId) -> CargoResult<Option<&'a Package>> {
-        if let Some(pkg) = self.cx.package_cache.get(&id) {
+        if let Some(pkg) = self.package_cache.get(&id) {
             return Ok(Some(pkg));
         }
         if !self.waiting_on_download.insert(id) {
             return Ok(None);
         }
         if let Some(pkg) = self.downloads.start(id)? {
-            self.cx.package_cache.insert(id, pkg);
+            self.package_cache.insert(id, pkg);
             self.waiting_on_download.remove(&id);
             return Ok(Some(pkg));
         }
@@ -550,7 +531,7 @@ impl<'a, 'cfg, 'tmp> State<'a, 'cfg, 'tmp> {
         loop {
             let pkg = self.downloads.wait()?;
             self.waiting_on_download.remove(&pkg.package_id());
-            self.cx.package_cache.insert(pkg.package_id(), pkg);
+            self.package_cache.insert(pkg.package_id(), pkg);
 
             // Arbitrarily choose that 5 or more packages concurrently download
             // is a good enough number to "fill the network pipe". If we have

src/cargo/ops/cargo_clean.rs

@@ -2,6 +2,7 @@ use std::collections::HashMap;
 use std::fs;
 use std::path::Path;
 
+use crate::core::compiler::build_unit_dependencies;
 use crate::core::compiler::UnitInterner;
 use crate::core::compiler::{BuildConfig, BuildContext, CompileMode, Context, Kind};
 use crate::core::profiles::UnitFor;
@@ -104,7 +105,8 @@ pub fn clean(ws: &Workspace<'_>, opts: &CleanOptions<'_>) -> CargoResult<()> {
         }
     }
 
-    let mut cx = Context::new(config, &bcx)?;
+    let unit_dependencies = build_unit_dependencies(&bcx, &units)?;
+    let mut cx = Context::new(config, &bcx, unit_dependencies)?;
     cx.prepare_units(None, &units)?;
 
     for unit in units.iter() {