From 610956aa92aa3925c1369fe1f61d1714b4bdc3ac Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Mar 2024 08:18:03 -0500 Subject: [PATCH] refactor(toml): Only collect nested paths when needed This simplifies the interface for `toml/mod.rs` and reduces the work we do. --- src/cargo/core/workspace.rs | 8 ++--- src/cargo/ops/cargo_package.rs | 3 +- src/cargo/ops/cargo_read_manifest.rs | 49 +++++++++++++++++++++++++--- src/cargo/sources/path.rs | 2 +- src/cargo/util/toml/mod.rs | 30 +++++++---------- 5 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 5291cbdbd4bf..e689933492ab 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -420,7 +420,6 @@ impl<'gctx> Workspace<'gctx> { let source = SourceId::for_path(self.root())?; let mut warnings = Vec::new(); - let mut nested_paths = Vec::new(); let mut patch = HashMap::new(); for (url, deps) in config_patch.into_iter().flatten() { @@ -442,7 +441,6 @@ impl<'gctx> Workspace<'gctx> { dep, name, source, - &mut nested_paths, self.gctx, &mut warnings, /* platform */ None, @@ -1063,7 +1061,7 @@ impl<'gctx> Workspace<'gctx> { return Ok(p); } let source_id = SourceId::for_path(manifest_path.parent().unwrap())?; - let (package, _nested_paths) = ops::read_package(manifest_path, source_id, self.gctx)?; + let package = ops::read_package(manifest_path, source_id, self.gctx)?; loaded.insert(manifest_path.to_path_buf(), package.clone()); Ok(package) } @@ -1596,7 +1594,7 @@ impl<'gctx> Packages<'gctx> { Entry::Occupied(e) => Ok(e.into_mut()), Entry::Vacant(v) => { let source_id = SourceId::for_path(key)?; - let (manifest, _nested_paths) = read_manifest(manifest_path, source_id, self.gctx)?; + let manifest = read_manifest(manifest_path, source_id, self.gctx)?; Ok(v.insert(match manifest { EitherManifest::Real(manifest) => { MaybePackage::Package(Package::new(manifest, manifest_path)) @@ -1746,7 +1744,7 @@ pub fn find_workspace_root( find_workspace_root_with_loader(manifest_path, gctx, |self_path| { let key = self_path.parent().unwrap(); let source_id = SourceId::for_path(key)?; - let (manifest, _nested_paths) = read_manifest(self_path, source_id, gctx)?; + let manifest = read_manifest(self_path, source_id, gctx)?; Ok(manifest .workspace_config() .get_ws_root(self_path, manifest_path)) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index a4070900df96..1bc8f4309f00 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -456,8 +456,7 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { let toml_manifest = prepare_for_publish(orig_pkg.manifest().resolved_toml(), ws, orig_pkg.root())?; let source_id = orig_pkg.package_id().source_id(); - let (manifest, _nested_paths) = - to_real_manifest(toml_manifest, source_id, orig_pkg.manifest_path(), gctx)?; + let manifest = to_real_manifest(toml_manifest, source_id, orig_pkg.manifest_path(), gctx)?; let new_pkg = Package::new(manifest, orig_pkg.manifest_path()); // Regenerate Cargo.lock using the old one as a guide. diff --git a/src/cargo/ops/cargo_read_manifest.rs b/src/cargo/ops/cargo_read_manifest.rs index 0cfc7bb2b00c..e8eaf1b43f9d 100644 --- a/src/cargo/ops/cargo_read_manifest.rs +++ b/src/cargo/ops/cargo_read_manifest.rs @@ -3,7 +3,7 @@ use std::fs; use std::io; use std::path::{Path, PathBuf}; -use crate::core::{EitherManifest, Package, PackageId, SourceId}; +use crate::core::{EitherManifest, Manifest, Package, PackageId, SourceId}; use crate::util::errors::CargoResult; use crate::util::important_paths::find_project_manifest_exact; use crate::util::toml::read_manifest; @@ -15,13 +15,13 @@ pub fn read_package( path: &Path, source_id: SourceId, gctx: &GlobalContext, -) -> CargoResult<(Package, Vec)> { +) -> CargoResult { trace!( "read_package; path={}; source-id={}", path.display(), source_id ); - let (manifest, nested) = read_manifest(path, source_id, gctx)?; + let manifest = read_manifest(path, source_id, gctx)?; let manifest = match manifest { EitherManifest::Real(manifest) => manifest, EitherManifest::Virtual(..) => anyhow::bail!( @@ -31,7 +31,7 @@ pub fn read_package( ), }; - Ok((Package::new(manifest, path), nested)) + Ok(Package::new(manifest, path)) } pub fn read_packages( @@ -107,6 +107,44 @@ pub fn read_packages( } } +fn nested_paths(manifest: &Manifest) -> Vec { + let mut nested_paths = Vec::new(); + let resolved = manifest.resolved_toml(); + let dependencies = resolved + .dependencies + .iter() + .chain(resolved.build_dependencies()) + .chain(resolved.dev_dependencies()) + .chain( + resolved + .target + .as_ref() + .into_iter() + .flat_map(|t| t.values()) + .flat_map(|t| { + t.dependencies + .iter() + .chain(t.build_dependencies()) + .chain(t.dev_dependencies()) + }), + ); + for dep_table in dependencies { + for dep in dep_table.values() { + let cargo_util_schemas::manifest::InheritableDependency::Value(dep) = dep else { + continue; + }; + let cargo_util_schemas::manifest::TomlDependency::Detailed(dep) = dep else { + continue; + }; + let Some(path) = dep.path.as_ref() else { + continue; + }; + nested_paths.push(PathBuf::from(path.as_str())); + } + } + nested_paths +} + fn walk(path: &Path, callback: &mut dyn FnMut(&Path) -> CargoResult) -> CargoResult<()> { if !callback(path)? { trace!("not processing {}", path.display()); @@ -151,7 +189,7 @@ fn read_nested_packages( let manifest_path = find_project_manifest_exact(path, "Cargo.toml")?; - let (manifest, nested) = match read_manifest(&manifest_path, source_id, gctx) { + let manifest = match read_manifest(&manifest_path, source_id, gctx) { Err(err) => { // Ignore malformed manifests found on git repositories // @@ -174,6 +212,7 @@ fn read_nested_packages( EitherManifest::Real(manifest) => manifest, EitherManifest::Virtual(..) => return Ok(()), }; + let nested = nested_paths(&manifest); let pkg = Package::new(manifest, &manifest_path); let pkg_id = pkg.package_id(); diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 415298eb7c7a..2e4d9042038c 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -110,7 +110,7 @@ impl<'gctx> PathSource<'gctx> { ops::read_packages(&self.path, self.source_id, self.gctx) } else { let path = self.path.join("Cargo.toml"); - let (pkg, _) = ops::read_package(&path, self.source_id, self.gctx)?; + let pkg = ops::read_package(&path, self.source_id, self.gctx)?; Ok(vec![pkg]) } } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 79571b814dbd..a6be13aec97e 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -47,7 +47,7 @@ pub fn read_manifest( path: &Path, source_id: SourceId, gctx: &GlobalContext, -) -> CargoResult<(EitherManifest, Vec)> { +) -> CargoResult { let contents = read_toml_string(path, gctx).map_err(|err| ManifestError::new(err, path.into()))?; let document = @@ -174,13 +174,13 @@ fn convert_toml( manifest_file: &Path, source_id: SourceId, gctx: &GlobalContext, -) -> CargoResult<(EitherManifest, Vec)> { +) -> CargoResult { return if manifest.package().is_some() { - let (manifest, paths) = to_real_manifest(manifest, source_id, manifest_file, gctx)?; - Ok((EitherManifest::Real(manifest), paths)) + let manifest = to_real_manifest(manifest, source_id, manifest_file, gctx)?; + Ok(EitherManifest::Real(manifest)) } else { - let (m, paths) = to_virtual_manifest(manifest, source_id, manifest_file, gctx)?; - Ok((EitherManifest::Virtual(m), paths)) + let manifest = to_virtual_manifest(manifest, source_id, manifest_file, gctx)?; + Ok(EitherManifest::Virtual(manifest)) }; } @@ -464,7 +464,7 @@ pub fn to_real_manifest( source_id: SourceId, manifest_file: &Path, gctx: &GlobalContext, -) -> CargoResult<(Manifest, Vec)> { +) -> CargoResult { fn get_ws( gctx: &GlobalContext, resolved_path: &Path, @@ -516,7 +516,6 @@ pub fn to_real_manifest( } } - let mut nested_paths = vec![]; let mut warnings = vec![]; let mut errors = vec![]; @@ -770,7 +769,6 @@ pub fn to_real_manifest( let mut manifest_ctx = ManifestContext { deps: &mut deps, source_id, - nested_paths: &mut nested_paths, gctx, warnings: &mut warnings, features: &features, @@ -1272,7 +1270,7 @@ pub fn to_real_manifest( manifest.feature_gate()?; - Ok((manifest, nested_paths)) + Ok(manifest) } fn to_virtual_manifest( @@ -1280,7 +1278,7 @@ fn to_virtual_manifest( source_id: SourceId, manifest_file: &Path, gctx: &GlobalContext, -) -> CargoResult<(VirtualManifest, Vec)> { +) -> CargoResult { let root = manifest_file.parent().unwrap(); if let Some(deps) = me @@ -1302,7 +1300,6 @@ fn to_virtual_manifest( bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); } - let mut nested_paths = Vec::new(); let mut warnings = Vec::new(); let mut deps = Vec::new(); let empty = Vec::new(); @@ -1315,7 +1312,6 @@ fn to_virtual_manifest( let mut manifest_ctx = ManifestContext { deps: &mut deps, source_id, - nested_paths: &mut nested_paths, gctx, warnings: &mut warnings, platform: None, @@ -1376,7 +1372,7 @@ fn to_virtual_manifest( manifest.warnings_mut().add_warning(warning); } - Ok((manifest, nested_paths)) + Ok(manifest) } fn replace( @@ -1467,7 +1463,6 @@ fn patch( struct ManifestContext<'a, 'b> { deps: &'a mut Vec, source_id: SourceId, - nested_paths: &'a mut Vec, gctx: &'b GlobalContext, warnings: &'a mut Vec, platform: Option, @@ -1563,7 +1558,7 @@ fn inheritable_from_path( }; let source_id = SourceId::for_path(workspace_path_root)?; - let (man, _) = read_manifest(&workspace_path, source_id, gctx)?; + let man = read_manifest(&workspace_path, source_id, gctx)?; match man.workspace_config() { WorkspaceConfig::Root(root) => { gctx.ws_roots @@ -1885,7 +1880,6 @@ pub(crate) fn to_dependency( dep: &manifest::TomlDependency

, name: &str, source_id: SourceId, - nested_paths: &mut Vec, gctx: &GlobalContext, warnings: &mut Vec, platform: Option, @@ -1899,7 +1893,6 @@ pub(crate) fn to_dependency( &mut ManifestContext { deps: &mut Vec::new(), source_id, - nested_paths, gctx, warnings, platform, @@ -2068,7 +2061,6 @@ fn detailed_dep_to_dependency( } (None, Some(path), _, _) => { let path = path.resolve(manifest_ctx.gctx); - manifest_ctx.nested_paths.push(path.clone()); // If the source ID for the package we're parsing is a path // source, then we normalize the path here to get rid of // components like `..`.