From 11c0025bac7958520ffe3a140bad4251542f5b58 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sun, 1 Dec 2024 14:22:15 +0000 Subject: [PATCH] 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 is a precursor for 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. --- crate_universe/Cargo.toml | 42 +- crate_universe/extensions.bzl | 6 +- crate_universe/private/splicing_utils.bzl | 2 + crate_universe/private/srcs.bzl | 1 + crate_universe/src/cli/splice.rs | 13 +- crate_universe/src/cli/vendor.rs | 8 +- crate_universe/src/metadata.rs | 37 +- crate_universe/src/metadata/cargo_bin.rs | 6 - .../src/metadata/cargo_tree_resolver.rs | 20 +- .../src/metadata/workspace_discoverer.rs | 337 +++++++++ crate_universe/src/splicing.rs | 67 +- crate_universe/src/splicing/splicer.rs | 679 ++++++------------ .../workspace_examples/non-ws/Cargo.toml | 4 + .../workspace_examples/non-ws/src/main.rs | 3 + .../workspace_examples/symlinked/Cargo.toml | 4 + .../workspace_examples/symlinked/src/main.rs | 3 + .../workspace_examples/ws1/Cargo.toml | 7 + .../workspace_examples/ws1/src/main.rs | 3 + .../workspace_examples/ws1/ws1c1/Cargo.toml | 6 + .../workspace_examples/ws1/ws1c1/src/main.rs | 3 + .../ws1/ws1c1/ws1c1c1/Cargo.toml | 6 + .../ws1/ws1c1/ws1c1c1/src/main.rs | 3 + .../workspace_examples/ws1/ws1c2/Cargo.toml | 6 + .../workspace_examples/ws1/ws1c2/src/main.rs | 3 + .../workspace_examples/ws2/Cargo.toml | 6 + .../workspace_examples/ws2/src/main.rs | 3 + .../workspace_examples/ws2/ws2c1/Cargo.toml | 6 + .../workspace_examples/ws2/ws2c1/src/main.rs | 3 + .../ws2/ws2excluded/Cargo.toml | 6 + .../ws2/ws2excluded/src/main.rs | 3 + .../ws2/ws2excluded/ws2excluded2/Cargo.toml | 6 + .../ws2/ws2excluded/ws2excluded2/src/main.rs | 3 + .../ws2/ws2excluded/ws2included/Cargo.toml | 6 + .../ws2/ws2excluded/ws2included/src/main.rs | 3 + .../tests/cargo_integration_test.rs | 1 + .../crate_universe_local_path/MODULE.bazel | 4 +- .../vendor_lazy_static.sh | 6 + 37 files changed, 780 insertions(+), 545 deletions(-) create mode 100644 crate_universe/src/metadata/workspace_discoverer.rs create mode 100644 crate_universe/test_data/workspace_examples/non-ws/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/non-ws/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/symlinked/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/symlinked/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws1/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws1/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws1/ws1c1/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws1/ws1c1/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws1/ws1c2/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws1/ws1c2/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws2/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws2/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2c1/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2c1/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2excluded/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2excluded/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/src/main.rs create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/Cargo.toml create mode 100644 crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/src/main.rs diff --git a/crate_universe/Cargo.toml b/crate_universe/Cargo.toml index 3f83984036..5f1d73093e 100644 --- a/crate_universe/Cargo.toml +++ b/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" diff --git a/crate_universe/extensions.bzl b/crate_universe/extensions.bzl index 279b9d703e..a224cc1224 100644 --- a/crate_universe/extensions.bzl +++ b/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", diff --git a/crate_universe/private/splicing_utils.bzl b/crate_universe/private/splicing_utils.bzl index 6234510127..c9ccdb77f1 100644 --- a/crate_universe/private/splicing_utils.bzl +++ b/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 diff --git a/crate_universe/private/srcs.bzl b/crate_universe/private/srcs.bzl index bea5cc28f9..53f9e7b8cd 100644 --- a/crate_universe/private/srcs.bzl +++ b/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"), diff --git a/crate_universe/src/cli/splice.rs b/crate_universe/src/cli/splice.rs index bc9da74c82..c47fcd3a8b 100644 --- a/crate_universe/src/cli/splice.rs +++ b/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, + pub workspace_dir: Option, /// 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"); diff --git a/crate_universe/src/cli/vendor.rs b/crate_universe/src/cli/vendor.rs index f80e119447..ee2fa83b60 100644 --- a/crate_universe/src/cli/vendor.rs +++ b/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 diff --git a/crate_universe/src/metadata.rs b/crate_universe/src/metadata.rs index a2e6a662b0..729c88228c 100644 --- a/crate_universe/src/metadata.rs +++ b/crate_universe/src/metadata.rs @@ -4,6 +4,7 @@ mod cargo_bin; mod cargo_tree_resolver; mod dependency; mod metadata_annotation; +mod workspace_discoverer; use std::env; use std::fs; @@ -11,6 +12,7 @@ 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, update_request: &Option, ) -> Result { - 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 ) })?; diff --git a/crate_universe/src/metadata/cargo_bin.rs b/crate_universe/src/metadata/cargo_bin.rs index 8d2555f6a8..7d00dc72e2 100644 --- a/crate_universe/src/metadata/cargo_bin.rs +++ b/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 { let mut command = Command::new(&self.path); diff --git a/crate_universe/src/metadata/cargo_tree_resolver.rs b/crate_universe/src/metadata/cargo_tree_resolver.rs index 8908db898f..eaf42f848d 100644 --- a/crate_universe/src/metadata/cargo_tree_resolver.rs +++ b/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, ) -> Result { 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 { 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::>>()?; - 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 ) })?; diff --git a/crate_universe/src/metadata/workspace_discoverer.rs b/crate_universe/src/metadata/workspace_discoverer.rs new file mode 100644 index 0000000000..51eb0fd233 --- /dev/null +++ b/crate_universe/src/metadata/workspace_discoverer.rs @@ -0,0 +1,337 @@ +use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; + +use anyhow::{anyhow, bail, Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; +use cargo_toml::Manifest; + +/// A description of Cargo.toml files and how they are related in workspaces. +/// All `Utf8PathBuf` values are paths of Cargo.toml files. +#[derive(Debug, PartialEq)] +pub(crate) struct DiscoveredWorkspaces { + workspaces_to_members: BTreeMap>, + non_workspaces: BTreeSet, +} + +impl DiscoveredWorkspaces { + pub(crate) fn workspaces(&self) -> BTreeSet { + self.workspaces_to_members.keys().cloned().collect() + } + + pub(crate) fn all_workspaces_and_members(&self) -> BTreeSet { + self.workspaces_to_members + .keys() + .chain(self.workspaces_to_members.values().flatten()) + .cloned() + .collect() + } +} + +pub(crate) fn discover_workspaces( + cargo_toml_paths: BTreeSet, + known_manifests: &BTreeMap, + bazel_workspace_root: &Path, +) -> Result { + let mut manifest_cache = ManifestCache { + cache: BTreeMap::new(), + known_manifests, + }; + discover_workspaces_with_cache(cargo_toml_paths, bazel_workspace_root, &mut manifest_cache) +} + +fn discover_workspaces_with_cache( + cargo_toml_paths: BTreeSet, + bazel_workspace_root: &Path, + manifest_cache: &mut ManifestCache, +) -> Result { + let mut discovered_workspaces = DiscoveredWorkspaces { + workspaces_to_members: BTreeMap::new(), + non_workspaces: BTreeSet::new(), + }; + + // First pass: Discover workspace parents. + for cargo_toml_path in cargo_toml_paths { + if let Some(workspace_parent) = discover_workspace_parent(&cargo_toml_path, manifest_cache) + { + discovered_workspaces + .workspaces_to_members + .insert(workspace_parent, BTreeSet::new()); + } else { + discovered_workspaces.non_workspaces.insert(cargo_toml_path); + } + } + + // Second pass: Find all child manifests. + for workspace_path in discovered_workspaces + .workspaces_to_members + .keys() + .cloned() + .collect::>() + { + let workspace_manifest = manifest_cache.get(&workspace_path).unwrap(); + 'per_child: for entry in walkdir::WalkDir::new(workspace_path.parent().unwrap()) + .follow_links(true) + .follow_root_links(true) + .into_iter() + // Avoid traversing the bazel-$workspace symlink which mirrors the whole source root. + // This is not super correct - technically the symlinks can be renamed, + // and technically people can create symlinks they care about which match this pattern to. + // But it's Good Enough. + .filter_entry(|e| { + if !e.path_is_symlink() { + return true; + } + if e.path().parent().unwrap() != bazel_workspace_root { + return true; + } + if let Some(file_name) = e.file_name().to_str() { + if file_name.starts_with("bazel-") { + return false; + } + } + true + }) + { + let entry = + entry.context("Failed to walk filesystem finding workspace Cargo.toml files")?; + + if entry.file_name() != "Cargo.toml" { + continue; + } + + let child_path = Utf8Path::from_path(entry.path()) + .ok_or_else(|| anyhow!("Failed to parse {:?} as UTF-8", entry.path()))? + .to_path_buf(); + if child_path == workspace_path { + continue; + } + + let manifest = manifest_cache + .get(&child_path) + .ok_or_else(|| anyhow!("Failed to read manifest at {}", child_path))?; + + let mut actual_workspace_path = workspace_path.clone(); + if let Some(package) = manifest.package { + if let Some(explicit_workspace_path) = package.workspace { + actual_workspace_path = + child_path.parent().unwrap().join(explicit_workspace_path); + } + } + if !discovered_workspaces + .workspaces_to_members + .contains_key(&actual_workspace_path) + { + bail!("Found manifest at {} which is a member of the workspace at {} which isn't included in the crates_universe", child_path, actual_workspace_path); + } + + for exclude in &workspace_manifest.workspace.as_ref().unwrap().exclude { + let exclude_path = workspace_path.parent().unwrap().join(exclude); + if exclude_path == child_path.parent().unwrap() { + discovered_workspaces.non_workspaces.insert(child_path); + continue 'per_child; + } + } + + discovered_workspaces + .workspaces_to_members + .get_mut(&actual_workspace_path) + .unwrap() + .insert(child_path.clone()); + } + } + + Ok(discovered_workspaces) +} + +fn discover_workspace_parent( + cargo_toml_path: &Utf8PathBuf, + manifest_cache: &mut ManifestCache, +) -> Option { + for parent_dir in cargo_toml_path.ancestors().skip(1) { + let maybe_cargo_toml_path = parent_dir.join("Cargo.toml"); + let maybe_manifest = manifest_cache.get(&maybe_cargo_toml_path); + if let Some(manifest) = maybe_manifest { + if manifest.workspace.is_some() { + return Some(maybe_cargo_toml_path); + } + } + } + None +} + +struct ManifestCache<'a> { + cache: BTreeMap>, + known_manifests: &'a BTreeMap, +} + +impl ManifestCache<'_> { + fn get(&mut self, path: &Utf8PathBuf) -> Option { + if let Some(manifest) = self.known_manifests.get(path) { + return Some(manifest.clone()); + } + if let Some(maybe_manifest) = self.cache.get(path) { + return maybe_manifest.clone(); + } + let maybe_manifest = if let Ok(manifest) = Manifest::from_path(path) { + Some(manifest) + } else { + None + }; + self.cache.insert(path.clone(), maybe_manifest.clone()); + maybe_manifest + } +} + +#[cfg(test)] +mod test { + use super::*; + use std::path::{Path, PathBuf}; + use std::sync::Mutex; + + // Both of these tests try to create the same symlink, so they can't run in parallel. + static FILESYSTEM_GUARD: Mutex<()> = Mutex::new(()); + + #[test] + fn test_discover() { + let _guard = FILESYSTEM_GUARD.lock().unwrap(); + let r = runfiles::Runfiles::create().unwrap(); + let root_dir = + runfiles::rlocation!(r, "rules_rust/crate_universe/test_data/workspace_examples") + .unwrap(); + let root_dir = Utf8PathBuf::from_path_buf(root_dir).unwrap(); + + let _symlink = DeleteOnDropDirSymlink::symlink( + Path::new("..").join("symlinked"), + root_dir.join("ws1").join("bazel-ws1").into_std_path_buf(), + ) + .unwrap(); + + let mut workspaces_to_members = BTreeMap::new(); + workspaces_to_members.insert( + root_dir.join("ws1").join("Cargo.toml"), + BTreeSet::from([ + // This isn't at the bazel repo root level, so gets included. + root_dir.join("ws1").join("bazel-ws1").join("Cargo.toml"), + root_dir.join("ws1").join("ws1c1").join("Cargo.toml"), + root_dir + .join("ws1") + .join("ws1c1") + .join("ws1c1c1") + .join("Cargo.toml"), + root_dir.join("ws1").join("ws1c2").join("Cargo.toml"), + ]), + ); + workspaces_to_members.insert( + root_dir.join("ws2").join("Cargo.toml"), + BTreeSet::from([ + root_dir.join("ws2").join("ws2c1").join("Cargo.toml"), + root_dir + .join("ws2") + .join("ws2excluded") + .join("ws2included") + .join("Cargo.toml"), + ]), + ); + let non_workspaces = BTreeSet::from([ + root_dir.join("non-ws").join("Cargo.toml"), + root_dir.join("ws2").join("ws2excluded").join("Cargo.toml"), + root_dir + .join("ws2") + .join("ws2excluded") + .join("ws2excluded2") + .join("Cargo.toml"), + ]); + + let actual = discover_workspaces( + vec![ + root_dir.join("ws1/ws1c1/Cargo.toml"), + root_dir.join("ws2/Cargo.toml"), + root_dir.join("non-ws/Cargo.toml"), + ] + .into_iter() + .collect(), + &BTreeMap::new(), + root_dir.as_std_path(), + ) + .unwrap(); + let expected = DiscoveredWorkspaces { + workspaces_to_members, + non_workspaces, + }; + + assert_eq!(expected, actual); + } + + #[test] + fn test_ignore_bazel_root_symlink() { + let _guard = FILESYSTEM_GUARD.lock().unwrap(); + let r = runfiles::Runfiles::create().unwrap(); + let root_dir = + runfiles::rlocation!(r, "rules_rust/crate_universe/test_data/workspace_examples") + .unwrap(); + let root_dir = Utf8PathBuf::from_path_buf(root_dir).unwrap(); + + let _symlink = DeleteOnDropDirSymlink::symlink( + Path::new("..").join("symlinked"), + root_dir.join("ws1").join("bazel-ws1").into_std_path_buf(), + ) + .unwrap(); + + let mut workspaces_to_members = BTreeMap::new(); + workspaces_to_members.insert( + root_dir.join("ws1").join("Cargo.toml"), + BTreeSet::from([ + root_dir.join("ws1").join("ws1c1").join("Cargo.toml"), + root_dir + .join("ws1") + .join("ws1c1") + .join("ws1c1c1") + .join("Cargo.toml"), + root_dir.join("ws1").join("ws1c2").join("Cargo.toml"), + ]), + ); + let non_workspaces = BTreeSet::new(); + + let actual = discover_workspaces( + vec![root_dir.join("ws1/ws1c1/Cargo.toml")] + .into_iter() + .collect(), + &BTreeMap::new(), + root_dir.join("ws1").as_std_path(), + ) + .unwrap(); + let expected = DiscoveredWorkspaces { + workspaces_to_members, + non_workspaces, + }; + + assert_eq!(expected, actual); + } + + struct DeleteOnDropDirSymlink(PathBuf); + + impl DeleteOnDropDirSymlink { + #[cfg(unix)] + fn symlink>(original: P, link: PathBuf) -> std::io::Result { + std::os::unix::fs::symlink(original, &link)?; + Ok(Self(link)) + } + + #[cfg(windows)] + fn symlink>(original: P, link: PathBuf) -> std::io::Result { + std::os::windows::fs::symlink_dir(original, &link)?; + Ok(Self(link)) + } + } + + impl Drop for DeleteOnDropDirSymlink { + #[cfg(unix)] + fn drop(&mut self) { + std::fs::remove_file(&self.0).expect("Failed to delete symlink"); + } + #[cfg(windows)] + fn drop(&mut self) { + std::fs::remove_dir(&self.0).expect("Failed to delete symlink"); + } + } +} diff --git a/crate_universe/src/splicing.rs b/crate_universe/src/splicing.rs index 7f0560f47d..92f7d6e05a 100644 --- a/crate_universe/src/splicing.rs +++ b/crate_universe/src/splicing.rs @@ -10,6 +10,7 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use anyhow::{anyhow, bail, Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; use cargo_lock::package::SourceKind; use cargo_toml::Manifest; use serde::{Deserialize, Serialize}; @@ -33,10 +34,10 @@ pub(crate) struct SplicingManifest { pub(crate) direct_packages: DirectPackageManifest, /// A mapping of manifest paths to the labels representing them - pub(crate) manifests: BTreeMap, + pub(crate) manifests: BTreeMap, /// The path of a Cargo config file - pub(crate) cargo_config: Option, + pub(crate) cargo_config: Option, /// The Cargo resolver version to use for splicing pub(crate) resolver_version: cargo_toml::Resolver, @@ -71,20 +72,20 @@ impl SplicingManifest { .into_iter() .map(|(path, label)| { let resolved_path = path - .to_string_lossy() + .to_string() .replace("${build_workspace_directory}", &workspace_dir_str) .replace("${output_base}", &output_base_str); - (PathBuf::from(resolved_path), label) + (Utf8PathBuf::from(resolved_path), label) }) .collect(); // Ensure the cargo config is located at an absolute path let cargo_config = cargo_config.map(|path| { let resolved_path = path - .to_string_lossy() + .to_string() .replace("${build_workspace_directory}", &workspace_dir_str) .replace("${output_base}", &output_base_str); - PathBuf::from(resolved_path) + Utf8PathBuf::from(resolved_path) }); Self { @@ -125,17 +126,17 @@ impl TryFrom for SplicingMetadata { // workspace manifest is also included in the hash. // See https://github.com/bazelbuild/rules_rust/issues/2016 let manifest_content = fs::read(&path) - .with_context(|| format!("Failed to load manifest '{}'", path.display()))?; + .with_context(|| format!("Failed to load manifest '{}'", path))?; let manifest = cargo_toml::Manifest::from_slice(&manifest_content) - .with_context(|| format!("Failed to parse manifest '{}'", path.display()))?; + .with_context(|| format!("Failed to parse manifest '{}'", path))?; Ok((label, manifest)) }) .collect::>>()?; let cargo_config = match value.cargo_config { Some(path) => Some( - CargoConfig::try_from_path(&path) - .with_context(|| format!("Failed to load cargo config '{}'", path.display()))?, + CargoConfig::try_from_path(path.as_std_path()) + .with_context(|| format!("Failed to load cargo config '{}'", path))?, ), None => None, }; @@ -206,7 +207,7 @@ impl TryFrom for WorkspaceMetadata { impl WorkspaceMetadata { fn new( splicing_manifest: &SplicingManifest, - member_manifests: BTreeMap<&PathBuf, String>, + member_manifests: BTreeMap<&Utf8PathBuf, String>, ) -> Result { let mut package_prefixes: BTreeMap = member_manifests .iter() @@ -257,8 +258,8 @@ impl WorkspaceMetadata { cargo: &Cargo, lockfile: &cargo_lock::Lockfile, resolver_data: TreeResolverMetadata, - input_manifest_path: &Path, - output_manifest_path: &Path, + input_manifest_path: &Utf8Path, + output_manifest_path: &Utf8Path, ) -> Result<()> { let mut manifest = read_manifest(input_manifest_path)?; @@ -300,7 +301,7 @@ impl WorkspaceMetadata { .join("config.toml"); if config_path.exists() { - Some(CargoConfig::try_from_path(&config_path)?) + Some(CargoConfig::try_from_path(config_path.as_std_path())?) } else { None } @@ -395,7 +396,7 @@ impl WorkspaceMetadata { workspace_metaata.tree_metadata = resolver_data; workspace_metaata.inject_into(&mut manifest)?; - write_root_manifest(output_manifest_path, manifest)?; + write_root_manifest(output_manifest_path.as_std_path(), manifest)?; Ok(()) } @@ -424,13 +425,13 @@ impl WorkspaceMetadata { #[derive(Debug)] pub(crate) enum SplicedManifest { - Workspace(PathBuf), - Package(PathBuf), - MultiPackage(PathBuf), + Workspace(Utf8PathBuf), + Package(Utf8PathBuf), + MultiPackage(Utf8PathBuf), } impl SplicedManifest { - pub(crate) fn as_path_buf(&self) -> &PathBuf { + pub(crate) fn as_path_buf(&self) -> &Utf8PathBuf { match self { SplicedManifest::Workspace(p) => p, SplicedManifest::Package(p) => p, @@ -439,8 +440,8 @@ impl SplicedManifest { } } -pub(crate) fn read_manifest(manifest: &Path) -> Result { - let content = fs::read_to_string(manifest)?; +pub(crate) fn read_manifest(manifest: &Utf8Path) -> Result { + let content = fs::read_to_string(manifest.as_std_path())?; cargo_toml::Manifest::from_str(content.as_str()).context("Failed to deserialize manifest") } @@ -499,15 +500,15 @@ mod test { manifest.manifests, BTreeMap::from([ ( - PathBuf::from("${build_workspace_directory}/submod/Cargo.toml"), + Utf8PathBuf::from("${build_workspace_directory}/submod/Cargo.toml"), Label::from_str("//submod:Cargo.toml").unwrap() ), ( - PathBuf::from("${output_base}/external_crate/Cargo.toml"), + Utf8PathBuf::from("${output_base}/external_crate/Cargo.toml"), Label::from_str("@external_crate//:Cargo.toml").unwrap() ), ( - PathBuf::from("/tmp/abs/path/workspace/Cargo.toml"), + Utf8PathBuf::from("/tmp/abs/path/workspace/Cargo.toml"), Label::from_str("//:Cargo.toml").unwrap() ), ]) @@ -562,7 +563,9 @@ mod test { // Check cargo config assert_eq!( manifest.cargo_config, - Some(PathBuf::from("/tmp/abs/path/workspace/.cargo/config.toml")) + Some(Utf8PathBuf::from( + "/tmp/abs/path/workspace/.cargo/config.toml" + )) ); } @@ -578,7 +581,7 @@ mod test { let content = std::fs::read_to_string(path).unwrap(); let mut manifest: SplicingManifest = serde_json::from_str(&content).unwrap(); - manifest.cargo_config = Some(PathBuf::from( + manifest.cargo_config = Some(Utf8PathBuf::from( "${build_workspace_directory}/.cargo/config.toml", )); manifest = manifest.resolve( @@ -591,15 +594,15 @@ mod test { manifest.manifests, BTreeMap::from([ ( - PathBuf::from("/tmp/abs/path/workspace/submod/Cargo.toml"), + Utf8PathBuf::from("/tmp/abs/path/workspace/submod/Cargo.toml"), Label::from_str("//submod:Cargo.toml").unwrap() ), ( - PathBuf::from("/tmp/output_base/external_crate/Cargo.toml"), + Utf8PathBuf::from("/tmp/output_base/external_crate/Cargo.toml"), Label::from_str("@external_crate//:Cargo.toml").unwrap() ), ( - PathBuf::from("/tmp/abs/path/workspace/Cargo.toml"), + Utf8PathBuf::from("/tmp/abs/path/workspace/Cargo.toml"), Label::from_str("//:Cargo.toml").unwrap() ), ]) @@ -635,15 +638,15 @@ mod test { direct_packages: BTreeMap::new(), manifests: BTreeMap::from([ ( - workspace_manifest_path, + Utf8PathBuf::try_from(workspace_manifest_path).unwrap(), Label::from_str("//:Cargo.toml").unwrap(), ), ( - child_a_manifest_path, + Utf8PathBuf::try_from(child_a_manifest_path).unwrap(), Label::from_str("//child_a:Cargo.toml").unwrap(), ), ( - child_b_manifest_path, + Utf8PathBuf::try_from(child_b_manifest_path).unwrap(), Label::from_str("//child_b:Cargo.toml").unwrap(), ), ]), diff --git a/crate_universe/src/splicing/splicer.rs b/crate_universe/src/splicing/splicer.rs index 8b4b7ea84f..bfc3d7d84e 100644 --- a/crate_universe/src/splicing/splicer.rs +++ b/crate_universe/src/splicing/splicer.rs @@ -1,15 +1,16 @@ //! Utility for creating valid Cargo workspaces -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::Path; use anyhow::{bail, Context, Result}; -use cargo_toml::{Dependency, Manifest}; -use normpath::PathExt; +use camino::{Utf8Path, Utf8PathBuf}; +use cargo_toml::Manifest; use crate::config::CrateId; -use crate::splicing::{Cargo, SplicedManifest, SplicingManifest}; +use crate::metadata::discover_workspaces; +use crate::splicing::{SplicedManifest, SplicingManifest}; use crate::utils::starlark::Label; use crate::utils::symlink::{remove_symlink, symlink}; @@ -20,20 +21,20 @@ use super::{read_manifest, DirectPackageManifest, WorkspaceMetadata}; pub(crate) enum SplicerKind<'a> { /// Splice a manifest which is represented by a Cargo workspace Workspace { - path: &'a PathBuf, + path: &'a Utf8PathBuf, manifest: &'a Manifest, splicing_manifest: &'a SplicingManifest, }, /// Splice a manifest for a single package. This includes cases where /// were defined directly in Bazel. Package { - path: &'a PathBuf, + path: &'a Utf8PathBuf, manifest: &'a Manifest, splicing_manifest: &'a SplicingManifest, }, /// Splice a manifest from multiple disjoint Cargo manifests. MultiPackage { - manifests: &'a BTreeMap, + manifests: &'a BTreeMap, splicing_manifest: &'a SplicingManifest, }, } @@ -43,91 +44,60 @@ const IGNORE_LIST: &[&str] = &[".git", "bazel-*", ".svn"]; impl<'a> SplicerKind<'a> { pub(crate) fn new( - manifests: &'a BTreeMap, + manifests: &'a BTreeMap, splicing_manifest: &'a SplicingManifest, - cargo_bin: &Cargo, + nonhermetic_root_bazel_workspace_dir: &Path, ) -> Result { - // First check for any workspaces in the provided manifests - let workspace_owned: BTreeMap<&PathBuf, &Manifest> = manifests - .iter() - .filter(|(_, manifest)| is_workspace_owned(manifest)) - .collect(); - - let mut root_workspace_pair: Option<(&PathBuf, &Manifest)> = None; - - if !workspace_owned.is_empty() { - // Filter for the root workspace manifest info - let (workspace_roots, workspace_packages): ( - BTreeMap<&PathBuf, &Manifest>, - BTreeMap<&PathBuf, &Manifest>, - ) = workspace_owned - .into_iter() - .partition(|(_, manifest)| is_workspace_root(manifest)); - - if workspace_roots.len() > 1 { - bail!("When splicing manifests, there can only be 1 root workspace manifest"); - } + let workspaces = discover_workspaces( + manifests.keys().cloned().collect(), + manifests, + nonhermetic_root_bazel_workspace_dir, + )?; + let workspace_roots = workspaces.workspaces(); + if workspace_roots.len() > 1 { + bail!("When splicing manifests, manifests are not allowed to from from different workspaces. Saw manifests which belong to the following workspaces: {}", workspace_roots.iter().map(|wr| wr.to_string()).collect::>().join(", ")); + } - // This is an error case - we've detected some manifests are in a workspace, but can't - // find it. - // This block is just for trying to give as useful an error message as possible in this - // case. - if workspace_roots.is_empty() { - let sorted_manifests: BTreeSet<_> = manifests.keys().collect(); - for manifest_path in sorted_manifests { - let metadata_result = cargo_bin - .metadata_command_with_options(manifest_path, Vec::new())? - .no_deps() - .exec(); - if let Ok(metadata) = metadata_result { - let label = Label::from_absolute_path( - metadata.workspace_root.join("Cargo.toml").as_std_path(), - ); - if let Ok(label) = label { - bail!("Missing root workspace manifest. Please add the following label to the `manifests` key: \"{}\"", label); - } - } + let all_workspace_and_member_paths = workspaces.all_workspaces_and_members(); + let mut missing_labels = Vec::new(); + let mut missing_paths = Vec::new(); + for manifest_path in &all_workspace_and_member_paths { + if !manifests.contains_key(manifest_path) { + if let Ok(label) = Label::from_absolute_path(manifest_path.as_path().as_std_path()) + { + missing_labels.push(label.to_string()); + } else { + missing_paths.push(manifest_path.to_string()); } - bail!("Missing root workspace manifest. Please add the label of the workspace root to the `manifests` key"); } - - // Ensure all workspace owned manifests are members of the one workspace root - // UNWRAP: Safe because we've checked workspace_roots isn't empty. - let (root_manifest_path, root_manifest) = workspace_roots.into_iter().next().unwrap(); - let external_workspace_members: BTreeSet = workspace_packages - .into_iter() - .filter(|(manifest_path, _)| { - !is_workspace_member(root_manifest, root_manifest_path, manifest_path) - }) - .map(|(path, _)| path.to_string_lossy().to_string()) - .collect(); - - if !external_workspace_members.is_empty() { - bail!("A package was provided that appears to be a part of another workspace.\nworkspace root: '{}'\nexternal packages: {:#?}", root_manifest_path.display(), external_workspace_members) - } - - // UNWRAP: Safe because a Cargo.toml file must have a parent directory. - let root_manifest_dir = root_manifest_path.parent().unwrap(); - let missing_manifests = Self::find_missing_manifests( - root_manifest, - root_manifest_dir, - &manifests - .keys() - .map(|p| { - p.normalize() - .with_context(|| format!("Failed to normalize path {p:?}")) - }) - .collect::>()?, + } + if !missing_labels.is_empty() || !missing_paths.is_empty() { + bail!( + "Some manifests are not being tracked.{}{}", + if !missing_labels.is_empty() { + format!( + "\nPlease add the following labels to the `manifests` key:\n {}.", + missing_labels.join("\n ") + ) + } else { + String::new() + }, + if !missing_paths.is_empty() { + format!( + " Please add labels for the following paths to the `manifests` key:\n {}.", + missing_paths.join("\n ") + ) + } else { + String::new() + }, ) - .context("Identifying missing manifests")?; - if !missing_manifests.is_empty() { - bail!("Some manifests are not being tracked. Please add the following labels to the `manifests` key: {:#?}", missing_manifests); - } - - root_workspace_pair = Some((root_manifest_path, root_manifest)); } - if let Some((path, manifest)) = root_workspace_pair { + if let Some((path, manifest)) = workspace_roots + .iter() + .next() + .and_then(|path| manifests.get_key_value(path)) + { Ok(Self::Workspace { path, manifest, @@ -148,43 +118,9 @@ impl<'a> SplicerKind<'a> { } } - fn find_missing_manifests( - root_manifest: &Manifest, - root_manifest_dir: &Path, - known_manifest_paths: &BTreeSet, - ) -> Result> { - let workspace_manifest_paths = root_manifest - .workspace - .as_ref() - .unwrap() - .members - .iter() - .map(|member| { - let path = root_manifest_dir.join(member).join("Cargo.toml"); - path.normalize() - .with_context(|| format!("Failed to normalize path {path:?}")) - }) - .collect::, _>>()?; - - // Ensure all workspace members are present for the given workspace - workspace_manifest_paths - .into_iter() - .filter(|workspace_manifest_path| { - !known_manifest_paths.contains(workspace_manifest_path) - }) - .map(|workspace_manifest_path| { - let label = Label::from_absolute_path(workspace_manifest_path.as_path()) - .with_context(|| { - format!("Failed to identify label for path {workspace_manifest_path:?}") - })?; - Ok(label.to_string()) - }) - .collect() - } - /// Performs splicing based on the current variant. #[tracing::instrument(skip_all)] - pub(crate) fn splice(&self, workspace_dir: &Path) -> Result { + pub(crate) fn splice(&self, workspace_dir: &Utf8Path) -> Result { match self { SplicerKind::Workspace { path, @@ -206,8 +142,8 @@ impl<'a> SplicerKind<'a> { /// Implementation for splicing Cargo workspaces #[tracing::instrument(skip_all)] fn splice_workspace( - workspace_dir: &Path, - path: &&PathBuf, + workspace_dir: &Utf8Path, + path: &&Utf8PathBuf, manifest: &&Manifest, splicing_manifest: &&SplicingManifest, ) -> Result { @@ -217,10 +153,14 @@ impl<'a> SplicerKind<'a> { .expect("Every manifest should havee a parent directory"); // Link the sources of the root manifest into the new workspace - symlink_roots(manifest_dir, workspace_dir, Some(IGNORE_LIST))?; + symlink_roots( + manifest_dir.as_std_path(), + workspace_dir.as_std_path(), + Some(IGNORE_LIST), + )?; // Optionally install the cargo config after contents have been symlinked - Self::setup_cargo_config(&splicing_manifest.cargo_config, workspace_dir)?; + Self::setup_cargo_config(&splicing_manifest.cargo_config, workspace_dir.as_std_path())?; // Add any additional depeendencies to the root package if !splicing_manifest.direct_packages.is_empty() { @@ -235,7 +175,7 @@ impl<'a> SplicerKind<'a> { workspace_metadata.inject_into(&mut manifest)?; // Write the root manifest - write_root_manifest(&root_manifest_path, manifest)?; + write_root_manifest(root_manifest_path.as_std_path(), manifest)?; Ok(SplicedManifest::Workspace(root_manifest_path)) } @@ -243,8 +183,8 @@ impl<'a> SplicerKind<'a> { /// Implementation for splicing individual Cargo packages #[tracing::instrument(skip_all)] fn splice_package( - workspace_dir: &Path, - path: &&PathBuf, + workspace_dir: &Utf8Path, + path: &&Utf8PathBuf, manifest: &&Manifest, splicing_manifest: &&SplicingManifest, ) -> Result { @@ -253,10 +193,14 @@ impl<'a> SplicerKind<'a> { .expect("Every manifest should havee a parent directory"); // Link the sources of the root manifest into the new workspace - symlink_roots(manifest_dir, workspace_dir, Some(IGNORE_LIST))?; + symlink_roots( + manifest_dir.as_std_path(), + workspace_dir.as_std_path(), + Some(IGNORE_LIST), + )?; // Optionally install the cargo config after contents have been symlinked - Self::setup_cargo_config(&splicing_manifest.cargo_config, workspace_dir)?; + Self::setup_cargo_config(&splicing_manifest.cargo_config, workspace_dir.as_std_path())?; // Ensure the root package manifest has a populated `workspace` member let mut manifest = (*manifest).clone(); @@ -278,7 +222,7 @@ impl<'a> SplicerKind<'a> { workspace_metadata.inject_into(&mut manifest)?; // Write the root manifest - write_root_manifest(&root_manifest_path, manifest)?; + write_root_manifest(root_manifest_path.as_std_path(), manifest)?; Ok(SplicedManifest::Package(root_manifest_path)) } @@ -286,17 +230,17 @@ impl<'a> SplicerKind<'a> { /// Implementation for splicing together multiple Cargo packages/workspaces #[tracing::instrument(skip_all)] fn splice_multi_package( - workspace_dir: &Path, - manifests: &&BTreeMap, + workspace_dir: &Utf8Path, + manifests: &&BTreeMap, splicing_manifest: &&SplicingManifest, ) -> Result { let mut manifest = default_cargo_workspace_manifest(&splicing_manifest.resolver_version); // Optionally install a cargo config file into the workspace root. - Self::setup_cargo_config(&splicing_manifest.cargo_config, workspace_dir)?; + Self::setup_cargo_config(&splicing_manifest.cargo_config, workspace_dir.as_std_path())?; let installations = - Self::inject_workspace_members(&mut manifest, manifests, workspace_dir)?; + Self::inject_workspace_members(&mut manifest, manifests, workspace_dir.as_std_path())?; // Collect all patches from the manifests provided for (_, sub_manifest) in manifests.iter() { @@ -305,7 +249,7 @@ impl<'a> SplicerKind<'a> { "Duplicate `[patch]` entries detected in {:#?}", manifests .keys() - .map(|p| p.display().to_string()) + .map(|p| p.to_string()) .collect::>() ) })?; @@ -322,14 +266,17 @@ impl<'a> SplicerKind<'a> { // Write the root manifest let root_manifest_path = workspace_dir.join("Cargo.toml"); - write_root_manifest(&root_manifest_path, manifest)?; + write_root_manifest(root_manifest_path.as_std_path(), manifest)?; Ok(SplicedManifest::MultiPackage(root_manifest_path)) } /// A helper for installing Cargo config files into the spliced workspace while also /// ensuring no other linked config file is available - fn setup_cargo_config(cargo_config_path: &Option, workspace_dir: &Path) -> Result<()> { + fn setup_cargo_config( + cargo_config_path: &Option, + workspace_dir: &Path, + ) -> Result<()> { // If the `.cargo` dir is a symlink, we'll need to relink it and ensure // a Cargo config file is omitted let dot_cargo_dir = workspace_dir.join(".cargo"); @@ -417,9 +364,9 @@ impl<'a> SplicerKind<'a> { /// Cargo workspace members. fn inject_workspace_members<'b>( root_manifest: &mut Manifest, - manifests: &'b BTreeMap, + manifests: &'b BTreeMap, workspace_dir: &Path, - ) -> Result> { + ) -> Result> { manifests .iter() .map(|(path, manifest)| { @@ -442,7 +389,11 @@ impl<'a> SplicerKind<'a> { let dest_package_dir = workspace_dir.join(package_name); - match symlink_roots(manifest_dir, &dest_package_dir, Some(IGNORE_LIST)) { + match symlink_roots( + manifest_dir.as_std_path(), + &dest_package_dir, + Some(IGNORE_LIST), + ) { Ok(_) => Ok((path, package_name.clone())), Err(e) => Err(e), } @@ -520,23 +471,26 @@ impl<'a> SplicerKind<'a> { } pub(crate) struct Splicer { - workspace_dir: PathBuf, - manifests: BTreeMap, + workspace_dir: Utf8PathBuf, + manifests: BTreeMap, splicing_manifest: SplicingManifest, } impl Splicer { - pub(crate) fn new(workspace_dir: PathBuf, splicing_manifest: SplicingManifest) -> Result { + pub(crate) fn new( + workspace_dir: Utf8PathBuf, + splicing_manifest: SplicingManifest, + ) -> Result { // Load all manifests let manifests = splicing_manifest .manifests .keys() .map(|path| { let m = read_manifest(path) - .with_context(|| format!("Failed to read manifest at {}", path.display()))?; + .with_context(|| format!("Failed to read manifest at {}", path))?; Ok((path.clone(), m)) }) - .collect::>>()?; + .collect::>>()?; Ok(Self { workspace_dir, @@ -546,9 +500,16 @@ impl Splicer { } /// Build a new workspace root - pub(crate) fn splice_workspace(&self, cargo: &Cargo) -> Result { - SplicerKind::new(&self.manifests, &self.splicing_manifest, cargo)? - .splice(&self.workspace_dir) + pub(crate) fn splice_workspace( + &self, + nonhermetic_root_bazel_workspace_dir: &Path, + ) -> Result { + SplicerKind::new( + &self.manifests, + &self.splicing_manifest, + nonhermetic_root_bazel_workspace_dir, + )? + .splice(&self.workspace_dir) } } const DEFAULT_SPLICING_PACKAGE_NAME: &str = "direct-cargo-bazel-deps"; @@ -601,48 +562,6 @@ pub(crate) fn default_cargo_workspace_manifest( manifest } -/// Determine whtether or not the manifest is a workspace root -pub(crate) fn is_workspace_root(manifest: &Manifest) -> bool { - // Anything with any workspace data is considered a workspace - manifest.workspace.is_some() -} - -/// Evaluates whether or not a manifest is considered a "workspace" manifest. -/// See [Cargo workspaces](https://doc.rust-lang.org/cargo/reference/workspaces.html). -pub(crate) fn is_workspace_owned(manifest: &Manifest) -> bool { - if is_workspace_root(manifest) { - return true; - } - - // Additionally, anything that contains path dependencies is also considered a workspace - manifest.dependencies.iter().any(|(_, dep)| match dep { - Dependency::Detailed(dep) => dep.path.is_some(), - _ => false, - }) -} - -/// Determines whether or not a particular manifest is a workspace member to a given root manifest -pub(crate) fn is_workspace_member( - root_manifest: &Manifest, - root_manifest_path: &Path, - manifest_path: &Path, -) -> bool { - let members = match root_manifest.workspace.as_ref() { - Some(workspace) => &workspace.members, - None => return false, - }; - - let root_parent = root_manifest_path - .parent() - .expect("All manifest paths should have a parent"); - let manifest_abs_path = root_parent.join(manifest_path); - - members.iter().any(|member| { - let member_manifest_path = root_parent.join(member).join("Cargo.toml"); - member_manifest_path == manifest_abs_path - }) -} - pub(crate) fn write_root_manifest(path: &Path, manifest: cargo_toml::Manifest) -> Result<()> { // Remove the file in case one exists already, preventing symlinked files // from having their contents overwritten. @@ -732,7 +651,6 @@ mod test { use std::str::FromStr; use cargo_metadata::PackageId; - use maplit::btreeset; use crate::splicing::Cargo; @@ -763,7 +681,7 @@ mod test { /// Get cargo and rustc binaries the Bazel way #[cfg(not(feature = "cargo"))] - fn get_cargo_and_rustc_paths() -> (PathBuf, PathBuf) { + fn get_cargo_and_rustc_paths() -> (std::path::PathBuf, std::path::PathBuf) { let r = runfiles::Runfiles::create().unwrap(); let cargo_path = runfiles::rlocation!(r, concat!("rules_rust/", env!("CARGO"))).unwrap(); let rustc_path = runfiles::rlocation!(r, concat!("rules_rust/", env!("RUSTC"))).unwrap(); @@ -782,20 +700,20 @@ mod test { Cargo::new(cargo, rustc) } - fn generate_metadata(manifest_path: &Path) -> cargo_metadata::Metadata { + fn generate_metadata>(manifest_path: P) -> cargo_metadata::Metadata { cargo() - .metadata_command_with_options(manifest_path, vec!["--offline".to_owned()]) + .metadata_command_with_options(manifest_path.as_ref(), vec!["--offline".to_owned()]) .unwrap() .exec() .unwrap() } - fn mock_cargo_toml(path: &Path, name: &str) -> cargo_toml::Manifest { + fn mock_cargo_toml>(path: P, name: &str) -> cargo_toml::Manifest { mock_cargo_toml_with_dependencies(path, name, &[]) } - fn mock_cargo_toml_with_dependencies( - path: &Path, + fn mock_cargo_toml_with_dependencies>( + path: P, name: &str, deps: &[&str], ) -> cargo_toml::Manifest { @@ -816,6 +734,7 @@ mod test { ))) .unwrap(); + let path = path.as_ref(); fs::create_dir_all(path.parent().unwrap()).unwrap(); fs::write(path, toml::to_string(&manifest).unwrap()).unwrap(); @@ -863,11 +782,14 @@ mod test { // Write workspace members for pkg in &["sub_pkg_a", "sub_pkg_b"] { - let manifest_path = cache_dir - .as_ref() - .join("root_pkg") - .join(pkg) - .join("Cargo.toml"); + let manifest_path = Utf8PathBuf::try_from( + cache_dir + .as_ref() + .join("root_pkg") + .join(pkg) + .join("Cargo.toml"), + ) + .unwrap(); let deps = if pkg == &"sub_pkg_b" { vec![r#"sub_pkg_a = { path = "../sub_pkg_a" }"#] } else { @@ -903,7 +825,7 @@ mod test { File::create(workspace_root.join("WORKSPACE.bazel")).unwrap(); } let root_pkg = workspace_root.join("root_pkg"); - let manifest_path = root_pkg.join("Cargo.toml"); + let manifest_path = Utf8PathBuf::try_from(root_pkg.join("Cargo.toml")).unwrap(); fs::create_dir_all(manifest_path.parent().unwrap()).unwrap(); fs::write(&manifest_path, toml::to_string(&manifest).unwrap()).unwrap(); { @@ -930,7 +852,8 @@ mod test { // Write workspace members for pkg in &["sub_pkg_a", "sub_pkg_b"] { - let manifest_path = cache_dir.as_ref().join(pkg).join("Cargo.toml"); + let manifest_path = + Utf8PathBuf::try_from(cache_dir.as_ref().join(pkg).join("Cargo.toml")).unwrap(); mock_cargo_toml(&manifest_path, pkg); splicing_manifest.manifests.insert( @@ -960,7 +883,7 @@ mod test { { File::create(workspace_root.join("WORKSPACE.bazel")).unwrap(); } - let manifest_path = workspace_root.join("Cargo.toml"); + let manifest_path = Utf8PathBuf::try_from(workspace_root.join("Cargo.toml")).unwrap(); fs::create_dir_all(manifest_path.parent().unwrap()).unwrap(); fs::write(&manifest_path, toml::to_string(&manifest).unwrap()).unwrap(); @@ -982,7 +905,8 @@ mod test { let cache_dir = tempfile::tempdir().unwrap(); // Add an additional package - let manifest_path = cache_dir.as_ref().join("root_pkg").join("Cargo.toml"); + let manifest_path = + Utf8PathBuf::try_from(cache_dir.as_ref().join("root_pkg").join("Cargo.toml")).unwrap(); mock_cargo_toml(&manifest_path, "root_pkg"); splicing_manifest .manifests @@ -997,7 +921,8 @@ mod test { // Add an additional package for pkg in &["pkg_a", "pkg_b", "pkg_c"] { - let manifest_path = cache_dir.as_ref().join(pkg).join("Cargo.toml"); + let manifest_path = + Utf8PathBuf::try_from(cache_dir.as_ref().join(pkg).join("Cargo.toml")).unwrap(); mock_cargo_toml(&manifest_path, pkg); splicing_manifest .manifests @@ -1053,9 +978,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Locate cargo @@ -1096,9 +1021,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Locate cargo @@ -1144,10 +1069,12 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); - let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) - .unwrap() - .splice_workspace(&cargo()); + let workspace_manifest = Splicer::new( + Utf8PathBuf::try_from(workspace_root.as_ref().to_path_buf()).unwrap(), + splicing_manifest, + ) + .unwrap() + .splice_workspace(Path::new("/doesnotexist")); assert!(workspace_manifest.is_err()); @@ -1172,17 +1099,19 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); - let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) - .unwrap() - .splice_workspace(&cargo()); + let workspace_manifest = Splicer::new( + Utf8PathBuf::try_from(workspace_root.as_ref().to_path_buf()).unwrap(), + splicing_manifest, + ) + .unwrap() + .splice_workspace(Path::new("/doesnotexist")); assert!(workspace_manifest.is_err()); // Ensure both the missing manifests are mentioned in the error string let err_str = format!("{:?}", &workspace_manifest); assert!( - err_str.contains("Missing root workspace manifest") + err_str.contains("Some manifests are not being tracked") && err_str.contains("//root_pkg:Cargo.toml") ); } @@ -1193,11 +1122,31 @@ mod test { // Add a new package from an existing external workspace let external_workspace_root = tempfile::tempdir().unwrap(); - let external_manifest = external_workspace_root - .as_ref() - .join("external_workspace_member") - .join("Cargo.toml"); + let external_manifest = Utf8PathBuf::try_from( + external_workspace_root + .as_ref() + .join("external_workspace_member") + .join("Cargo.toml"), + ) + .unwrap(); fs::create_dir_all(external_manifest.parent().unwrap()).unwrap(); + + fs::write( + external_workspace_root.as_ref().join("Cargo.toml"), + textwrap::dedent( + r#" + [workspace] + [package] + name = "external_workspace_root" + version = "0.0.1" + + [lib] + path = "lib.rs" + "#, + ), + ) + .unwrap(); + fs::write( &external_manifest, textwrap::dedent( @@ -1208,9 +1157,6 @@ mod test { [lib] path = "lib.rs" - - [dependencies] - neighbor = { path = "../neighbor" } "#, ), ) @@ -1224,19 +1170,18 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()); + .splice_workspace(Path::new("/doesnotexist")); assert!(workspace_manifest.is_err()); // Ensure both the external workspace member let err_str = format!("{:?}", &workspace_manifest); - let bytes_str = format!("{:?}", external_manifest.to_string_lossy()); assert!( err_str - .contains("A package was provided that appears to be a part of another workspace.") - && err_str.contains(&bytes_str) + .contains("When splicing manifests, manifests are not allowed to from from different workspaces. Saw manifests which belong to the following workspaces:") + && err_str.contains(external_workspace_root.path().to_string_lossy().as_ref()) ); } @@ -1262,9 +1207,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); let metadata = generate_metadata(workspace_manifest.as_path_buf()); @@ -1287,9 +1232,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Locate cargo @@ -1324,9 +1269,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Check the default resolver version @@ -1374,9 +1319,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Check the specified resolver version @@ -1420,7 +1365,7 @@ mod test { return; } - let (mut splicing_manifest, cache_dir) = mock_splicing_manifest_with_multi_package(); + let (mut splicing_manifest, _cache_dir) = mock_splicing_manifest_with_multi_package(); // Add a "direct dependency" entry splicing_manifest.direct_packages.insert( @@ -1434,9 +1379,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo().with_cargo_home(cache_dir.path().to_owned())) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Check the default resolver version @@ -1476,9 +1421,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo().with_cargo_home(cache_dir.path().to_owned())) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Ensure the patches match the expected value @@ -1539,9 +1484,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo().with_cargo_home(cache_dir.path().to_owned())) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Ensure the patches match the expected value @@ -1590,9 +1535,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); let workspace_manifest = - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo().with_cargo_home(cache_dir.path().to_owned())) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); // Ensure the patches match the expected value @@ -1632,9 +1577,9 @@ mod test { // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); - let result = Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + let result = Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()); + .splice_workspace(Path::new("/doesnotexist")); // Confirm conflicting patches have been detected assert!(result.is_err()); @@ -1648,15 +1593,15 @@ mod test { // Write a cargo config let temp_dir = tempfile::tempdir().unwrap(); - let external_config = temp_dir.as_ref().join("config.toml"); + let external_config = tempdir_utf8pathbuf(&temp_dir).join("config.toml"); fs::write(&external_config, "# Cargo configuration file").unwrap(); splicing_manifest.cargo_config = Some(external_config); // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); let cargo_config = workspace_root.as_ref().join(".cargo").join("config.toml"); @@ -1681,15 +1626,15 @@ mod test { // Write a cargo config let temp_dir = tempfile::tempdir().unwrap(); - let external_config = temp_dir.as_ref().join("config.toml"); + let external_config = tempdir_utf8pathbuf(&temp_dir).join("config.toml"); fs::write(&external_config, "# Cargo configuration file").unwrap(); splicing_manifest.cargo_config = Some(external_config); // Splice the workspace let workspace_root = tempfile::tempdir().unwrap(); - Splicer::new(workspace_root.as_ref().to_path_buf(), splicing_manifest) + Splicer::new(tempdir_utf8pathbuf(&workspace_root), splicing_manifest) .unwrap() - .splice_workspace(&cargo()) + .splice_workspace(Path::new("/doesnotexist")) .unwrap(); let cargo_config = workspace_root.as_ref().join(".cargo").join("config.toml"); @@ -1706,230 +1651,24 @@ mod test { // Write a cargo config let temp_dir = tempfile::tempdir().unwrap(); - let dot_cargo_dir = temp_dir.as_ref().join(".cargo"); + let dot_cargo_dir = tempdir_utf8pathbuf(&temp_dir).join(".cargo"); fs::create_dir_all(&dot_cargo_dir).unwrap(); let external_config = dot_cargo_dir.join("config.toml"); fs::write(&external_config, "# Cargo configuration file").unwrap(); splicing_manifest.cargo_config = Some(external_config.clone()); // Splice the workspace - let workspace_root = temp_dir.as_ref().join("workspace_root"); + let workspace_root = tempdir_utf8pathbuf(&temp_dir).join("workspace_root"); let splicing_result = Splicer::new(workspace_root.clone(), splicing_manifest) .unwrap() - .splice_workspace(&cargo()); + .splice_workspace(Path::new("/doesnotexist")); // Ensure cargo config files in parent directories lead to errors assert!(splicing_result.is_err()); let err_str = splicing_result.err().unwrap().to_string(); assert!(err_str.starts_with("A Cargo config file was found in a parent directory")); - assert!(err_str.contains(&format!("Workspace = {}", workspace_root.display()))); - assert!(err_str.contains(&format!("Cargo config = {}", external_config.display()))); - } - - #[test] - fn find_missing_manifests_correct_without_root() { - let temp_dir = tempfile::tempdir().unwrap(); - let root_manifest_dir = temp_dir.path(); - touch(&root_manifest_dir.join("WORKSPACE.bazel")); - touch(&root_manifest_dir.join("BUILD.bazel")); - touch(&root_manifest_dir.join("Cargo.toml")); - touch(&root_manifest_dir.join("foo").join("Cargo.toml")); - touch(&root_manifest_dir.join("bar").join("BUILD.bazel")); - touch(&root_manifest_dir.join("bar").join("Cargo.toml")); - - let known_manifest_paths = btreeset![ - root_manifest_dir - .join("foo") - .join("Cargo.toml") - .normalize() - .unwrap(), - root_manifest_dir - .join("bar") - .join("Cargo.toml") - .normalize() - .unwrap(), - ]; - - let root_manifest: cargo_toml::Manifest = toml::toml! { - [workspace] - members = [ - "foo", - "bar", - ] - [package] - name = "root_pkg" - version = "0.0.1" - - [lib] - path = "lib.rs" - } - .try_into() - .unwrap(); - let missing_manifests = SplicerKind::find_missing_manifests( - &root_manifest, - root_manifest_dir, - &known_manifest_paths, - ) - .unwrap(); - assert_eq!(missing_manifests, btreeset![]); - } - - #[test] - fn find_missing_manifests_correct_with_root() { - let temp_dir = tempfile::tempdir().unwrap(); - let root_manifest_dir = temp_dir.path(); - touch(&root_manifest_dir.join("WORKSPACE.bazel")); - touch(&root_manifest_dir.join("BUILD.bazel")); - touch(&root_manifest_dir.join("Cargo.toml")); - touch(&root_manifest_dir.join("foo").join("Cargo.toml")); - touch(&root_manifest_dir.join("bar").join("BUILD.bazel")); - touch(&root_manifest_dir.join("bar").join("Cargo.toml")); - - let known_manifest_paths = btreeset![ - root_manifest_dir.join("Cargo.toml").normalize().unwrap(), - root_manifest_dir - .join("foo") - .join("Cargo.toml") - .normalize() - .unwrap(), - root_manifest_dir - .join("bar") - .join("Cargo.toml") - .normalize() - .unwrap(), - ]; - - let root_manifest: cargo_toml::Manifest = toml::toml! { - [workspace] - members = [ - ".", - "foo", - "bar", - ] - [package] - name = "root_pkg" - version = "0.0.1" - - [lib] - path = "lib.rs" - } - .try_into() - .unwrap(); - let missing_manifests = SplicerKind::find_missing_manifests( - &root_manifest, - root_manifest_dir, - &known_manifest_paths, - ) - .unwrap(); - assert_eq!(missing_manifests, btreeset![]); - } - - #[test] - fn find_missing_manifests_missing_root() { - let temp_dir = tempfile::tempdir().unwrap(); - let root_manifest_dir = temp_dir.path(); - touch(&root_manifest_dir.join("WORKSPACE.bazel")); - touch(&root_manifest_dir.join("BUILD.bazel")); - touch(&root_manifest_dir.join("Cargo.toml")); - touch(&root_manifest_dir.join("foo").join("Cargo.toml")); - touch(&root_manifest_dir.join("bar").join("BUILD.bazel")); - touch(&root_manifest_dir.join("bar").join("Cargo.toml")); - - let known_manifest_paths = btreeset![ - root_manifest_dir - .join("foo") - .join("Cargo.toml") - .normalize() - .unwrap(), - root_manifest_dir - .join("bar") - .join("Cargo.toml") - .normalize() - .unwrap(), - ]; - - let root_manifest: cargo_toml::Manifest = toml::toml! { - [workspace] - members = [ - ".", - "foo", - "bar", - ] - [package] - name = "root_pkg" - version = "0.0.1" - - [lib] - path = "lib.rs" - } - .try_into() - .unwrap(); - let missing_manifests = SplicerKind::find_missing_manifests( - &root_manifest, - root_manifest_dir, - &known_manifest_paths, - ) - .unwrap(); - assert_eq!(missing_manifests, btreeset![String::from("//:Cargo.toml")]); - } - - #[test] - fn find_missing_manifests_missing_nonroot() { - let temp_dir = tempfile::tempdir().unwrap(); - let root_manifest_dir = temp_dir.path(); - touch(&root_manifest_dir.join("WORKSPACE.bazel")); - touch(&root_manifest_dir.join("BUILD.bazel")); - touch(&root_manifest_dir.join("Cargo.toml")); - touch(&root_manifest_dir.join("foo").join("Cargo.toml")); - touch(&root_manifest_dir.join("bar").join("BUILD.bazel")); - touch(&root_manifest_dir.join("bar").join("Cargo.toml")); - touch(&root_manifest_dir.join("baz").join("BUILD.bazel")); - touch(&root_manifest_dir.join("baz").join("Cargo.toml")); - - let known_manifest_paths = btreeset![ - root_manifest_dir - .join("foo") - .join("Cargo.toml") - .normalize() - .unwrap(), - root_manifest_dir - .join("bar") - .join("Cargo.toml") - .normalize() - .unwrap(), - ]; - - let root_manifest: cargo_toml::Manifest = toml::toml! { - [workspace] - members = [ - "foo", - "bar", - "baz", - ] - [package] - name = "root_pkg" - version = "0.0.1" - - [lib] - path = "lib.rs" - } - .try_into() - .unwrap(); - let missing_manifests = SplicerKind::find_missing_manifests( - &root_manifest, - root_manifest_dir, - &known_manifest_paths, - ) - .unwrap(); - assert_eq!( - missing_manifests, - btreeset![String::from("//baz:Cargo.toml")] - ); - } - - fn touch(path: &Path) { - std::fs::create_dir_all(path.parent().unwrap()).unwrap(); - std::fs::write(path, []).unwrap(); + assert!(err_str.contains(&format!("Workspace = {}", workspace_root))); + assert!(err_str.contains(&format!("Cargo config = {}", external_config))); } fn syn_dependency_detail() -> cargo_toml::DependencyDetail { @@ -1947,4 +1686,8 @@ mod test { ..cargo_toml::DependencyDetail::default() } } + + fn tempdir_utf8pathbuf(tempdir: &tempfile::TempDir) -> Utf8PathBuf { + Utf8PathBuf::try_from(tempdir.as_ref().to_path_buf()).unwrap() + } } diff --git a/crate_universe/test_data/workspace_examples/non-ws/Cargo.toml b/crate_universe/test_data/workspace_examples/non-ws/Cargo.toml new file mode 100644 index 0000000000..14bc9afb24 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/non-ws/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "non-ws" +version = "0.1.0" +edition = "2021" diff --git a/crate_universe/test_data/workspace_examples/non-ws/src/main.rs b/crate_universe/test_data/workspace_examples/non-ws/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/non-ws/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/symlinked/Cargo.toml b/crate_universe/test_data/workspace_examples/symlinked/Cargo.toml new file mode 100644 index 0000000000..f1cc7144e3 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/symlinked/Cargo.toml @@ -0,0 +1,4 @@ +[package] +name = "symlinked" +version = "0.1.0" +edition = "2021" diff --git a/crate_universe/test_data/workspace_examples/symlinked/src/main.rs b/crate_universe/test_data/workspace_examples/symlinked/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/symlinked/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws1/Cargo.toml b/crate_universe/test_data/workspace_examples/ws1/Cargo.toml new file mode 100644 index 0000000000..15f10821b1 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/Cargo.toml @@ -0,0 +1,7 @@ +[workspace] +members = ["ws1c1", "ws1c2"] + +[package] +name = "ws1" +version = "0.1.0" +edition = "2021" diff --git a/crate_universe/test_data/workspace_examples/ws1/src/main.rs b/crate_universe/test_data/workspace_examples/ws1/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws1/ws1c1/Cargo.toml b/crate_universe/test_data/workspace_examples/ws1/ws1c1/Cargo.toml new file mode 100644 index 0000000000..aaf6ae258f --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/ws1c1/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws1c1" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws1/ws1c1/src/main.rs b/crate_universe/test_data/workspace_examples/ws1/ws1c1/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/ws1c1/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/Cargo.toml b/crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/Cargo.toml new file mode 100644 index 0000000000..7854ddced8 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws1c1c1" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/src/main.rs b/crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/ws1c1/ws1c1c1/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws1/ws1c2/Cargo.toml b/crate_universe/test_data/workspace_examples/ws1/ws1c2/Cargo.toml new file mode 100644 index 0000000000..530ad7fc1a --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/ws1c2/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws1c2" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws1/ws1c2/src/main.rs b/crate_universe/test_data/workspace_examples/ws1/ws1c2/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws1/ws1c2/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws2/Cargo.toml b/crate_universe/test_data/workspace_examples/ws2/Cargo.toml new file mode 100644 index 0000000000..58fe97c9c1 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/Cargo.toml @@ -0,0 +1,6 @@ +[workspace] +exclude = ["ws2excluded", "ws2excluded/ws2excluded2"] +[package] +name = "ws2" +version = "0.1.0" +edition = "2021" diff --git a/crate_universe/test_data/workspace_examples/ws2/src/main.rs b/crate_universe/test_data/workspace_examples/ws2/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2c1/Cargo.toml b/crate_universe/test_data/workspace_examples/ws2/ws2c1/Cargo.toml new file mode 100644 index 0000000000..fbae0ef0e9 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2c1/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws2c1" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2c1/src/main.rs b/crate_universe/test_data/workspace_examples/ws2/ws2c1/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2c1/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2excluded/Cargo.toml b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/Cargo.toml new file mode 100644 index 0000000000..559086fa34 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws2excluded" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2excluded/src/main.rs b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/Cargo.toml b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/Cargo.toml new file mode 100644 index 0000000000..4a50272141 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws2excluded2" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/src/main.rs b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2excluded2/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/Cargo.toml b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/Cargo.toml new file mode 100644 index 0000000000..e4c6f023d6 --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "ws2included" +version = "0.1.0" +edition = "2021" + +[dependencies] diff --git a/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/src/main.rs b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/src/main.rs new file mode 100644 index 0000000000..e7a11a969c --- /dev/null +++ b/crate_universe/test_data/workspace_examples/ws2/ws2excluded/ws2included/src/main.rs @@ -0,0 +1,3 @@ +fn main() { + println!("Hello, world!"); +} diff --git a/crate_universe/tests/cargo_integration_test.rs b/crate_universe/tests/cargo_integration_test.rs index ac45952734..49b739b8d2 100644 --- a/crate_universe/tests/cargo_integration_test.rs +++ b/crate_universe/tests/cargo_integration_test.rs @@ -115,6 +115,7 @@ fn run(repository_name: &str, manifests: HashMap, lockfile: &str config, cargo, rustc, + nonhermetic_root_bazel_workspace_dir: PathBuf::from("/doesnotexist"), }) .unwrap(); diff --git a/examples/crate_universe_local_path/MODULE.bazel b/examples/crate_universe_local_path/MODULE.bazel index 34fdf16a8e..33111a9d55 100644 --- a/examples/crate_universe_local_path/MODULE.bazel +++ b/examples/crate_universe_local_path/MODULE.bazel @@ -20,8 +20,6 @@ crate = use_extension( crate.from_cargo( name = "bzlmod_crates_from_cargo_workspace", cargo_lockfile = "//crates_from_workspace:Cargo.lock", - manifests = [ - "//crates_from_workspace:Cargo.toml", - ], + manifests = ["//crates_from_workspace:Cargo.toml"], ) use_repo(crate, "bzlmod_crates_from_cargo_workspace") diff --git a/examples/crate_universe_local_path/vendor_lazy_static.sh b/examples/crate_universe_local_path/vendor_lazy_static.sh index 8621b7a457..4f81d5aee4 100755 --- a/examples/crate_universe_local_path/vendor_lazy_static.sh +++ b/examples/crate_universe_local_path/vendor_lazy_static.sh @@ -12,6 +12,12 @@ elif [[ "$#" -eq 1 ]]; then path_dep_path="$1" copy_to="crates_from_workspace/$1" mkdir -p "${copy_to}" + sed_i=(sed -i) + if [[ "$(uname)" == "Darwin" ]]; then + sed_i=(sed -i '') + fi + "${sed_i[@]}" -e 's#manifests = \["//crates_from_workspace:Cargo\.toml"\],#manifests = ["//crates_from_workspace:Cargo.toml", "//crates_from_workspace:'"$1"'/Cargo.toml"],#g' WORKSPACE.bazel + "${sed_i[@]}" -e 's#manifests = \["//crates_from_workspace:Cargo\.toml"\],#manifests = ["//crates_from_workspace:Cargo.toml", "//crates_from_workspace:'"$1"'/Cargo.toml"],#g' MODULE.bazel else echo >&2 "Usage: $0 [/path/to/copy/to]" echo >&2 "If no arg is passed, a tempdir will be created"