Improve workspace discovery logic

Previously we were assuming that any crate with a path dep lives in a
workspace, and any that doesn't isn't.

This isn't correct logic.
https://doc.rust-lang.org/cargo/reference/workspaces.html describes the
specification. We do filesystem traversal to detect implicit members.
This is a little more expensive, but much more correct.

This helps supporting path dependencies - the current
incorrect logic incorrectly identifies any Cargo.toml file containing a
path dependency as expecting to live in a workspace, which breaks some
use-cases.

Author: illicitonion

crate_universe/Cargo.toml

@@ -1,6 +1,46 @@
 [workspace]
 members = ["tools/cross_installer", "tools/urls_generator"]
-exclude = ["test_data"]
+exclude = [
+    "test_data/metadata/abspath",
+    "test_data/metadata/aliases",
+    "test_data/metadata/build_scripts",
+    "test_data/metadata/common",
+    "test_data/metadata/crate_combined_features",
+    "test_data/metadata/crate_optional_deps_disabled",
+    "test_data/metadata/crate_optional_deps_disabled_build_dep_enabled",
+    "test_data/metadata/crate_optional_deps_enabled",
+    "test_data/metadata/crate_renamed_optional_deps_disabled",
+    "test_data/metadata/crate_renamed_optional_deps_enabled",
+    "test_data/metadata/crate_types",
+    "test_data/metadata/example_proc_macro_dep",
+    "test_data/metadata/git_repos",
+    "test_data/metadata/has_package_metadata",
+    "test_data/metadata/host_specific_build_deps",
+    "test_data/metadata/multi_cfg_dep",
+    "test_data/metadata/multi_kind_proc_macro_dep",
+    "test_data/metadata/nested_build_dependencies",
+    "test_data/metadata/no_deps",
+    "test_data/metadata/resolver_2_deps",
+    "test_data/metadata/target_cfg_features",
+    "test_data/metadata/target_features",
+    "test_data/metadata/tree_data",
+    "test_data/metadata/workspace",
+    "test_data/metadata/workspace/child",
+    "test_data/metadata/workspace_path",
+    "test_data/metadata/workspace_path/child_a",
+    "test_data/metadata/workspace_path/child_b",
+    "test_data/test_data_passing_crate",
+    "test_data/workspace_examples/non-ws",
+    "test_data/workspace_examples/ws1",
+    "test_data/workspace_examples/ws1/ws1c1",
+    "test_data/workspace_examples/ws1/ws1c1/ws1c1c1",
+    "test_data/workspace_examples/ws1/ws1c2",
+    "test_data/workspace_examples/ws2",
+    "test_data/workspace_examples/ws2/ws2c1",
+    "test_data/workspace_examples/ws2/ws2excluded",
+    "test_data/workspace_examples/ws2/ws2excluded/ws2excluded2",
+    "test_data/workspace_examples/ws2/ws2excluded/ws2included",
+]
 
 [package]
 name = "cargo-bazel"

crate_universe/extensions.bzl

@@ -112,6 +112,8 @@ def _generate_hub_and_spokes(*, module_ctx, cargo_bazel, cfg, annotations, cargo
         ),
     )
 
