WIP: try making cargo tree's Graph from a UnitGraph

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

@@ -414,6 +414,7 @@ impl Project {
         let mut execs = self.process(&cargo);
         if let Some(ref mut p) = execs.process_builder {
             p.env("CARGO", cargo);
+            p.env("CARGO_TREE_FROM_UNIT_GRAPH", "1");
             p.arg_line(cmd);
         }
         execs

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

@@ -378,6 +378,7 @@ pub struct Package {
     name: String,
     vers: String,
     deps: Vec<Dependency>,
+    needs_build_script: bool,
     files: Vec<PackageFile>,
     yanked: bool,
     features: FeatureMap,
@@ -854,6 +855,7 @@ impl Package {
             name: name.to_string(),
             vers: vers.to_string(),
             deps: Vec::new(),
+            needs_build_script: false,
             files: Vec::new(),
             yanked: false,
             features: BTreeMap::new(),
@@ -890,6 +892,15 @@ impl Package {
         self
     }
 
+    /// Add a build.rs so that the UnitGraph will not ignore our [build-dependenies] section.
+    ///
+    /// Also adds a src/lib.rs, because make_archive() will no longer do it for us.
+    /// FIXME: there is probably a better way to do this.
+    pub fn build_script(&mut self) -> &mut Package {
+        self.needs_build_script = true;
+        self
+    }
+
     /// Adds a file to the package.
     pub fn file(&mut self, name: &str, contents: &str) -> &mut Package {
         self.file_with_mode(name, DEFAULT_MODE, contents)
@@ -979,6 +990,9 @@ impl Package {
     /// foo = {version = "1.0"}
     /// ```
     pub fn build_dep(&mut self, name: &str, vers: &str) -> &mut Package {
+        // FIXME: these will be removed from UnitGraph if there is no matching is_custom_build unit.
+        // Either implicitly add one here, or add a `.add_build_script()` and make sure it is called
+        // everywhere?.
         self.add_dep(Dependency::new(name, vers).build())
     }
 
@@ -1142,6 +1156,14 @@ impl Package {
                 }
             }
         }
+        if self.needs_build_script && !self.files.iter().any(|f| f.path == "build.rs") {
+            self.append(
+                &mut a,
+                "src/lib.rs",
+                DEFAULT_MODE,
+                &EntryData::Regular("fn main {}".into()),
+            )
+        }
     }
 
     fn append_manifest<W: Write>(&self, ar: &mut Builder<W>) {
@@ -1164,6 +1186,10 @@ impl Package {
             self.name, self.vers
         ));
 
+        if self.needs_build_script {
+            manifest.push_str("build = \"build.rs\"");
+        }
+
         if let Some(version) = &self.rust_version {
             manifest.push_str(&format!("rust-version = \"{}\"", version));
         }

src/cargo/core/compiler/unit_dependencies.rs

@@ -1080,6 +1080,12 @@ impl<'a, 'cfg> State<'a, 'cfg> {
                         // dependencies, otherwise we want everything *other than* build
                         // dependencies.
                         if unit.target.is_custom_build() != dep.is_build() {
+                            log::trace!(
+                                "dropping dep of {:?}: {:?}: buildiness mismatch",
+                                pkg_id.name(),
+                                id.name()
+                            );
+
                             return false;
                         }
 

src/cargo/core/resolver/features.rs

@@ -96,7 +96,7 @@ pub enum HasDevUnits {
 }
 
 /// Flag to indicate that target-specific filtering should be disabled.
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 pub enum ForceAllTargets {
     Yes,
     No,

src/cargo/ops/cargo_compile/mod.rs

@@ -41,7 +41,7 @@ use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context};
 use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, RustcTargetData, Unit};
 use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner};
 use crate::core::profiles::{Profiles, UnitFor};
-use crate::core::resolver::features::{self, CliFeatures, FeaturesFor};
+use crate::core::resolver::features::{self, CliFeatures, FeaturesFor, ForceAllTargets};
 use crate::core::resolver::{HasDevUnits, Resolve};
 use crate::core::{FeatureValue, Package, PackageSet, Shell, Summary, Target};
 use crate::core::{PackageId, SourceId, TargetKind, Workspace};
@@ -79,6 +79,8 @@ pub struct CompileOptions {
     /// Filter to apply to the root package to select which targets will be
     /// built.
     pub filter: CompileFilter,
+    /// HACK: used by `cargo tree` to support `--target=all`.
+    pub force_all_targets: ForceAllTargets,
     /// Extra arguments to be passed to rustdoc (single target only)
     pub target_rustdoc_args: Option<Vec<String>>,
     /// The specified target will be compiled with all the available arguments,
@@ -105,6 +107,7 @@ impl CompileOptions {
             filter: CompileFilter::Default {
                 required_features_filterable: false,
             },
+            force_all_targets: ForceAllTargets::No,
             target_rustdoc_args: None,
             target_rustc_args: None,
             target_rustc_crate_types: None,
@@ -200,6 +203,7 @@ pub fn create_bcx<'a, 'cfg>(
         ref spec,
         ref cli_features,
         ref filter,
+        force_all_targets,
         ref target_rustdoc_args,
         ref target_rustc_args,
         ref target_rustc_crate_types,
@@ -255,7 +259,7 @@ pub fn create_bcx<'a, 'cfg>(
         cli_features,
         &resolve_specs,
         has_dev_units,
-        crate::core::resolver::features::ForceAllTargets::No,
+        force_all_targets,
     )?;
     let WorkspaceResolve {
         mut pkg_set,

src/cargo/ops/cargo_compile/packages.rs

@@ -13,7 +13,7 @@ use anyhow::{bail, Context as _};
 ///
 /// Generally, it represents the combination of all `-p` flag. When working within
 /// a workspace, `--exclude` and `--workspace` flags also contribute to it.
-#[derive(PartialEq, Eq, Debug)]
+#[derive(PartialEq, Eq, Debug, Clone)]
 pub enum Packages {
     /// Pacakges selected by default. Ususally means no flag provided.
     Default,

src/cargo/ops/cargo_package.rs

@@ -8,7 +8,7 @@ use std::sync::Arc;
 use std::task::Poll;
 
 use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor};
-use crate::core::resolver::CliFeatures;
+use crate::core::resolver::{CliFeatures, ForceAllTargets};
 use crate::core::{Feature, Shell, Verbosity, Workspace};
 use crate::core::{Package, PackageId, PackageSet, Resolve, SourceId};
 use crate::sources::PathSource;
@@ -851,6 +851,7 @@ fn run_verify(
             filter: ops::CompileFilter::Default {
                 required_features_filterable: true,
             },
+            force_all_targets: ForceAllTargets::No,
             target_rustdoc_args: None,
             target_rustc_args: rustc_args,
             target_rustc_crate_types: None,

src/cargo/ops/tree/graph.rs

@@ -1,7 +1,9 @@
 //! Code for building the graph used by `cargo tree`.
 
+use itertools::Itertools;
+
 use super::TreeOptions;
-use crate::core::compiler::{CompileKind, RustcTargetData};
+use crate::core::compiler::{BuildContext, CompileKind, RustcTargetData, Unit};
 use crate::core::dependency::DepKind;
 use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures};
 use crate::core::resolver::Resolve;
@@ -26,6 +28,20 @@ pub enum Node {
     },
 }
 
+impl Node {
+    /// Make a Node representing the Node::Package of a Unit.
+    ///
+    /// Note: There is a many:1 relationship between Unit and Package,
+    /// because some units are build.rs builds or build.rs invocations.
+    fn package_for_unit(unit: &Unit) -> Node {
+        Node::Package {
+            package_id: unit.pkg.package_id(),
+            features: unit.features.clone(),
+            kind: unit.kind,
+        }
+    }
+}
+
 /// The kind of edge, for separating dependencies into different sections.
 #[derive(Debug, Copy, Hash, Eq, Clone, PartialEq)]
 pub enum EdgeKind {
@@ -49,7 +65,7 @@ impl Edges {
         Edges(HashMap::new())
     }
 
-    /// Adds an edge pointing to the given node.
+    /// Adds an edge pointing to the given node. This is idempotent.
     fn add_edge(&mut self, kind: EdgeKind, index: usize) {
         let indexes = self.0.entry(kind).or_default();
         if !indexes.contains(&index) {
@@ -93,6 +109,15 @@ impl<'a> Graph<'a> {
         }
     }
 
+    /// Adds a new node to the graph if it doesn't exist, returning its index.
+    fn add_node_idempotently(&mut self, node: Node) -> usize {
+        if let Some(index) = self.index.get(&node) {
+            *index
+        } else {
+            self.add_node(node)
+        }
+    }
+
     /// Adds a new node to the graph, returning its new index.
     fn add_node(&mut self, node: Node) -> usize {
         let from_index = self.nodes.len();
@@ -145,7 +170,9 @@ impl<'a> Graph<'a> {
     }
 
     pub fn package_for_id(&self, id: PackageId) -> &Package {
-        self.package_map[&id]
+        self.package_map
+            .get(&id)
+            .unwrap_or_else(|| panic!("could not find {id:#?} in {:#?}", self.package_map))
     }
 
     fn package_id_for_index(&self, index: usize) -> PackageId {
@@ -266,6 +293,149 @@ impl<'a> Graph<'a> {
     }
 }
 
+/// Builds the graph by iterating over the UnitDeps of a BuildContext.
+///
+/// This is useful for finding bugs in the implementation of `build()`, below.
+pub fn from_bcx<'a, 'cfg>(
+    bcx: BuildContext<'a, 'cfg>,
+    resolve: &Resolve,
+    // FIXME: it feels like it would be easy for specs and cli_features to get out-of-sync with
+    // what bcx has been configured with. Either make that structurally impossible or add an assert.
+    specs: &[PackageIdSpec],
+    cli_features: &CliFeatures,
+    package_map: HashMap<PackageId, &'a Package>,
+    opts: &TreeOptions,
+) -> CargoResult<Graph<'a>> {
+    let mut graph = Graph::new(package_map);
+
+    // First pass: add all of the nodes for Packages
+    for unit in bcx.unit_graph.keys().sorted() {
+        let node = Node::package_for_unit(unit);
+        // There may be multiple units for the same package if build-scripts are involved.
+        graph.add_node_idempotently(node);
+
+        // FIXME: I quite like the idea of adding all of the nodes in the first pass, but adding
+        // the full set of features doesn't seem to be possible here. Maybe it's better to think of
+        // features as more like a kind of Edge, and do it in the second loop.
+        // if opts.graph_features {
+        //     for name in unit.features.iter().copied() {
+        //         let node = Node::Feature { node_index, name };
+        //         graph.add_node_idempotently(node);
+        //     }
+        // }
+    }
+
+    // second pass: add all of the edges (and `Node::Feature`s if that's what's asked for)
+    for (unit, deps) in bcx.unit_graph.iter() {
+        let node = Node::package_for_unit(unit);
+        let from_index = *graph.index.get(&node).unwrap();
+
+        for dep in deps {
+            if dep.unit.pkg.package_id() == unit.pkg.package_id() {
+                // Probably a build script that's part of the same package. Skip it.
+                continue;
+            }
+            let dep_node = Node::package_for_unit(&dep.unit);
+            let dep_index = *graph.index.get(&dep_node).unwrap();
+
+            // FIXME: This is really ugly. It's also quadratic in `deps.len()`, but `deps` is only
+            // the direct dependencies of `unit`, so the ugliness is more important.
+            // I think I want to `zip(sorted(deps), sorted(resolve.deps(unit)))` and then assert
+            // that the ids line up, with nothing left over.
+            let mut found = false;
+            let mut added = false;
+            for (_, dep_set) in resolve
+                .deps(unit.pkg.package_id())
+                .filter(|(dep_id, _dep_set)| dep_id == &dep.unit.pkg.package_id())
+            {
+                found = true;
+                assert!(
+                    !dep_set.is_empty(),
+                    "resolver should be able to tell us why {unit:?} depends on {dep:?}"
+                );
+
+                // FIXME: think of better names for dep and link
+                // (most code needs to have `dep` renamed to `link` when copy-pasting)
+                for link in dep_set {
+                    if opts.graph_features {
+                        if link.uses_default_features() {
+                            add_feature(
+                                &mut graph,
+                                InternedString::new("default"),
+                                Some(from_index),
+                                dep_index,
+                                EdgeKind::Dep(link.kind()),
+                            );
+                            added = true;
+                        }
+                        for feature in link.features() {
+                            // FIXME: is add_feature() idempotent?
+                            add_feature(
+                                &mut graph,
+                                *feature,
+                                Some(from_index),
+                                dep_index,
+                                EdgeKind::Dep(link.kind()),
+                            );
+                            added = true;
+                        }
+                        // FIXME: do this in its own pass or something?
+                        graph
+                            .dep_name_map
+                            .entry(from_index)
+                            .or_default()
+                            .entry(link.name_in_toml())
+                            .or_default()
+                            .insert((dep_index, link.is_optional()));
+                    } else {
+                        let kind = EdgeKind::Dep(link.kind());
+                        if opts.edge_kinds.contains(&kind) {
+                            // FIXME: if it's not possible to get from unit to dep with this kind
+                            // of edge then maybe we shouldn't add it? Maybe this would help with
+                            // the tree::host_dep_feature test? Not sure how to determine this though.
+                            graph.edges[from_index].add_edge(kind, dep_index);
+                        }
+                    }
+                }
+            }
+
+            assert!(
+                found,
+                "resolver should have a record of {unit:?} depending on {dep:?}"
+            );
+            if opts.graph_features && !added {
+                // HACK: if dep was added with default-features = false and no other features then
+                // it won't be linked up yet. Fudge a direct link in there so that we can represent
+                // it on the graph.
+                graph.edges[from_index].add_edge(EdgeKind::Dep(DepKind::Normal), dep_index);
+            }
+        }
+    }
+
+    if opts.graph_features {
+        let mut members_with_features = bcx.ws.members_with_features(specs, cli_features)?;
+        members_with_features.sort_unstable_by_key(|e| e.0.package_id());
+        for (member, cli_features) in members_with_features {
+            // This package might be built for both host and target.
+            let member_indexes = graph.indexes_from_ids(&[member.package_id()]);
+            assert!(!member_indexes.is_empty());
+
+            // FIXME: if the package shows up in both host and `target`, it may be possible for the
+            // features to be different (this may not even be possible for workspace members in the
+            // current resolver - I've not checked).
+            //
+            // We might be better off querying the UnitGraph again or something?
+            let fmap = resolve.summary(member.package_id()).features();
+            for member_index in member_indexes.into_iter() {
+                add_cli_features(&mut graph, member_index, &cli_features, fmap);
+            }
+        }
+
+        add_internal_features(&mut graph, resolve)
+    }
+    Ok(graph)
+}
+
 /// Builds the graph.
 pub fn build<'a>(
     ws: &Workspace<'_>,
@@ -519,7 +689,11 @@ fn add_cli_features(
                 dep_feature,
                 weak,
             } => {
-                let dep_connections = match graph.dep_name_map[&package_index].get(&dep_name) {
+                let dep_connections = match graph
+                    .dep_name_map
+                    .get(&package_index)
+                    .and_then(|h| h.get(&dep_name))
+                {
                     // Clone to deal with immutable borrow of `graph`. :(
                     Some(dep_connections) => dep_connections.clone(),
                     None => {