Auto merge of #8028 - ehuss:new-proc-macro-decouple, r=alexcrichton

Re-implement proc-macro feature decoupling.

This is essentially a rewrite of #8003. Instead of adding proc-macro to the index, it uses a strategy of downloading all packages before doing feature resolution. Then the package can be inspected for the proc-macro field.

This is a fairly major change. A brief overview:
- `PackageSet` now has a `download_accessible` method which tries to download a minimal set of every accessible package. This isn't very smart right now, and errs on downloading too much. In most cases it should be the same (or nearly the same) as before. It downloads extra in the following cases:
    - The check for `[target]` dependencies checks both host and target for every dependency. I could tighten that up a little so build dependencies only check for the host, but it would add some complexity and I wanted to get feedback first.
    - Optional dependencies disabled by the new feature resolver will get downloaded.
- Removed the loop in computing unit dependencies where downloading used to reside.
- When downloading starts, it should now show a more accurate count of how many crates are to be downloaded. Previously the count would fluctuate while the graph is being built.

Author: bors

Cargo.toml

@@ -22,7 +22,7 @@ path = "src/cargo/lib.rs"
 atty = "0.2"
 bytesize = "1.0"
 cargo-platform = { path = "crates/cargo-platform", version = "0.1.1" }
-crates-io = { path = "crates/crates-io", version = "0.32" }
+crates-io = { path = "crates/crates-io", version = "0.31" }
 crossbeam-utils = "0.7"
 crypto-hash = "0.3.1"
 curl = { version = "0.4.23", features = ["http2"] }

crates/cargo-test-support/src/registry.rs

@@ -420,7 +420,6 @@ impl Package {
             "cksum": cksum,
             "features": self.features,
             "yanked": self.yanked,
-            "pm": self.proc_macro,
         })
         .to_string();
 

crates/crates-io/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "crates-io"
-version = "0.32.0"
+version = "0.31.0"
 edition = "2018"
 authors = ["Alex Crichton <[email protected]>"]
 license = "MIT OR Apache-2.0"

crates/crates-io/lib.rs

@@ -55,7 +55,6 @@ pub struct NewCrate {
     pub repository: Option<String>,
     pub badges: BTreeMap<String, BTreeMap<String, String>>,
     pub links: Option<String>,
-    pub proc_macro: bool,
 }
 
 #[derive(Serialize)]

crates/resolver-tests/src/lib.rs

@@ -176,7 +176,6 @@ pub fn resolve_with_config_raw(
         &BTreeMap::<String, Vec<String>>::new(),
         None::<&String>,
         false,
-        false,
     )
     .unwrap();
     let opts = ResolveOpts::everything();
@@ -578,7 +577,6 @@ pub fn pkg_dep<T: ToPkgId>(name: T, dep: Vec<Dependency>) -> Summary {
         &BTreeMap::<String, Vec<String>>::new(),
         link,
         false,
-        false,
     )
     .unwrap()
 }
@@ -607,7 +605,6 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary {
         &BTreeMap::<String, Vec<String>>::new(),
         link,
         false,
-        false,
     )
     .unwrap()
 }
@@ -622,7 +619,6 @@ pub fn remove_dep(sum: &Summary, ind: usize) -> Summary {
         &BTreeMap::<String, Vec<String>>::new(),
         sum.links().map(|a| a.as_str()),
         sum.namespaced_features(),
-        sum.proc_macro(),
     )
     .unwrap()
 }

src/cargo/core/compiler/unit_dependencies.rs

@@ -19,7 +19,6 @@ use crate::core::compiler::unit_graph::{UnitDep, UnitGraph};
 use crate::core::compiler::Unit;
 use crate::core::compiler::{BuildContext, CompileKind, CompileMode};
 use crate::core::dependency::DepKind;
-use crate::core::package::Downloads;
 use crate::core::profiles::{Profile, UnitFor};
 use crate::core::resolver::features::{FeaturesFor, ResolvedFeatures};
 use crate::core::resolver::Resolve;