+    nonhermetic_root_bazel_workspace_dir = module_ctx.path(Label("@@//:MODULE.bazel")).dirname
+
     splicing_output_dir = tag_path.get_child("splicing-output")
     splice_args = [
         "splice",
@@ -121,6 +123,8 @@ def _generate_hub_and_spokes(*, module_ctx, cargo_bazel, cfg, annotations, cargo
         config_file,
         "--splicing-manifest",
         splicing_manifest,
+        "--nonhermetic-root-bazel-workspace-dir",
+        nonhermetic_root_bazel_workspace_dir,
     ]
     if cargo_lockfile:
         splice_args.extend([
@@ -153,7 +157,7 @@ def _generate_hub_and_spokes(*, module_ctx, cargo_bazel, cfg, annotations, cargo
         "--lockfile",
         lockfile_path,
         "--nonhermetic-root-bazel-workspace-dir",
-        module_ctx.path(Label("@@//:MODULE.bazel")).dirname,
+        nonhermetic_root_bazel_workspace_dir,
         "--paths-to-track",
         paths_to_track_file,
         "--warnings-output-path",

crate_universe/private/splicing_utils.bzl

@@ -149,6 +149,8 @@ def splice_workspace_manifest(repository_ctx, generator, cargo_lockfile, splicin
         rustc,
         "--cargo-lockfile",
         cargo_lockfile,
+        "--nonhermetic-root-bazel-workspace-dir",
+        repository_ctx.workspace_root,
     ]
 
     # Optionally set the splicing workspace directory to somewhere within the repository directory

crate_universe/private/srcs.bzl

@@ -28,6 +28,7 @@ CARGO_BAZEL_SRCS = [
     Label("//crate_universe:src/metadata/cargo_tree_rustc_wrapper.sh"),
     Label("//crate_universe:src/metadata/dependency.rs"),
     Label("//crate_universe:src/metadata/metadata_annotation.rs"),
+    Label("//crate_universe:src/metadata/workspace_discoverer.rs"),
     Label("//crate_universe:src/rendering.rs"),
     Label("//crate_universe:src/rendering/template_engine.rs"),
     Label("//crate_universe:src/rendering/templates/module_bzl.j2"),

crate_universe/src/cli/splice.rs

@@ -3,6 +3,7 @@
 use std::path::PathBuf;
 
 use anyhow::Context;
+use camino::Utf8PathBuf;
 use clap::Parser;
 
 use crate::cli::Result;
@@ -31,7 +32,7 @@ pub struct SpliceOptions {
     /// The directory in which to build the workspace. If this argument is not
     /// passed, a temporary directory will be generated.
     #[clap(long)]
-    pub workspace_dir: Option<PathBuf>,
+    pub workspace_dir: Option<Utf8PathBuf>,
 
     /// The location where the results of splicing are written.
     #[clap(long)]
@@ -56,6 +57,9 @@ pub struct SpliceOptions {
     /// The path to a rustc binary for use with Cargo
     #[clap(long, env = "RUSTC")]
     pub rustc: PathBuf,
+
+    #[clap(long)]
+    pub nonhermetic_root_bazel_workspace_dir: PathBuf,
 }
 
 /// Combine a set of disjoint manifests into a single workspace.
@@ -70,7 +74,8 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
         Some(dir) => dir.clone(),
         None => {
             temp_dir = tempfile::tempdir().context("Failed to generate temporary directory")?;
-            temp_dir.as_ref().to_path_buf()
+            Utf8PathBuf::from_path_buf(temp_dir.as_ref().to_path_buf())
+                .unwrap_or_else(|path| panic!("Temporary directory wasn't valid UTF-8: {:?}", path))
         }
     };
 
@@ -81,7 +86,7 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
 
     // Splice together the manifest
     let manifest_path = splicer
-        .splice_workspace(&cargo)
+        .splice_workspace(&opt.nonhermetic_root_bazel_workspace_dir)
         .context("Failed to splice workspace")?;
 
     // Generate a lockfile
@@ -126,7 +131,7 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {
         .with_context(|| {
             format!(
                 "The path {} is expected to have a parent directory",
-                manifest_path.as_path_buf().display()
+                manifest_path.as_path_buf()
             )
         })?
         .join("Cargo.lock");

crate_universe/src/cli/vendor.rs

@@ -130,16 +130,18 @@ pub fn vendor(opt: VendorOptions) -> Result<()> {
         .resolve(&opt.workspace_dir, &output_base);
 
     let temp_dir = tempfile::tempdir().context("Failed to create temporary directory")?;
+    let temp_dir_path = Utf8PathBuf::from_path_buf(temp_dir.as_ref().to_path_buf())
+        .unwrap_or_else(|path| panic!("Temporary directory wasn't valid UTF-8: {:?}", path));
 
     // Generate a splicer for creating a Cargo workspace manifest
-    let splicer = Splicer::new(PathBuf::from(temp_dir.as_ref()), splicing_manifest)
-        .context("Failed to create splicer")?;
+    let splicer =
+        Splicer::new(temp_dir_path, splicing_manifest).context("Failed to create splicer")?;
 
     let cargo = Cargo::new(opt.cargo, opt.rustc.clone());
 
     // Splice together the manifest
     let manifest_path = splicer
-        .splice_workspace(&cargo)
+        .splice_workspace(opt.nonhermetic_root_bazel_workspace_dir.as_std_path())
         .context("Failed to splice workspace")?;
 
     // Gather a cargo lockfile

crate_universe/src/metadata.rs

@@ -4,13 +4,15 @@ mod cargo_bin;
 mod cargo_tree_resolver;
 mod dependency;
 mod metadata_annotation;
+mod workspace_discoverer;
 
 use std::env;
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
 use anyhow::{bail, Context, Result};
+use camino::Utf8Path;
 use cargo_lock::Lockfile as CargoLockfile;
 use cargo_metadata::Metadata as CargoMetadata;
 use tracing::debug;
@@ -19,6 +21,7 @@ pub(crate) use self::cargo_bin::*;
 pub(crate) use self::cargo_tree_resolver::*;
 pub(crate) use self::dependency::*;
 pub(crate) use self::metadata_annotation::*;
+pub(crate) use self::workspace_discoverer::*;
 
 // TODO: This should also return a set of [crate-index::IndexConfig]s for packages in metadata.packages
 /// A Trait for generating metadata (`cargo metadata` output and a lock file) from a Cargo manifest.
@@ -187,11 +190,11 @@ impl LockGenerator {
     #[tracing::instrument(name = "LockGenerator::generate", skip_all)]
     pub(crate) fn generate(
         &self,
-        manifest_path: &Path,
+        manifest_path: &Utf8Path,
         existing_lock: &Option<PathBuf>,
         update_request: &Option<CargoUpdateRequest>,
     ) -> Result<cargo_lock::Lockfile> {
-        debug!("Generating Cargo Lockfile for {}", manifest_path.display());
+        debug!("Generating Cargo Lockfile for {}", manifest_path);
 
         let manifest_dir = manifest_path.parent().unwrap();
         let generated_lockfile_path = manifest_dir.join("Cargo.lock");
@@ -212,7 +215,7 @@ impl LockGenerator {
             fs::copy(lock, &generated_lockfile_path)?;
 
             if let Some(request) = update_request {
-                request.update(manifest_path, &self.cargo_bin)?;
+                request.update(manifest_path.as_std_path(), &self.cargo_bin)?;
             }
 
             // Ensure the Cargo cache is up to date to simulate the behavior
@@ -223,14 +226,14 @@ impl LockGenerator {
                 // Cargo detects config files based on `pwd` when running so
                 // to ensure user provided Cargo config files are used, it's
                 // critical to set the working directory to the manifest dir.
-                .current_dir(manifest_dir)
+                .current_dir(manifest_dir.as_std_path())
                 .arg("fetch")
                 .arg("--manifest-path")
-                .arg(manifest_path)
+                .arg(manifest_path.as_std_path())
                 .output()
                 .context(format!(
                     "Error running cargo to fetch crates '{}'",
-                    manifest_path.display()
+                    manifest_path
                 ))?;
 
             if !output.status.success() {
@@ -250,14 +253,14 @@ impl LockGenerator {
                 // Cargo detects config files based on `pwd` when running so
                 // to ensure user provided Cargo config files are used, it's
                 // critical to set the working directory to the manifest dir.
-                .current_dir(manifest_dir)
+                .current_dir(manifest_dir.as_std_path())
                 .arg("generate-lockfile")
                 .arg("--manifest-path")
-                .arg(manifest_path)
+                .arg(manifest_path.as_std_path())
                 .output()
                 .context(format!(
                     "Error running cargo to generate lockfile '{}'",
-                    manifest_path.display()
+                    manifest_path
                 ))?;
 
             if !output.status.success() {
@@ -269,7 +272,7 @@ impl LockGenerator {
 
         cargo_lock::Lockfile::load(&generated_lockfile_path).context(format!(
             "Failed to load lockfile: {}",
-            generated_lockfile_path.display()
+            generated_lockfile_path
         ))
     }
 }
@@ -291,12 +294,8 @@ impl VendorGenerator {
         }
     }
     #[tracing::instrument(name = "VendorGenerator::generate", skip_all)]
-    pub(crate) fn generate(&self, manifest_path: &Path, output_dir: &Path) -> Result<()> {
-        debug!(
-            "Vendoring {} to {}",
-            manifest_path.display(),
-            output_dir.display()
-        );
+    pub(crate) fn generate(&self, manifest_path: &Utf8Path, output_dir: &Path) -> Result<()> {
+        debug!("Vendoring {} to {}", manifest_path, output_dir.display());
         let manifest_dir = manifest_path.parent().unwrap();
 
         // Simply invoke `cargo generate-lockfile`
@@ -306,10 +305,10 @@ impl VendorGenerator {
             // Cargo detects config files based on `pwd` when running so
             // to ensure user provided Cargo config files are used, it's
             // critical to set the working directory to the manifest dir.
-            .current_dir(manifest_dir)
+            .current_dir(manifest_dir.as_std_path())
             .arg("vendor")
             .arg("--manifest-path")
-            .arg(manifest_path)
+            .arg(manifest_path.as_std_path())
             .arg("--locked")
             .arg("--versioned-dirs")
             .arg(output_dir)
@@ -318,7 +317,7 @@ impl VendorGenerator {
             .with_context(|| {
                 format!(
                     "Error running cargo to vendor sources for manifest '{}'",
-                    manifest_path.display()
+                    manifest_path
                 )
             })?;
 

crate_universe/src/metadata/cargo_bin.rs

@@ -33,12 +33,6 @@ impl Cargo {
         }
     }
 
-    #[cfg(test)]
-    pub(crate) fn with_cargo_home(mut self, path: PathBuf) -> Cargo {
-        self.cargo_home = Some(path);
-        self
-    }
-
     /// Returns a new `Command` for running this cargo.
     pub(crate) fn command(&self) -> Result<Command> {
         let mut command = Command::new(&self.path);

crate_universe/src/metadata/cargo_tree_resolver.rs

@@ -6,6 +6,7 @@ use std::path::{Path, PathBuf};
 use std::process::Child;
 
 use anyhow::{anyhow, bail, Context, Result};
+use camino::Utf8Path;
 use semver::Version;
 use serde::{Deserialize, Serialize};
 use tracing::debug;
@@ -303,12 +304,12 @@ impl TreeResolver {
     #[tracing::instrument(name = "TreeResolver::generate", skip_all)]
     pub(crate) fn generate(
         &self,
-        pristine_manifest_path: &Path,
+        pristine_manifest_path: &Utf8Path,
         target_triples: &BTreeSet<TargetTriple>,
     ) -> Result<TreeResolverMetadata> {
         debug!(
             "Generating features for manifest {}",
-            pristine_manifest_path.display()
+            pristine_manifest_path
         );
 
         let tempdir = tempfile::tempdir().context("Failed to make tempdir")?;
@@ -446,7 +447,7 @@ impl TreeResolver {
     // and if we don't have this fake root injection, cross-compiling from Darwin to Linux won't work because features don't get correctly resolved for the exec=darwin case.
     fn copy_project_with_explicit_deps_on_all_transitive_proc_macros(
         &self,
-        pristine_manifest_path: &Path,
+        pristine_manifest_path: &Utf8Path,
         output_dir: &Path,
     ) -> Result<PathBuf> {
         if !output_dir.exists() {
@@ -480,8 +481,11 @@ impl TreeResolver {
 
         let cargo_metadata = self
             .cargo_bin
-            .metadata_command_with_options(pristine_manifest_path, vec!["--locked".to_owned()])?
-            .manifest_path(pristine_manifest_path)
+            .metadata_command_with_options(
+                pristine_manifest_path.as_std_path(),
+                vec!["--locked".to_owned()],
+            )?
+            .manifest_path(pristine_manifest_path.as_std_path())
             .exec()
             .context("Failed to run cargo metadata to list transitive proc macros")?;
         let proc_macros = cargo_metadata
@@ -517,10 +521,10 @@ impl TreeResolver {
             })
             .collect::<Result<BTreeSet<_>>>()?;
 
-        let mut manifest =
-            cargo_toml::Manifest::from_path(pristine_manifest_path).with_context(|| {
+        let mut manifest = cargo_toml::Manifest::from_path(pristine_manifest_path.as_std_path())
+            .with_context(|| {
                 format!(
-                    "Failed to parse Cargo.toml file at {:?}",
+                    "Failed to parse Cargo.toml file at {}",
                     pristine_manifest_path
                 )
             })?;