From f3adda2806fbacb8d736fc636dbf5ace3e0acfac Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 3 Jun 2024 15:16:18 -0500 Subject: [PATCH 1/8] refactor(path): Clarify the relationship between read_packages and update --- src/cargo/sources/path.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index aa04f6719c6..7ac1513e64e 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -264,10 +264,14 @@ impl<'gctx> RecursivePathSource<'gctx> { if self.updated { Ok(self.packages.clone()) } else { - ops::read_packages(&self.path, self.source_id, self.gctx) + self.read_packages_inner() } } + fn read_packages_inner(&self) -> CargoResult> { + ops::read_packages(&self.path, self.source_id, self.gctx) + } + /// List all files relevant to building this package inside this source. /// /// This function will use the appropriate methods to determine the @@ -301,7 +305,7 @@ impl<'gctx> RecursivePathSource<'gctx> { /// Discovers packages inside this source if it hasn't yet done. pub fn update(&mut self) -> CargoResult<()> { if !self.updated { - let packages = self.read_packages()?; + let packages = self.read_packages_inner()?; self.packages.extend(packages.into_iter()); self.updated = true; } From d985f435223c0d0d826f5a57df0b7324dd28735f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 3 Jun 2024 15:18:51 -0500 Subject: [PATCH 2/8] refactor(path): Clarify we're replaces existing packages --- src/cargo/sources/path.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 7ac1513e64e..947091013cd 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -305,8 +305,7 @@ impl<'gctx> RecursivePathSource<'gctx> { /// Discovers packages inside this source if it hasn't yet done. pub fn update(&mut self) -> CargoResult<()> { if !self.updated { - let packages = self.read_packages_inner()?; - self.packages.extend(packages.into_iter()); + self.packages = self.read_packages_inner()?; self.updated = true; } From de0f695d90e61365295f64c053f33f4076e8c5e9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 29 Jun 2024 21:09:44 -0400 Subject: [PATCH 3/8] refactor(source): Rename PathSource::update to load This better matches the semantics (its one-time rather than forcing a new update) and is consistent with other parts (`preload_with`) --- src/cargo/ops/cargo_package.rs | 2 +- src/cargo/ops/resolve.rs | 2 +- src/cargo/sources/directory.rs | 2 +- src/cargo/sources/git/source.rs | 2 +- src/cargo/sources/path.rs | 56 +++++++++++++++---------------- src/cargo/sources/registry/mod.rs | 2 +- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index e5821867f8a..b32f2ed20d0 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -224,7 +224,7 @@ fn prepare_archive( ) -> CargoResult> { let gctx = ws.gctx(); let mut src = PathSource::new(pkg.root(), pkg.package_id().source_id(), gctx); - src.update()?; + src.load()?; if opts.check_metadata { check_metadata(pkg, gctx)?; diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index bfa10496d46..e725211594e 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -460,7 +460,7 @@ pub fn add_overrides<'a>( for (path, definition) in paths { let id = SourceId::for_path(&path)?; let mut source = RecursivePathSource::new(&path, id, ws.gctx()); - source.update().with_context(|| { + source.load().with_context(|| { format!( "failed to update path override `{}` \ (defined in `{}`)", diff --git a/src/cargo/sources/directory.rs b/src/cargo/sources/directory.rs index 33c0f8d97a0..6fff8ed2763 100644 --- a/src/cargo/sources/directory.rs +++ b/src/cargo/sources/directory.rs @@ -175,7 +175,7 @@ impl<'gctx> Source for DirectorySource<'gctx> { } let mut src = PathSource::new(&path, self.source_id, self.gctx); - src.update()?; + src.load()?; let mut pkg = src.root_package()?; let cksum_file = path.join(".cargo-checksum.json"); diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index f38c880f695..1209d43f005 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -361,7 +361,7 @@ impl<'gctx> Source for GitSource<'gctx> { self.path_source = Some(path_source); self.short_id = Some(short_id.as_str().into()); self.locked_rev = Revision::Locked(actual_rev); - self.path_source.as_mut().unwrap().update()?; + self.path_source.as_mut().unwrap().load()?; // Hopefully this shouldn't incur too much of a performance hit since // most of this should already be in cache since it was just diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 947091013cd..da506bef4df 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -29,8 +29,8 @@ pub struct PathSource<'gctx> { source_id: SourceId, /// The root path of this source. path: PathBuf, - /// Whether this source has updated all package information it may contain. - updated: bool, + /// Whether this source has loaded all package information it may contain. + loaded: bool, /// Packages that this sources has discovered. package: Option, gctx: &'gctx GlobalContext, @@ -45,7 +45,7 @@ impl<'gctx> PathSource<'gctx> { Self { source_id, path: path.to_path_buf(), - updated: false, + loaded: false, package: None, gctx, } @@ -59,7 +59,7 @@ impl<'gctx> PathSource<'gctx> { Self { source_id, path, - updated: true, + loaded: true, package: Some(pkg), gctx, } @@ -69,7 +69,7 @@ impl<'gctx> PathSource<'gctx> { pub fn root_package(&mut self) -> CargoResult { trace!("root_package; source={:?}", self); - self.update()?; + self.load()?; match &self.package { Some(pkg) => Ok(pkg.clone()), @@ -81,9 +81,9 @@ impl<'gctx> PathSource<'gctx> { } /// Returns the packages discovered by this source. It may walk the - /// filesystem if package information haven't yet updated. + /// filesystem if package information haven't yet loaded. pub fn read_packages(&self) -> CargoResult> { - if self.updated { + if self.loaded { Ok(self.package.clone().into_iter().collect()) } else { let pkg = self.read_package()?; @@ -113,9 +113,9 @@ impl<'gctx> PathSource<'gctx> { /// Gets the last modified file in a package. pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { - if !self.updated { + if !self.loaded { return Err(internal(format!( - "BUG: source `{:?}` was not updated", + "BUG: source `{:?}` was not loaded", self.path ))); } @@ -128,10 +128,10 @@ impl<'gctx> PathSource<'gctx> { } /// Discovers packages inside this source if it hasn't yet done. - pub fn update(&mut self) -> CargoResult<()> { - if !self.updated { + pub fn load(&mut self) -> CargoResult<()> { + if !self.loaded { self.package = Some(self.read_package()?); - self.updated = true; + self.loaded = true; } Ok(()) @@ -151,7 +151,7 @@ impl<'gctx> Source for PathSource<'gctx> { kind: QueryKind, f: &mut dyn FnMut(IndexSummary), ) -> Poll> { - self.update()?; + self.load()?; if let Some(s) = self.package.as_ref().map(|p| p.summary()) { let matched = match kind { QueryKind::Exact => dep.matches(s), @@ -179,7 +179,7 @@ impl<'gctx> Source for PathSource<'gctx> { fn download(&mut self, id: PackageId) -> CargoResult { trace!("getting packages; id={}", id); - self.update()?; + self.load()?; let pkg = self.package.iter().find(|pkg| pkg.package_id() == id); pkg.cloned() .map(MaybePackage::Ready) @@ -213,7 +213,7 @@ impl<'gctx> Source for PathSource<'gctx> { } fn block_until_ready(&mut self) -> CargoResult<()> { - self.update() + self.load() } fn invalidate_cache(&mut self) { @@ -232,8 +232,8 @@ pub struct RecursivePathSource<'gctx> { source_id: SourceId, /// The root path of this source. path: PathBuf, - /// Whether this source has updated all package information it may contain. - updated: bool, + /// Whether this source has loaded all package information it may contain. + loaded: bool, /// Packages that this sources has discovered. packages: Vec, gctx: &'gctx GlobalContext, @@ -252,16 +252,16 @@ impl<'gctx> RecursivePathSource<'gctx> { Self { source_id, path: root.to_path_buf(), - updated: false, + loaded: false, packages: Vec::new(), gctx, } } /// Returns the packages discovered by this source. It may walk the - /// filesystem if package information haven't yet updated. + /// filesystem if package information haven't yet loaded. pub fn read_packages(&self) -> CargoResult> { - if self.updated { + if self.loaded { Ok(self.packages.clone()) } else { self.read_packages_inner() @@ -288,9 +288,9 @@ impl<'gctx> RecursivePathSource<'gctx> { /// Gets the last modified file in a package. pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { - if !self.updated { + if !self.loaded { return Err(internal(format!( - "BUG: source `{:?}` was not updated", + "BUG: source `{:?}` was not loaded", self.path ))); } @@ -303,10 +303,10 @@ impl<'gctx> RecursivePathSource<'gctx> { } /// Discovers packages inside this source if it hasn't yet done. - pub fn update(&mut self) -> CargoResult<()> { - if !self.updated { + pub fn load(&mut self) -> CargoResult<()> { + if !self.loaded { self.packages = self.read_packages_inner()?; - self.updated = true; + self.loaded = true; } Ok(()) @@ -326,7 +326,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> { kind: QueryKind, f: &mut dyn FnMut(IndexSummary), ) -> Poll> { - self.update()?; + self.load()?; for s in self.packages.iter().map(|p| p.summary()) { let matched = match kind { QueryKind::Exact => dep.matches(s), @@ -354,7 +354,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> { fn download(&mut self, id: PackageId) -> CargoResult { trace!("getting packages; id={}", id); - self.update()?; + self.load()?; let pkg = self.packages.iter().find(|pkg| pkg.package_id() == id); pkg.cloned() .map(MaybePackage::Ready) @@ -388,7 +388,7 @@ impl<'gctx> Source for RecursivePathSource<'gctx> { } fn block_until_ready(&mut self) -> CargoResult<()> { - self.update() + self.load() } fn invalidate_cache(&mut self) { diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index ff852b134fb..6ef6f70be22 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -719,7 +719,7 @@ impl<'gctx> RegistrySource<'gctx> { .unpack_package(package, path) .with_context(|| format!("failed to unpack package `{}`", package))?; let mut src = PathSource::new(&path, self.source_id, self.gctx); - src.update()?; + src.load()?; let mut pkg = match src.download(package)? { MaybePackage::Ready(pkg) => pkg, MaybePackage::Download { .. } => unreachable!(), From 168feb853d6449fa7f819bc20d81020a0baa3a5b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 29 Jun 2024 21:17:28 -0400 Subject: [PATCH 4/8] refactor(add): Simplify use of PathSource With the split `PathSource` / `RecursivePathSource`, we can simplify this `cargo add` code. --- src/cargo/ops/cargo_add/mod.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/cargo/ops/cargo_add/mod.rs b/src/cargo/ops/cargo_add/mod.rs index a8fa97afe07..98757f595ad 100644 --- a/src/cargo/ops/cargo_add/mod.rs +++ b/src/cargo/ops/cargo_add/mod.rs @@ -349,11 +349,8 @@ fn resolve_dependency( } selected } else { - let source = crate::sources::PathSource::new(&path, src.source_id()?, gctx); - let package = source - .read_packages()? - .pop() - .expect("read_packages errors when no packages"); + let mut source = crate::sources::PathSource::new(&path, src.source_id()?, gctx); + let package = source.root_package()?; Dependency::from(package.summary()) }; selected From 161b1e76548aac8c069bc5a80508f3c440a4fc44 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 29 Jun 2024 21:20:29 -0400 Subject: [PATCH 5/8] refactor(source): Remove PathSource::read_packages With the split of `PathSource` / `RecursivePathSource`, this doesn't quite make sense anymore. --- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/cargo_uninstall.rs | 2 +- src/cargo/sources/path.rs | 11 ----------- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index e4304d74829..7abc1b39375 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -141,7 +141,7 @@ impl<'gctx> InstallablePackage<'gctx> { select_pkg( &mut src, dep, - |path: &mut PathSource<'_>| path.read_packages(), + |path: &mut PathSource<'_>| path.root_package().map(|p| vec![p]), gctx, current_rust_version, )? diff --git a/src/cargo/ops/cargo_uninstall.rs b/src/cargo/ops/cargo_uninstall.rs index c06e391fd5f..3a2fc39f25e 100644 --- a/src/cargo/ops/cargo_uninstall.rs +++ b/src/cargo/ops/cargo_uninstall.rs @@ -87,7 +87,7 @@ fn uninstall_cwd(root: &Filesystem, bins: &[String], gctx: &GlobalContext) -> Ca let pkg = select_pkg( &mut src, None, - |path: &mut PathSource<'_>| path.read_packages(), + |path: &mut PathSource<'_>| path.root_package().map(|p| vec![p]), gctx, None, )?; diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index da506bef4df..dd558df8fb5 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -80,17 +80,6 @@ impl<'gctx> PathSource<'gctx> { } } - /// Returns the packages discovered by this source. It may walk the - /// filesystem if package information haven't yet loaded. - pub fn read_packages(&self) -> CargoResult> { - if self.loaded { - Ok(self.package.clone().into_iter().collect()) - } else { - let pkg = self.read_package()?; - Ok(vec![pkg]) - } - } - fn read_package(&self) -> CargoResult { let path = self.path.join("Cargo.toml"); let pkg = ops::read_package(&path, self.source_id, self.gctx)?; From 5c820b8f86b074aebbcee87552435539d3aac1c4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 29 Jun 2024 21:22:54 -0400 Subject: [PATCH 6/8] refactor(source): Move PathSource::read_package closer to use I'd inline it into the only caller left but I expect it to get more complicated with cargo script. --- src/cargo/sources/path.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index dd558df8fb5..ca564e2cfd2 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -80,12 +80,6 @@ impl<'gctx> PathSource<'gctx> { } } - fn read_package(&self) -> CargoResult { - let path = self.path.join("Cargo.toml"); - let pkg = ops::read_package(&path, self.source_id, self.gctx)?; - Ok(pkg) - } - /// List all files relevant to building this package inside this source. /// /// This function will use the appropriate methods to determine the @@ -125,6 +119,12 @@ impl<'gctx> PathSource<'gctx> { Ok(()) } + + fn read_package(&self) -> CargoResult { + let path = self.path.join("Cargo.toml"); + let pkg = ops::read_package(&path, self.source_id, self.gctx)?; + Ok(pkg) + } } impl<'gctx> Debug for PathSource<'gctx> { From b89d18b4a60309b96fa6f164c0e4f93f61142330 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 29 Jun 2024 21:25:20 -0400 Subject: [PATCH 7/8] refactor(source): Remove redundant PathSource::loaded --- src/cargo/sources/path.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index ca564e2cfd2..3da0bcc1092 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -29,8 +29,6 @@ pub struct PathSource<'gctx> { source_id: SourceId, /// The root path of this source. path: PathBuf, - /// Whether this source has loaded all package information it may contain. - loaded: bool, /// Packages that this sources has discovered. package: Option, gctx: &'gctx GlobalContext, @@ -45,7 +43,6 @@ impl<'gctx> PathSource<'gctx> { Self { source_id, path: path.to_path_buf(), - loaded: false, package: None, gctx, } @@ -59,7 +56,6 @@ impl<'gctx> PathSource<'gctx> { Self { source_id, path, - loaded: true, package: Some(pkg), gctx, } @@ -96,7 +92,7 @@ impl<'gctx> PathSource<'gctx> { /// Gets the last modified file in a package. pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { - if !self.loaded { + if self.package.is_none() { return Err(internal(format!( "BUG: source `{:?}` was not loaded", self.path @@ -112,9 +108,8 @@ impl<'gctx> PathSource<'gctx> { /// Discovers packages inside this source if it hasn't yet done. pub fn load(&mut self) -> CargoResult<()> { - if !self.loaded { + if self.package.is_none() { self.package = Some(self.read_package()?); - self.loaded = true; } Ok(()) From 498245589841b6765ca28b6db235c4ce627a5a24 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 29 Jun 2024 21:52:49 -0400 Subject: [PATCH 8/8] refactor(source): Hide functions not needed elsewhere --- src/cargo/sources/path.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs index 3da0bcc1092..313159bcbc8 100644 --- a/src/cargo/sources/path.rs +++ b/src/cargo/sources/path.rs @@ -91,7 +91,7 @@ impl<'gctx> PathSource<'gctx> { } /// Gets the last modified file in a package. - pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { + fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { if self.package.is_none() { return Err(internal(format!( "BUG: source `{:?}` was not loaded", @@ -271,7 +271,7 @@ impl<'gctx> RecursivePathSource<'gctx> { } /// Gets the last modified file in a package. - pub fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { + fn last_modified_file(&self, pkg: &Package) -> CargoResult<(FileTime, PathBuf)> { if !self.loaded { return Err(internal(format!( "BUG: source `{:?}` was not loaded",