Skip to content

Commit

Permalink
refactor(toml): Only collect nested paths when needed
Browse files Browse the repository at this point in the history
This simplifies the interface for `toml/mod.rs` and reduces the work we
do.
  • Loading branch information
epage committed Mar 15, 2024
1 parent ae955c9 commit 716f1a7
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 32 deletions.
8 changes: 3 additions & 5 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -442,7 +441,6 @@ impl<'gctx> Workspace<'gctx> {
dep,
name,
source,
&mut nested_paths,
self.gctx,
&mut warnings,
/* platform */ None,
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down
3 changes: 1 addition & 2 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,7 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult<String> {
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.
Expand Down
49 changes: 44 additions & 5 deletions src/cargo/ops/cargo_read_manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -15,13 +15,13 @@ pub fn read_package(
path: &Path,
source_id: SourceId,
gctx: &GlobalContext,
) -> CargoResult<(Package, Vec<PathBuf>)> {
) -> CargoResult<Package> {
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!(
Expand All @@ -31,7 +31,7 @@ pub fn read_package(
),
};

Ok((Package::new(manifest, path), nested))
Ok(Package::new(manifest, path))
}

pub fn read_packages(
Expand Down Expand Up @@ -107,6 +107,44 @@ pub fn read_packages(
}
}

fn nested_paths(manifest: &Manifest) -> Vec<PathBuf> {
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<bool>) -> CargoResult<()> {
if !callback(path)? {
trace!("not processing {}", path.display());
Expand Down Expand Up @@ -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
//
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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])
}
}
Expand Down
30 changes: 11 additions & 19 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub fn read_manifest(
path: &Path,
source_id: SourceId,
gctx: &GlobalContext,
) -> CargoResult<(EitherManifest, Vec<PathBuf>)> {
) -> CargoResult<EitherManifest> {
let contents =
read_toml_string(path, gctx).map_err(|err| ManifestError::new(err, path.into()))?;
let document =
Expand Down Expand Up @@ -174,13 +174,13 @@ fn convert_toml(
manifest_file: &Path,
source_id: SourceId,
gctx: &GlobalContext,
) -> CargoResult<(EitherManifest, Vec<PathBuf>)> {
) -> CargoResult<EitherManifest> {
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))
};
}

Expand Down Expand Up @@ -464,7 +464,7 @@ pub fn to_real_manifest(
source_id: SourceId,
manifest_file: &Path,
gctx: &GlobalContext,
) -> CargoResult<(Manifest, Vec<PathBuf>)> {
) -> CargoResult<Manifest> {
fn get_ws(
gctx: &GlobalContext,
resolved_path: &Path,
Expand Down Expand Up @@ -516,7 +516,6 @@ pub fn to_real_manifest(
}
}

let mut nested_paths = vec![];
let mut warnings = vec![];
let mut errors = vec![];

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1272,15 +1270,15 @@ pub fn to_real_manifest(

manifest.feature_gate()?;

Ok((manifest, nested_paths))
Ok(manifest)
}

fn to_virtual_manifest(
me: manifest::TomlManifest,
source_id: SourceId,
manifest_file: &Path,
gctx: &GlobalContext,
) -> CargoResult<(VirtualManifest, Vec<PathBuf>)> {
) -> CargoResult<VirtualManifest> {
let root = manifest_file.parent().unwrap();

if let Some(deps) = me
Expand All @@ -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();
Expand All @@ -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,
Expand Down Expand Up @@ -1376,7 +1372,7 @@ fn to_virtual_manifest(
manifest.warnings_mut().add_warning(warning);
}

Ok((manifest, nested_paths))
Ok(manifest)
}

fn replace(
Expand Down Expand Up @@ -1467,7 +1463,6 @@ fn patch(
struct ManifestContext<'a, 'b> {
deps: &'a mut Vec<Dependency>,
source_id: SourceId,
nested_paths: &'a mut Vec<PathBuf>,
gctx: &'b GlobalContext,
warnings: &'a mut Vec<String>,
platform: Option<Platform>,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1885,7 +1880,6 @@ pub(crate) fn to_dependency<P: ResolveToPath + Clone>(
dep: &manifest::TomlDependency<P>,
name: &str,
source_id: SourceId,
nested_paths: &mut Vec<PathBuf>,
gctx: &GlobalContext,
warnings: &mut Vec<String>,
platform: Option<Platform>,
Expand All @@ -1899,7 +1893,6 @@ pub(crate) fn to_dependency<P: ResolveToPath + Clone>(
&mut ManifestContext {
deps: &mut Vec::new(),
source_id,
nested_paths,
gctx,
warnings,
platform,
Expand Down Expand Up @@ -2068,7 +2061,6 @@ fn detailed_dep_to_dependency<P: ResolveToPath + Clone>(
}
(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 `..`.
Expand Down

0 comments on commit 716f1a7

Please sign in to comment.