From e18c2c5452427b5cca2ae666954ce85e70cff90c Mon Sep 17 00:00:00 2001 From: Stepan Koltsov Date: Mon, 21 Jul 2025 19:48:01 +0100 Subject: [PATCH] Newtype CargoTomlPath When reading code, it is not always clear, what path is Cargo.toml path, and what path is directory path. --- crate_universe/private/srcs.bzl | 1 + crate_universe/src/cli/generate.rs | 6 +- crate_universe/src/cli/splice.rs | 19 +--- crate_universe/src/cli/vendor.rs | 10 +- crate_universe/src/metadata.rs | 11 ++- .../src/metadata/cargo_toml_path.rs | 60 ++++++++++++ .../src/metadata/cargo_tree_resolver.rs | 8 +- .../src/metadata/workspace_discoverer.rs | 91 +++++++++--------- crate_universe/src/splicing.rs | 55 +++++------ crate_universe/src/splicing/splicer.rs | 92 +++++++++---------- 10 files changed, 199 insertions(+), 154 deletions(-) create mode 100644 crate_universe/src/metadata/cargo_toml_path.rs diff --git a/crate_universe/private/srcs.bzl b/crate_universe/private/srcs.bzl index 53f9e7b8cd..7b226f800e 100644 --- a/crate_universe/private/srcs.bzl +++ b/crate_universe/private/srcs.bzl @@ -23,6 +23,7 @@ CARGO_BAZEL_SRCS = [ Label("//crate_universe:src/main.rs"), Label("//crate_universe:src/metadata.rs"), Label("//crate_universe:src/metadata/cargo_bin.rs"), + Label("//crate_universe:src/metadata/cargo_toml_path.rs"), Label("//crate_universe:src/metadata/cargo_tree_resolver.rs"), Label("//crate_universe:src/metadata/cargo_tree_rustc_wrapper.bat"), Label("//crate_universe:src/metadata/cargo_tree_rustc_wrapper.sh"), diff --git a/crate_universe/src/cli/generate.rs b/crate_universe/src/cli/generate.rs index 1769683fc0..97485912b8 100644 --- a/crate_universe/src/cli/generate.rs +++ b/crate_universe/src/cli/generate.rs @@ -13,7 +13,7 @@ use clap::Parser; use crate::config::Config; use crate::context::Context; use crate::lockfile::{lock_context, write_lockfile}; -use crate::metadata::{load_metadata, Annotations, Cargo, SourceAnnotation}; +use crate::metadata::{load_metadata, Annotations, Cargo, CargoTomlPath, SourceAnnotation}; use crate::rendering::{write_outputs, Renderer}; use crate::splicing::SplicingManifest; use crate::utils::normalize_cargo_file_paths; @@ -236,7 +236,7 @@ fn update_cargo_lockfile(path: &Path, cargo_lockfile: Lockfile) -> Result<()> { fn write_paths_to_track< 'a, SourceAnnotations: Iterator, - Paths: Iterator, + Paths: Iterator, UnusedPatches: Iterator, >( output_file: &Path, @@ -248,7 +248,7 @@ fn write_paths_to_track< let source_annotation_manifests: BTreeSet<_> = source_annotations .filter_map(|v| { if let SourceAnnotation::Path { path } = v { - Some(path.join("Cargo.toml")) + Some(CargoTomlPath::for_dir(path)) } else { None } diff --git a/crate_universe/src/cli/splice.rs b/crate_universe/src/cli/splice.rs index a3cdec8551..3e0973fb28 100644 --- a/crate_universe/src/cli/splice.rs +++ b/crate_universe/src/cli/splice.rs @@ -99,7 +99,7 @@ pub fn splice(opt: SpliceOptions) -> Result<()> { let resolver_data = TreeResolver::new(cargo.clone()) .generate( - manifest_path.as_path_buf(), + manifest_path.cargo_toml_path(), &config.supported_platform_triples, ) .context("Failed to generate features")?; @@ -108,8 +108,8 @@ pub fn splice(opt: SpliceOptions) -> Result<()> { &cargo, &cargo_lockfile, resolver_data, - manifest_path.as_path_buf(), - manifest_path.as_path_buf(), + manifest_path.cargo_toml_path(), + manifest_path.cargo_toml_path(), ) .context("Failed to write registry URLs and feature map")?; @@ -119,19 +119,10 @@ pub fn splice(opt: SpliceOptions) -> Result<()> { let (cargo_metadata, _) = Generator::new() .with_cargo(cargo) .with_rustc(opt.rustc) - .generate(manifest_path.as_path_buf()) + .generate(manifest_path.cargo_toml_path()) .context("Failed to generate cargo metadata")?; - let cargo_lockfile_path = manifest_path - .as_path_buf() - .parent() - .with_context(|| { - format!( - "The path {} is expected to have a parent directory", - manifest_path.as_path_buf() - ) - })? - .join("Cargo.lock"); + let cargo_lockfile_path = manifest_path.cargo_toml_path().parent().join("Cargo.lock"); // Generate the consumable outputs of the splicing process std::fs::create_dir_all(&output_dir) diff --git a/crate_universe/src/cli/vendor.rs b/crate_universe/src/cli/vendor.rs index ce480658fd..604eab1a0e 100644 --- a/crate_universe/src/cli/vendor.rs +++ b/crate_universe/src/cli/vendor.rs @@ -227,7 +227,7 @@ pub fn vendor(opt: VendorOptions) -> anyhow::Result<()> { let config = Config::try_from_path(&opt.config)?; let resolver_data = TreeResolver::new(cargo.clone()).generate( - manifest_path.as_path_buf(), + manifest_path.cargo_toml_path(), &config.supported_platform_triples, )?; @@ -236,15 +236,15 @@ pub fn vendor(opt: VendorOptions) -> anyhow::Result<()> { &cargo, &cargo_lockfile, resolver_data, - manifest_path.as_path_buf(), - manifest_path.as_path_buf(), + manifest_path.cargo_toml_path(), + manifest_path.cargo_toml_path(), )?; // Write metadata to the workspace for future reuse let (cargo_metadata, cargo_lockfile) = Generator::new() .with_cargo(cargo.clone()) .with_rustc(opt.rustc.clone()) - .generate(manifest_path.as_path_buf())?; + .generate(manifest_path.cargo_toml_path())?; // Annotate metadata let annotations = Annotations::new( @@ -281,7 +281,7 @@ pub fn vendor(opt: VendorOptions) -> anyhow::Result<()> { if matches!(config.rendering.vendor_mode, Some(VendorMode::Local)) { VendorGenerator::new(cargo, opt.rustc.clone()) - .generate(manifest_path.as_path_buf(), &vendor_dir) + .generate(manifest_path.cargo_toml_path(), &vendor_dir) .context("Failed to vendor dependencies")?; } diff --git a/crate_universe/src/metadata.rs b/crate_universe/src/metadata.rs index 7cb24b839c..dcfcd7cfb0 100644 --- a/crate_universe/src/metadata.rs +++ b/crate_universe/src/metadata.rs @@ -1,6 +1,7 @@ //! Tools for gathering various kinds of metadata (Cargo.lock, Cargo metadata, Crate Index info). mod cargo_bin; +mod cargo_toml_path; mod cargo_tree_resolver; mod dependency; mod metadata_annotation; @@ -12,12 +13,12 @@ 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; pub(crate) use self::cargo_bin::*; +pub(crate) use self::cargo_toml_path::*; pub(crate) use self::cargo_tree_resolver::*; pub(crate) use self::dependency::*; pub(crate) use self::metadata_annotation::*; @@ -190,13 +191,13 @@ impl LockGenerator { #[tracing::instrument(name = "LockGenerator::generate", skip_all)] pub(crate) fn generate( &self, - manifest_path: &Utf8Path, + manifest_path: &CargoTomlPath, existing_lock: &Option, update_request: &Option, ) -> Result { debug!("Generating Cargo Lockfile for {}", manifest_path); - let manifest_dir = manifest_path.parent().unwrap(); + let manifest_dir = manifest_path.parent(); let generated_lockfile_path = manifest_dir.join("Cargo.lock"); if let Some(lock) = existing_lock { @@ -304,9 +305,9 @@ impl VendorGenerator { } } #[tracing::instrument(name = "VendorGenerator::generate", skip_all)] - pub(crate) fn generate(&self, manifest_path: &Utf8Path, output_dir: &Path) -> Result<()> { + pub(crate) fn generate(&self, manifest_path: &CargoTomlPath, output_dir: &Path) -> Result<()> { debug!("Vendoring {} to {}", manifest_path, output_dir.display()); - let manifest_dir = manifest_path.parent().unwrap(); + let manifest_dir = manifest_path.parent(); // Simply invoke `cargo generate-lockfile` let output = self diff --git a/crate_universe/src/metadata/cargo_toml_path.rs b/crate_universe/src/metadata/cargo_toml_path.rs new file mode 100644 index 0000000000..43d1eb3957 --- /dev/null +++ b/crate_universe/src/metadata/cargo_toml_path.rs @@ -0,0 +1,60 @@ +use anyhow::{anyhow, Result}; +use camino::{Utf8Path, Utf8PathBuf}; +use serde::{Deserialize, Serialize}; +use std::fmt; +use std::fmt::Display; +use std::path::Path; + +/// Path that is know to end with `/Cargo.toml`. +#[derive(Eq, PartialEq, Debug, Clone, Hash, Ord, PartialOrd, Serialize, Deserialize)] +#[serde(transparent)] +pub(crate) struct CargoTomlPath(Utf8PathBuf); + +impl Display for CargoTomlPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + Display::fmt(&self.0, f) + } +} + +impl CargoTomlPath { + pub(crate) fn unchecked_new(path: impl Into) -> Self { + CargoTomlPath(path.into()) + } + + pub(crate) fn new(path: impl Into) -> Result { + let path = path.into(); + if !path.ends_with("Cargo.toml") { + return Err(anyhow!("Path does not end with Cargo.toml: {path}")); + } + Ok(Self::unchecked_new(path)) + } + + pub(crate) fn for_dir(dir: impl Into) -> Self { + let mut path = dir.into(); + path.push("Cargo.toml"); + CargoTomlPath::unchecked_new(path) + } + + /// Directory of `Cargo.toml` path. + pub(crate) fn parent(&self) -> &Utf8Path { + self.0.parent().expect("Validated in constructor") + } + + pub(crate) fn ancestors(&self) -> impl Iterator { + self.0.ancestors() + } + + pub(crate) fn as_path(&self) -> &Utf8Path { + self.0.as_path() + } + + pub(crate) fn as_std_path(&self) -> &Path { + self.0.as_std_path() + } +} + +impl AsRef for CargoTomlPath { + fn as_ref(&self) -> &Path { + self.0.as_ref() + } +} diff --git a/crate_universe/src/metadata/cargo_tree_resolver.rs b/crate_universe/src/metadata/cargo_tree_resolver.rs index 01ab6ad79c..e66623d211 100644 --- a/crate_universe/src/metadata/cargo_tree_resolver.rs +++ b/crate_universe/src/metadata/cargo_tree_resolver.rs @@ -6,7 +6,6 @@ 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, trace}; @@ -14,6 +13,7 @@ use url::Url; use crate::config::CrateId; use crate::metadata::cargo_bin::Cargo; +use crate::metadata::CargoTomlPath; use crate::select::{Select, SelectableScalar}; use crate::utils::symlink::symlink; use crate::utils::target_triple::TargetTriple; @@ -307,7 +307,7 @@ impl TreeResolver { #[tracing::instrument(name = "TreeResolver::generate", skip_all)] pub(crate) fn generate( &self, - pristine_manifest_path: &Utf8Path, + pristine_manifest_path: &CargoTomlPath, target_triples: &BTreeSet, ) -> Result { debug!( @@ -450,14 +450,14 @@ 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: &Utf8Path, + pristine_manifest_path: &CargoTomlPath, output_dir: &Path, ) -> Result { if !output_dir.exists() { std::fs::create_dir_all(output_dir)?; } - let pristine_root = pristine_manifest_path.parent().unwrap(); + let pristine_root = pristine_manifest_path.parent(); for file in std::fs::read_dir(pristine_root).context("Failed to read dir")? { let source_path = file?.path(); let file_name = source_path.file_name().unwrap(); diff --git a/crate_universe/src/metadata/workspace_discoverer.rs b/crate_universe/src/metadata/workspace_discoverer.rs index 8814f33b23..adc01b876c 100644 --- a/crate_universe/src/metadata/workspace_discoverer.rs +++ b/crate_universe/src/metadata/workspace_discoverer.rs @@ -1,23 +1,24 @@ use std::collections::{BTreeMap, BTreeSet}; +use crate::metadata::CargoTomlPath; use anyhow::{anyhow, bail, Context, Result}; -use camino::{Utf8Path, Utf8PathBuf}; +use camino::Utf8Path; 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, + workspaces_to_members: BTreeMap>, + non_workspaces: BTreeSet, } impl DiscoveredWorkspaces { - pub(crate) fn workspaces(&self) -> BTreeSet { + pub(crate) fn workspaces(&self) -> BTreeSet { self.workspaces_to_members.keys().cloned().collect() } - pub(crate) fn all_workspaces_and_members(&self) -> BTreeSet { + pub(crate) fn all_workspaces_and_members(&self) -> BTreeSet { self.workspaces_to_members .keys() .chain(self.workspaces_to_members.values().flatten()) @@ -27,8 +28,8 @@ impl DiscoveredWorkspaces { } pub(crate) fn discover_workspaces( - cargo_toml_paths: BTreeSet, - known_manifests: &BTreeMap, + cargo_toml_paths: BTreeSet, + known_manifests: &BTreeMap, ) -> Result { let mut manifest_cache = ManifestCache { cache: BTreeMap::new(), @@ -38,7 +39,7 @@ pub(crate) fn discover_workspaces( } fn discover_workspaces_with_cache( - cargo_toml_paths: BTreeSet, + cargo_toml_paths: BTreeSet, manifest_cache: &mut ManifestCache, ) -> Result { let mut discovered_workspaces = DiscoveredWorkspaces { @@ -85,7 +86,7 @@ fn discover_workspaces_with_cache( }) .transpose()?; - 'per_child: for entry in walkdir::WalkDir::new(workspace_path.parent().unwrap()) + 'per_child: for entry in walkdir::WalkDir::new(workspace_path.parent()) .follow_links(false) .follow_root_links(false) .into_iter() @@ -117,6 +118,7 @@ fn discover_workspaces_with_cache( let child_path = Utf8Path::from_path(entry.path()) .ok_or_else(|| anyhow!("Failed to parse {:?} as UTF-8", entry.path()))? .to_path_buf(); + let child_path = CargoTomlPath::unchecked_new(child_path); if child_path == workspace_path { continue; } @@ -136,7 +138,7 @@ fn discover_workspaces_with_cache( ) })?; actual_workspace_path = - child_path.parent().unwrap().join(explicit_workspace_path); + CargoTomlPath::new(child_path.parent().join(explicit_workspace_path))?; } } if !discovered_workspaces @@ -146,10 +148,8 @@ fn discover_workspaces_with_cache( 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); } - let dir_relative_to_workspace_dir = child_path - .parent() - .unwrap() - .strip_prefix(workspace_path.parent().unwrap()); + let dir_relative_to_workspace_dir = + child_path.parent().strip_prefix(workspace_path.parent()); if let Ok(dir_relative_to_workspace_dir) = dir_relative_to_workspace_dir { use itertools::Itertools; @@ -201,11 +201,11 @@ fn discover_workspaces_with_cache( } fn discover_workspace_parent( - cargo_toml_path: &Utf8PathBuf, + cargo_toml_path: &CargoTomlPath, manifest_cache: &mut ManifestCache, -) -> Option { +) -> Option { for parent_dir in cargo_toml_path.ancestors().skip(1) { - let maybe_cargo_toml_path = parent_dir.join("Cargo.toml"); + let maybe_cargo_toml_path = CargoTomlPath::for_dir(parent_dir); let maybe_manifest = manifest_cache.get(&maybe_cargo_toml_path); if let Some(manifest) = maybe_manifest { if manifest.workspace.is_some() { @@ -217,12 +217,12 @@ fn discover_workspace_parent( } struct ManifestCache<'a> { - cache: BTreeMap>, - known_manifests: &'a BTreeMap, + cache: BTreeMap>, + known_manifests: &'a BTreeMap, } impl ManifestCache<'_> { - fn get(&mut self, path: &Utf8PathBuf) -> Option { + fn get(&mut self, path: &CargoTomlPath) -> Option { if let Some(manifest) = self.known_manifests.get(path) { return Some(manifest.clone()); } @@ -240,6 +240,7 @@ mod test { use super::*; use std::path::{Path, PathBuf}; use std::sync::Mutex; + use camino::Utf8PathBuf; // Both of these tests try to create the same symlink, so they can't run in parallel. static FILESYSTEM_GUARD: Mutex<()> = Mutex::new(()); @@ -262,28 +263,27 @@ mod test { let mut expected = ws1_discovered_workspaces(&root_dir); expected.workspaces_to_members.insert( - root_dir.join("ws2").join("Cargo.toml"), + CargoTomlPath::for_dir(root_dir.join("ws2")), BTreeSet::from([ - root_dir.join("ws2").join("ws2c1").join("Cargo.toml"), - root_dir + CargoTomlPath::for_dir(root_dir.join("ws2").join("ws2c1")), + CargoTomlPath::for_dir(root_dir .join("ws2") .join("ws2excluded") - .join("ws2included") - .join("Cargo.toml"), + .join("ws2included")), ]), ); expected.non_workspaces.extend([ - root_dir.join("non-ws").join("Cargo.toml"), - root_dir.join("ws2").join("ws2excluded").join("Cargo.toml"), + CargoTomlPath::for_dir(root_dir.join("non-ws")), + CargoTomlPath::for_dir(root_dir.join("ws2").join("ws2excluded")), ]); let actual = discover_workspaces( vec![ - root_dir.join("ws1").join("ws1c1").join("Cargo.toml"), - root_dir.join("ws2").join("Cargo.toml"), - root_dir.join("ws2").join("ws2excluded").join("Cargo.toml"), - root_dir.join("non-ws").join("Cargo.toml"), + CargoTomlPath::for_dir(root_dir.join("ws1").join("ws1c1")), + CargoTomlPath::for_dir(root_dir.join("ws2")), + CargoTomlPath::for_dir(root_dir.join("ws2").join("ws2excluded")), + CargoTomlPath::for_dir(root_dir.join("non-ws")), ] .into_iter() .collect(), @@ -318,7 +318,7 @@ mod test { let expected = ws1_discovered_workspaces(&root_dir); let actual = discover_workspaces( - vec![root_dir.join("ws1").join("ws1c1").join("Cargo.toml")] + vec![CargoTomlPath::for_dir(root_dir.join("ws1").join("ws1c1"))] .into_iter() .collect(), &BTreeMap::new(), @@ -346,7 +346,7 @@ mod test { let expected = ws1_discovered_workspaces(&root_dir); let actual = discover_workspaces( - vec![root_dir.join("ws1").join("ws1c1").join("Cargo.toml")] + vec![CargoTomlPath::for_dir(root_dir.join("ws1").join("ws1c1"))] .into_iter() .collect(), &BTreeMap::new(), @@ -364,7 +364,7 @@ mod test { .unwrap(); let root_dir = Utf8PathBuf::from_path_buf(root_dir).unwrap(); - let mut workspaces_to_members = BTreeMap::>::new(); + let mut workspaces_to_members = BTreeMap::>::new(); let mut includes_members = BTreeSet::new(); let includes_root = root_dir.join("includes"); @@ -388,12 +388,11 @@ mod test { for path_part in child { path.push(path_part); } - path.push("Cargo.toml"); - includes_members.insert(path); + includes_members.insert(CargoTomlPath::for_dir(path)); } - workspaces_to_members.insert(includes_root.join("Cargo.toml"), includes_members); - let non_workspaces = BTreeSet::::new(); + workspaces_to_members.insert(CargoTomlPath::for_dir(includes_root), includes_members); + let non_workspaces = BTreeSet::::new(); let expected = DiscoveredWorkspaces { workspaces_to_members, @@ -401,10 +400,9 @@ mod test { }; let actual = discover_workspaces( - vec![root_dir + vec![CargoTomlPath::for_dir(root_dir .join("includes") - .join("explicit-child") - .join("Cargo.toml")] + .join("explicit-child"))] .into_iter() .collect(), &BTreeMap::new(), @@ -417,15 +415,14 @@ mod test { fn ws1_discovered_workspaces(root_dir: &Utf8Path) -> DiscoveredWorkspaces { let mut workspaces_to_members = BTreeMap::new(); workspaces_to_members.insert( - root_dir.join("ws1").join("Cargo.toml"), + CargoTomlPath::for_dir(root_dir.join("ws1")), BTreeSet::from([ - root_dir.join("ws1").join("ws1c1").join("Cargo.toml"), - root_dir + CargoTomlPath::for_dir(root_dir.join("ws1").join("ws1c1")), + CargoTomlPath::for_dir(root_dir .join("ws1") .join("ws1c1") - .join("ws1c1c1") - .join("Cargo.toml"), - root_dir.join("ws1").join("ws1c2").join("Cargo.toml"), + .join("ws1c1c1")), + CargoTomlPath::for_dir(root_dir.join("ws1").join("ws1c2")), ]), ); let non_workspaces = BTreeSet::new(); diff --git a/crate_universe/src/splicing.rs b/crate_universe/src/splicing.rs index 4417386bea..32a060b4bd 100644 --- a/crate_universe/src/splicing.rs +++ b/crate_universe/src/splicing.rs @@ -10,13 +10,15 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use anyhow::{anyhow, bail, Context, Result}; -use camino::{Utf8Path, Utf8PathBuf}; +use camino::Utf8PathBuf; use cargo_lock::package::SourceKind; use cargo_toml::Manifest; use serde::{Deserialize, Serialize}; use crate::config::CrateId; -use crate::metadata::{Cargo, CargoUpdateRequest, LockGenerator, TreeResolverMetadata}; +use crate::metadata::{ + Cargo, CargoTomlPath, CargoUpdateRequest, LockGenerator, TreeResolverMetadata, +}; use crate::utils; use crate::utils::starlark::Label; @@ -34,7 +36,7 @@ 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, @@ -75,7 +77,10 @@ impl SplicingManifest { .to_string() .replace("${build_workspace_directory}", &workspace_dir_str) .replace("${output_base}", &output_base_str); - (Utf8PathBuf::from(resolved_path), label) + ( + CargoTomlPath::unchecked_new(Utf8PathBuf::from(resolved_path)), + label, + ) }) .collect(); @@ -207,7 +212,7 @@ impl TryFrom for WorkspaceMetadata { impl WorkspaceMetadata { fn new( splicing_manifest: &SplicingManifest, - member_manifests: BTreeMap<&Utf8PathBuf, String>, + member_manifests: BTreeMap<&CargoTomlPath, String>, ) -> Result { let mut package_prefixes: BTreeMap = member_manifests .iter() @@ -258,8 +263,8 @@ impl WorkspaceMetadata { cargo: &Cargo, lockfile: &cargo_lock::Lockfile, resolver_data: TreeResolverMetadata, - input_manifest_path: &Utf8Path, - output_manifest_path: &Utf8Path, + input_manifest_path: &CargoTomlPath, + output_manifest_path: &CargoTomlPath, ) -> Result<()> { let mut manifest = read_manifest(input_manifest_path)?; @@ -296,7 +301,6 @@ impl WorkspaceMetadata { // Note that this path must match the one defined in `splicing::setup_cargo_config` let config_path = input_manifest_path .parent() - .unwrap() .join(".cargo") .join("config.toml"); @@ -439,13 +443,13 @@ impl WorkspaceMetadata { #[derive(Debug)] pub(crate) enum SplicedManifest { - Workspace(Utf8PathBuf), - Package(Utf8PathBuf), - MultiPackage(Utf8PathBuf), + Workspace(CargoTomlPath), + Package(CargoTomlPath), + MultiPackage(CargoTomlPath), } impl SplicedManifest { - pub(crate) fn as_path_buf(&self) -> &Utf8PathBuf { + pub(crate) fn cargo_toml_path(&self) -> &CargoTomlPath { match self { SplicedManifest::Workspace(p) => p, SplicedManifest::Package(p) => p, @@ -454,7 +458,7 @@ impl SplicedManifest { } } -pub(crate) fn read_manifest(manifest: &Utf8Path) -> Result { +pub(crate) fn read_manifest(manifest: &CargoTomlPath) -> Result { let content = fs::read_to_string(manifest.as_std_path())?; cargo_toml::Manifest::from_str(content.as_str()).context("Failed to deserialize manifest") } @@ -465,10 +469,7 @@ pub(crate) fn generate_lockfile( cargo_bin: Cargo, update_request: &Option, ) -> Result { - let manifest_dir = manifest_path - .as_path_buf() - .parent() - .expect("Every manifest should be contained in a parent directory"); + let manifest_dir = manifest_path.cargo_toml_path().parent(); let root_lockfile_path = manifest_dir.join("Cargo.lock"); @@ -479,7 +480,7 @@ pub(crate) fn generate_lockfile( // Generate the new lockfile let lockfile = LockGenerator::new(cargo_bin).generate( - manifest_path.as_path_buf(), + manifest_path.cargo_toml_path(), existing_lock, update_request, )?; @@ -514,15 +515,15 @@ mod test { manifest.manifests, BTreeMap::from([ ( - Utf8PathBuf::from("${build_workspace_directory}/submod/Cargo.toml"), + CargoTomlPath::unchecked_new("${build_workspace_directory}/submod/Cargo.toml"), Label::from_str("//submod:Cargo.toml").unwrap() ), ( - Utf8PathBuf::from("${output_base}/external_crate/Cargo.toml"), + CargoTomlPath::unchecked_new("${output_base}/external_crate/Cargo.toml"), Label::from_str("@external_crate//:Cargo.toml").unwrap() ), ( - Utf8PathBuf::from("/tmp/abs/path/workspace/Cargo.toml"), + CargoTomlPath::unchecked_new("/tmp/abs/path/workspace/Cargo.toml"), Label::from_str("//:Cargo.toml").unwrap() ), ]) @@ -608,15 +609,15 @@ mod test { manifest.manifests, BTreeMap::from([ ( - Utf8PathBuf::from("/tmp/abs/path/workspace/submod/Cargo.toml"), + CargoTomlPath::unchecked_new("/tmp/abs/path/workspace/submod/Cargo.toml"), Label::from_str("//submod:Cargo.toml").unwrap() ), ( - Utf8PathBuf::from("/tmp/output_base/external_crate/Cargo.toml"), + CargoTomlPath::unchecked_new("/tmp/output_base/external_crate/Cargo.toml"), Label::from_str("@external_crate//:Cargo.toml").unwrap() ), ( - Utf8PathBuf::from("/tmp/abs/path/workspace/Cargo.toml"), + CargoTomlPath::unchecked_new("/tmp/abs/path/workspace/Cargo.toml"), Label::from_str("//:Cargo.toml").unwrap() ), ]) @@ -652,15 +653,15 @@ mod test { direct_packages: BTreeMap::new(), manifests: BTreeMap::from([ ( - Utf8PathBuf::try_from(workspace_manifest_path).unwrap(), + CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(workspace_manifest_path).unwrap()), Label::from_str("//:Cargo.toml").unwrap(), ), ( - Utf8PathBuf::try_from(child_a_manifest_path).unwrap(), + CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(child_a_manifest_path).unwrap()), Label::from_str("//child_a:Cargo.toml").unwrap(), ), ( - Utf8PathBuf::try_from(child_b_manifest_path).unwrap(), + CargoTomlPath::unchecked_new(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 11b7fd088b..4ce072f06f 100644 --- a/crate_universe/src/splicing/splicer.rs +++ b/crate_universe/src/splicing/splicer.rs @@ -10,7 +10,7 @@ use cargo_toml::Manifest; use tracing::debug; use crate::config::CrateId; -use crate::metadata::discover_workspaces; +use crate::metadata::{discover_workspaces, CargoTomlPath}; use crate::splicing::{SplicedManifest, SplicingManifest}; use crate::utils::starlark::Label; use crate::utils::symlink::{remove_symlink, symlink}; @@ -22,20 +22,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 Utf8PathBuf, + path: &'a CargoTomlPath, 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 Utf8PathBuf, + path: &'a CargoTomlPath, 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, }, } @@ -45,7 +45,7 @@ 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, ) -> Result { let workspaces = discover_workspaces(manifests.keys().cloned().collect(), manifests)?; @@ -139,14 +139,12 @@ impl<'a> SplicerKind<'a> { #[tracing::instrument(skip_all)] fn splice_workspace( workspace_dir: &Utf8Path, - path: &&Utf8PathBuf, + path: &&CargoTomlPath, manifest: &&Manifest, splicing_manifest: &&SplicingManifest, ) -> Result { let mut manifest = (*manifest).clone(); - let manifest_dir = path - .parent() - .expect("Every manifest should have a parent directory"); + let manifest_dir = path.parent(); // Link the sources of the root manifest into the new workspace symlink_roots( @@ -163,7 +161,7 @@ impl<'a> SplicerKind<'a> { Self::inject_direct_packages(&mut manifest, &splicing_manifest.direct_packages)?; } - let root_manifest_path = workspace_dir.join("Cargo.toml"); + let root_manifest_path = CargoTomlPath::for_dir(workspace_dir); let member_manifests = BTreeMap::from([(*path, String::new())]); // Write the generated metadata to the manifest @@ -180,13 +178,11 @@ impl<'a> SplicerKind<'a> { #[tracing::instrument(skip_all)] fn splice_package( workspace_dir: &Utf8Path, - path: &&Utf8PathBuf, + path: &&CargoTomlPath, manifest: &&Manifest, splicing_manifest: &&SplicingManifest, ) -> Result { - let manifest_dir = path - .parent() - .expect("Every manifest should have a parent directory"); + let manifest_dir = path.parent(); // Link the sources of the root manifest into the new workspace symlink_roots( @@ -210,7 +206,7 @@ impl<'a> SplicerKind<'a> { Self::inject_direct_packages(&mut manifest, &splicing_manifest.direct_packages)?; } - let root_manifest_path = workspace_dir.join("Cargo.toml"); + let root_manifest_path = CargoTomlPath::for_dir(workspace_dir); let member_manifests = BTreeMap::from([(*path, String::new())]); // Write the generated metadata to the manifest @@ -227,7 +223,7 @@ impl<'a> SplicerKind<'a> { #[tracing::instrument(skip_all)] fn splice_multi_package( workspace_dir: &Utf8Path, - manifests: &&BTreeMap, + manifests: &&BTreeMap, splicing_manifest: &&SplicingManifest, ) -> Result { let mut manifest = default_cargo_workspace_manifest(&splicing_manifest.resolver_version); @@ -261,7 +257,7 @@ impl<'a> SplicerKind<'a> { } // Write the root manifest - let root_manifest_path = workspace_dir.join("Cargo.toml"); + let root_manifest_path = CargoTomlPath::for_dir(workspace_dir); write_root_manifest(root_manifest_path.as_std_path(), manifest)?; Ok(SplicedManifest::MultiPackage(root_manifest_path)) @@ -361,9 +357,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)| { @@ -380,9 +376,7 @@ impl<'a> SplicerKind<'a> { .members .push(package_name.clone()); - let manifest_dir = path - .parent() - .expect("Every manifest should have a parent directory"); + let manifest_dir = path.parent(); let dest_package_dir = workspace_dir.join(package_name); @@ -469,7 +463,7 @@ impl<'a> SplicerKind<'a> { pub(crate) struct Splicer { workspace_dir: Utf8PathBuf, - manifests: BTreeMap, + manifests: BTreeMap, splicing_manifest: SplicingManifest, } @@ -487,7 +481,7 @@ impl Splicer { .with_context(|| format!("Failed to read manifest at {}", path))?; Ok((path.clone(), m)) }) - .collect::>>()?; + .collect::>>()?; Ok(Self { workspace_dir, @@ -771,14 +765,14 @@ mod test { // Write workspace members for pkg in &["sub_pkg_a", "sub_pkg_b"] { - let manifest_path = Utf8PathBuf::try_from( + let manifest_path = CargoTomlPath::unchecked_new(Utf8PathBuf::try_from( cache_dir .as_ref() .join("root_pkg") .join(pkg) .join("Cargo.toml"), ) - .unwrap(); + .unwrap()); let deps = if pkg == &"sub_pkg_b" { vec![r#"sub_pkg_a = { path = "../sub_pkg_a" }"#] } else { @@ -814,8 +808,8 @@ mod test { File::create(workspace_root.join("WORKSPACE.bazel")).unwrap(); } let root_pkg = workspace_root.join("root_pkg"); - let manifest_path = Utf8PathBuf::try_from(root_pkg.join("Cargo.toml")).unwrap(); - fs::create_dir_all(manifest_path.parent().unwrap()).unwrap(); + let manifest_path = CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(root_pkg.join("Cargo.toml")).unwrap()); + fs::create_dir_all(manifest_path.parent()).unwrap(); fs::write(&manifest_path, toml::to_string(&manifest).unwrap()).unwrap(); { File::create(root_pkg.join("BUILD.bazel")).unwrap(); @@ -842,7 +836,7 @@ mod test { // Write workspace members for pkg in &["sub_pkg_a", "sub_pkg_b"] { let manifest_path = - Utf8PathBuf::try_from(cache_dir.as_ref().join(pkg).join("Cargo.toml")).unwrap(); + CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(cache_dir.as_ref().join(pkg).join("Cargo.toml")).unwrap()); mock_cargo_toml(&manifest_path, pkg); splicing_manifest.manifests.insert( @@ -872,8 +866,8 @@ mod test { { File::create(workspace_root.join("WORKSPACE.bazel")).unwrap(); } - let manifest_path = Utf8PathBuf::try_from(workspace_root.join("Cargo.toml")).unwrap(); - fs::create_dir_all(manifest_path.parent().unwrap()).unwrap(); + let manifest_path = CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(workspace_root.join("Cargo.toml")).unwrap()); + fs::create_dir_all(manifest_path.parent()).unwrap(); fs::write(&manifest_path, toml::to_string(&manifest).unwrap()).unwrap(); splicing_manifest @@ -895,7 +889,7 @@ mod test { // Add an additional package let manifest_path = - Utf8PathBuf::try_from(cache_dir.as_ref().join("root_pkg").join("Cargo.toml")).unwrap(); + CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(cache_dir.as_ref().join("root_pkg").join("Cargo.toml")).unwrap()); mock_cargo_toml(&manifest_path, "root_pkg"); splicing_manifest .manifests @@ -911,7 +905,7 @@ mod test { // Add an additional package for pkg in &["pkg_a", "pkg_b", "pkg_c"] { let manifest_path = - Utf8PathBuf::try_from(cache_dir.as_ref().join(pkg).join("Cargo.toml")).unwrap(); + CargoTomlPath::unchecked_new(Utf8PathBuf::try_from(cache_dir.as_ref().join(pkg).join("Cargo.toml")).unwrap()); mock_cargo_toml(&manifest_path, pkg); splicing_manifest .manifests @@ -976,7 +970,7 @@ mod test { let cargo = cargo(); // Ensure metadata is valid - let metadata = generate_metadata(workspace_manifest.as_path_buf()); + let metadata = generate_metadata(workspace_manifest.cargo_toml_path()); assert_sort_eq!( metadata.workspace_members, vec![ @@ -1019,7 +1013,7 @@ mod test { let cargo = cargo(); // Ensure metadata is valid - let metadata = generate_metadata(workspace_manifest.as_path_buf()); + let metadata = generate_metadata(workspace_manifest.cargo_toml_path()); assert_sort_eq!( metadata.workspace_members, vec![ @@ -1111,14 +1105,14 @@ mod test { // Add a new package from an existing external workspace let external_workspace_root = tempfile::tempdir().unwrap(); - let external_manifest = Utf8PathBuf::try_from( - external_workspace_root + let external_manifest = CargoTomlPath::new( + Utf8PathBuf::try_from(external_workspace_root .as_ref() .join("external_workspace_member") - .join("Cargo.toml"), + .join("Cargo.toml")).unwrap(), ) .unwrap(); - fs::create_dir_all(external_manifest.parent().unwrap()).unwrap(); + fs::create_dir_all(external_manifest.parent()).unwrap(); fs::write( external_workspace_root.as_ref().join("Cargo.toml"), @@ -1201,7 +1195,7 @@ mod test { .splice_workspace() .unwrap(); - let metadata = generate_metadata(workspace_manifest.as_path_buf()); + let metadata = generate_metadata(workspace_manifest.cargo_toml_path()); // Since no direct packages were added to the splicing manifest, the cargo_bazel // deps target should __not__ have been injected into the manifest. @@ -1230,7 +1224,7 @@ mod test { let cargo = cargo(); // Ensure metadata is valid - let metadata = generate_metadata(workspace_manifest.as_path_buf()); + let metadata = generate_metadata(workspace_manifest.cargo_toml_path()); assert_sort_eq!( metadata.workspace_members, vec![new_package_id( @@ -1265,7 +1259,7 @@ mod test { // Check the default resolver version let cargo_manifest = cargo_toml::Manifest::from_str( - &fs::read_to_string(workspace_manifest.as_path_buf()).unwrap(), + &fs::read_to_string(workspace_manifest.cargo_toml_path()).unwrap(), ) .unwrap(); assert!(cargo_manifest.workspace.is_some()); @@ -1278,7 +1272,7 @@ mod test { let cargo = cargo(); // Ensure metadata is valid - let metadata = generate_metadata(workspace_manifest.as_path_buf()); + let metadata = generate_metadata(workspace_manifest.cargo_toml_path()); assert_sort_eq!( metadata.workspace_members, vec![ @@ -1315,7 +1309,7 @@ mod test { // Check the specified resolver version let cargo_manifest = cargo_toml::Manifest::from_str( - &fs::read_to_string(workspace_manifest.as_path_buf()).unwrap(), + &fs::read_to_string(workspace_manifest.cargo_toml_path()).unwrap(), ) .unwrap(); assert!(cargo_manifest.workspace.is_some()); @@ -1328,7 +1322,7 @@ mod test { let cargo = cargo(); // Ensure metadata is valid - let metadata = generate_metadata(workspace_manifest.as_path_buf()); + let metadata = generate_metadata(workspace_manifest.cargo_toml_path()); assert_sort_eq!( metadata.workspace_members, vec![ @@ -1375,7 +1369,7 @@ mod test { // Check the default resolver version let cargo_manifest = cargo_toml::Manifest::from_str( - &fs::read_to_string(workspace_manifest.as_path_buf()).unwrap(), + &fs::read_to_string(workspace_manifest.cargo_toml_path()).unwrap(), ) .unwrap(); @@ -1417,7 +1411,7 @@ mod test { // Ensure the patches match the expected value let cargo_manifest = cargo_toml::Manifest::from_str( - &fs::read_to_string(workspace_manifest.as_path_buf()).unwrap(), + &fs::read_to_string(workspace_manifest.cargo_toml_path()).unwrap(), ) .unwrap(); assert_eq!(expected, cargo_manifest.patch); @@ -1480,7 +1474,7 @@ mod test { // Ensure the patches match the expected value let cargo_manifest = cargo_toml::Manifest::from_str( - &fs::read_to_string(workspace_manifest.as_path_buf()).unwrap(), + &fs::read_to_string(workspace_manifest.cargo_toml_path()).unwrap(), ) .unwrap(); assert_eq!(expected, cargo_manifest.patch); @@ -1531,7 +1525,7 @@ mod test { // Ensure the patches match the expected value let cargo_manifest = cargo_toml::Manifest::from_str( - &fs::read_to_string(workspace_manifest.as_path_buf()).unwrap(), + &fs::read_to_string(workspace_manifest.cargo_toml_path()).unwrap(), ) .unwrap(); assert_eq!(expected, cargo_manifest.patch);