@@ -31,10 +30,7 @@ use std::collections::{HashMap, HashSet};
 /// Collection of stuff used while creating the `UnitGraph`.
 struct State<'a, 'cfg> {
     bcx: &'a BuildContext<'a, 'cfg>,
-    waiting_on_download: HashSet<PackageId>,
-    downloads: Downloads<'a, 'cfg>,
     unit_dependencies: UnitGraph<'a>,
-    package_cache: HashMap<PackageId, &'a Package>,
     usr_resolve: &'a Resolve,
     usr_features: &'a ResolvedFeatures,
     std_resolve: Option<&'a Resolve>,
@@ -58,10 +54,7 @@ pub fn build_unit_dependencies<'a, 'cfg>(
     };
     let mut state = State {
         bcx,
-        downloads: bcx.packages.enable_download()?,
-        waiting_on_download: HashSet::new(),
         unit_dependencies: HashMap::new(),
-        package_cache: HashMap::new(),
         usr_resolve: resolve,
         usr_features: features,
         std_resolve,
@@ -141,44 +134,32 @@ fn attach_std_deps<'a, 'cfg>(
 /// Compute all the dependencies of the given root units.
 /// The result is stored in state.unit_dependencies.
 fn deps_of_roots<'a, 'cfg>(roots: &[Unit<'a>], mut state: &mut State<'a, 'cfg>) -> CargoResult<()> {
-    // Loop because we are downloading while building the dependency graph.
-    // The partially-built unit graph is discarded through each pass of the
-    // loop because it is incomplete because not all required Packages have
-    // been downloaded.
-    loop {
-        for unit in roots.iter() {
-            state.get(unit.pkg.package_id())?;
-
-            // Dependencies of tests/benches should not have `panic` set.
-            // We check the global test mode to see if we are running in `cargo
-            // test` in which case we ensure all dependencies have `panic`
-            // 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.bcx.build_config.test() {
-                UnitFor::new_test(state.bcx.config)
-            } else if unit.target.is_custom_build() {
-                // This normally doesn't happen, except `clean` aggressively
-                // generates all units.
-                UnitFor::new_host(false)
-            } else if unit.target.proc_macro() {
-                UnitFor::new_host(true)
-            } else if unit.target.for_host() {
-                // Plugin should never have panic set.
-                UnitFor::new_compiler()
-            } else {
-                UnitFor::new_normal()
-            };
-            deps_of(unit, &mut state, unit_for)?;
-        }
-
-        if !state.waiting_on_download.is_empty() {
-            state.finish_some_downloads()?;
-            state.unit_dependencies.clear();
+    for unit in roots.iter() {
+        state.get(unit.pkg.package_id());
+
+        // Dependencies of tests/benches should not have `panic` set.
+        // We check the global test mode to see if we are running in `cargo
+        // test` in which case we ensure all dependencies have `panic`
+        // 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.bcx.build_config.test() {
+            UnitFor::new_test(state.bcx.config)
+        } else if unit.target.is_custom_build() {
+            // This normally doesn't happen, except `clean` aggressively
+            // generates all units.
+            UnitFor::new_host(false)
+        } else if unit.target.proc_macro() {
+            UnitFor::new_host(true)
+        } else if unit.target.for_host() {
+            // Plugin should never have panic set.
+            UnitFor::new_compiler()
         } else {
-            break;
-        }
+            UnitFor::new_normal()
+        };
+        deps_of(unit, &mut state, unit_for)?;
     }
+
     Ok(())
 }
 
@@ -269,10 +250,7 @@ fn compute_deps<'a, 'cfg>(
 
     let mut ret = Vec::new();
     for (id, _) in filtered_deps {
-        let pkg = match state.get(id)? {
-            Some(pkg) => pkg,
-            None => continue,
-        };
+        let pkg = state.get(id);
         let lib = match pkg.targets().iter().find(|t| t.is_lib()) {
             Some(t) => t,
             None => continue,
@@ -419,10 +397,7 @@ fn compute_deps_doc<'a, 'cfg>(
     // the documentation of the library being built.
     let mut ret = Vec::new();
     for (id, _deps) in deps {
-        let dep = match state.get(id)? {
-            Some(dep) => dep,
-            None => continue,
-        };
+        let dep = state.get(id);
         let lib = match dep.targets().iter().find(|t| t.is_lib()) {
             Some(lib) => lib,
             None => continue,
@@ -730,44 +705,10 @@ impl<'a, 'cfg> State<'a, 'cfg> {
         features.activated_features(pkg_id, features_for)
     }
 
-    fn get(&mut self, id: PackageId) -> CargoResult<Option<&'a Package>> {
-        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.package_cache.insert(id, pkg);
-            self.waiting_on_download.remove(&id);
-            return Ok(Some(pkg));
-        }
-        Ok(None)
-    }
-
-    /// Completes at least one downloading, maybe waiting for more to complete.
-    ///
-    /// This function will block the current thread waiting for at least one
-    /// crate to finish downloading. The function may continue to download more
-    /// crates if it looks like there's a long enough queue of crates to keep
-    /// downloading. When only a handful of packages remain this function
-    /// returns, and it's hoped that by returning we'll be able to push more
-    /// packages to download into the queue.
-    fn finish_some_downloads(&mut self) -> CargoResult<()> {
-        assert!(self.downloads.remaining() > 0);
-        loop {
-            let pkg = self.downloads.wait()?;
-            self.waiting_on_download.remove(&pkg.package_id());
-            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
-            // less than this let's recompute the whole unit dependency graph
-            // again and try to find some more packages to download.
-            if self.downloads.remaining() < 5 {
-                break;
-            }
-        }
-        Ok(())
+    fn get(&self, id: PackageId) -> &'a Package {
+        self.bcx
+            .packages
+            .get_one(id)
+            .unwrap_or_else(|_| panic!("expected {} to be downloaded", id))
     }
 }

src/cargo/core/package.rs

@@ -1,6 +1,6 @@
 use std::cell::{Cell, Ref, RefCell, RefMut};
 use std::cmp::Ordering;
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap, HashSet};
 use std::fmt;
 use std::hash;
 use std::mem;
@@ -17,7 +17,10 @@ use semver::Version;
 use serde::ser;
 use serde::Serialize;
 
+use crate::core::compiler::{CompileKind, RustcTargetData};
+use crate::core::dependency::DepKind;
 use crate::core::interning::InternedString;
+use crate::core::resolver::{HasDevUnits, Resolve};
 use crate::core::source::MaybePackage;
 use crate::core::{Dependency, Manifest, PackageId, SourceId, Target};
 use crate::core::{FeatureMap, SourceMap, Summary};
@@ -189,6 +192,10 @@ impl Package {
     pub fn publish(&self) -> &Option<Vec<String>> {
         self.manifest.publish()
     }
+    /// Returns `true` if this package is a proc-macro.
+    pub fn proc_macro(&self) -> bool {
+        self.targets().iter().any(|target| target.proc_macro())
+    }
 
     /// Returns `true` if the package uses a custom build script for any target.
     pub fn has_custom_build(&self) -> bool {
@@ -432,6 +439,9 @@ impl<'cfg> PackageSet<'cfg> {
     }
 
     pub fn get_one(&self, id: PackageId) -> CargoResult<&Package> {
+        if let Some(pkg) = self.packages.get(&id).and_then(|slot| slot.borrow()) {
+            return Ok(pkg);
+        }
         Ok(self.get_many(Some(id))?.remove(0))
     }
 
@@ -448,6 +458,75 @@ impl<'cfg> PackageSet<'cfg> {
         Ok(pkgs)
     }
 
+    /// Downloads any packages accessible from the give root ids.
+    pub fn download_accessible(
+        &self,
+        resolve: &Resolve,
+        root_ids: &[PackageId],
+        has_dev_units: HasDevUnits,
+        requested_kind: CompileKind,
+        target_data: &RustcTargetData,
+    ) -> CargoResult<()> {
+        fn collect_used_deps(
+            used: &mut BTreeSet<PackageId>,
+            resolve: &Resolve,
+            pkg_id: PackageId,
+            has_dev_units: HasDevUnits,
+            requested_kind: CompileKind,
+            target_data: &RustcTargetData,
+        ) -> CargoResult<()> {
+            if !used.insert(pkg_id) {
+                return Ok(());
+            }
+            let filtered_deps = resolve.deps(pkg_id).filter(|&(_id, deps)| {
+                deps.iter().any(|dep| {
+                    if dep.kind() == DepKind::Development && has_dev_units == HasDevUnits::No {
+                        return false;
+                    }
+                    // This is overly broad, since not all target-specific
+                    // dependencies are used both for target and host. To tighten this
+                    // up, this function would need to track "for_host" similar to how
+                    // unit dependencies handles it.
+                    if !target_data.dep_platform_activated(dep, requested_kind)
+                        && !target_data.dep_platform_activated(dep, CompileKind::Host)
+                    {
+                        return false;
+                    }
+                    true
+                })
+            });
+            for (dep_id, _deps) in filtered_deps {
+                collect_used_deps(
+                    used,
+                    resolve,
+                    dep_id,
+                    has_dev_units,
+                    requested_kind,
+                    target_data,
+                )?;
+            }
+            Ok(())
+        }
+
+        // This is sorted by PackageId to get consistent behavior and error
+        // messages for Cargo's testsuite. Perhaps there is a better ordering
+        // that optimizes download time?
+        let mut to_download = BTreeSet::new();
+
+        for id in root_ids {
+            collect_used_deps(
+                &mut to_download,
+                resolve,
+                *id,
+                has_dev_units,
+                requested_kind,
+                target_data,
+            )?;
+        }
+        self.get_many(to_download.into_iter())?;
+        Ok(())
+    }
+
     pub fn sources(&self) -> Ref<'_, SourceMap<'cfg>> {
         self.sources.borrow()
     }