From 48f147cd0f6cdb54e7f01154258692cc2f0e9108 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 3 Mar 2025 20:10:34 -0500 Subject: [PATCH 1/2] test: show problematic behavior that delete non-cached sources Sources like directory sources (which usually are vendored source) likely contains real sources not cached sources. Cargo shouldn't delete them. --- tests/testsuite/vendor.rs | 41 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/testsuite/vendor.rs b/tests/testsuite/vendor.rs index 7ea31b5b5..4256d1de3 100644 --- a/tests/testsuite/vendor.rs +++ b/tests/testsuite/vendor.rs @@ -1939,3 +1939,44 @@ fn vendor_crate_with_ws_inherit() { "#]]) .run(); } + +#[cargo_test] +fn dont_delete_non_registry_sources_with_respect_source_config() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + log = "0.3.5" + "#, + ) + .file("src/lib.rs", "") + .build(); + + Package::new("log", "0.3.5").publish(); + + p.cargo("vendor --respect-source-config").run(); + let lock = p.read_file("vendor/log/Cargo.toml"); + assert!(lock.contains("version = \"0.3.5\"")); + + add_crates_io_vendor_config(&p); + p.cargo("vendor --respect-source-config new-vendor-dir") + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] failed to sync + +Caused by: + failed to load pkg lockfile + +Caused by: + no matching package named `log` found + location searched: directory source `[ROOT]/foo/vendor` (which is replacing registry `crates-io`) + required by package `foo v0.1.0 ([ROOT]/foo)` + +"#]]) + .run(); +} From 9e18e48f8c389c185c3e16e2a575b213343094fe Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 3 Mar 2025 20:20:43 -0500 Subject: [PATCH 2/2] fix(vendor): dont remove non-cached source cargo-vendor has a workaround that to mitigate rust-lang/cargo#5956, it removes all cached sources in order to trigger a re-unpack. It was meant for dealing with registry sources only, but accidentally applied to directory source kind. While directory source kind was invented for vendoring, and vendoring from one vendored directory to the other seems unusual, Cargo IMO should not delete any real sources. It does not mean that registry sources are okay to delete, In long term, we should explore a way that unpacks `.crate` files directly, without any removal. See https://github.com/rust-lang/cargo/pull/12509#issuecomment-1732415990 --- src/bin/cargo/commands/vendor.rs | 4 ++- src/cargo/ops/vendor.rs | 52 +++++++++++++++++++++++++++++--- tests/testsuite/vendor.rs | 17 ++++++----- 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/src/bin/cargo/commands/vendor.rs b/src/bin/cargo/commands/vendor.rs index 96b306767..63db209cc 100644 --- a/src/bin/cargo/commands/vendor.rs +++ b/src/bin/cargo/commands/vendor.rs @@ -60,7 +60,8 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult { // to respect any of the `source` configuration in Cargo itself. That's // intended for other consumers of Cargo, but we want to go straight to the // source, e.g. crates.io, to fetch crates. - if !args.flag("respect-source-config") { + let respect_source_config = args.flag("respect-source-config"); + if !respect_source_config { gctx.values_mut()?.remove("source"); } @@ -80,6 +81,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult { .unwrap_or_default() .cloned() .collect(), + respect_source_config, }, )?; Ok(()) diff --git a/src/cargo/ops/vendor.rs b/src/cargo/ops/vendor.rs index 44e2a2add..381e2528a 100644 --- a/src/cargo/ops/vendor.rs +++ b/src/cargo/ops/vendor.rs @@ -1,8 +1,10 @@ use crate::core::shell::Verbosity; +use crate::core::SourceId; use crate::core::{GitReference, Package, Workspace}; use crate::ops; use crate::sources::path::PathSource; use crate::sources::PathEntry; +use crate::sources::SourceConfigMap; use crate::sources::CRATES_IO_REGISTRY; use crate::util::cache_lock::CacheLockMode; use crate::util::{try_canonicalize, CargoResult, GlobalContext}; @@ -21,6 +23,7 @@ pub struct VendorOptions<'a> { pub versioned_dirs: bool, pub destination: &'a Path, pub extra: Vec, + pub respect_source_config: bool, } pub fn vendor(ws: &Workspace<'_>, opts: &VendorOptions<'_>) -> CargoResult<()> { @@ -76,6 +79,32 @@ enum VendorSource { }, } +/// Cache for mapping replaced sources to replacements. +struct SourceReplacementCache<'gctx> { + map: SourceConfigMap<'gctx>, + cache: HashMap, +} + +impl SourceReplacementCache<'_> { + fn new(gctx: &GlobalContext) -> CargoResult> { + Ok(SourceReplacementCache { + map: SourceConfigMap::new(gctx)?, + cache: Default::default(), + }) + } + + fn get(&mut self, id: SourceId) -> CargoResult { + use std::collections::hash_map::Entry; + match self.cache.entry(id) { + Entry::Occupied(e) => Ok(e.get().clone()), + Entry::Vacant(e) => { + let replaced = self.map.load(id, &HashSet::new())?.replaced_source_id(); + Ok(e.insert(replaced).clone()) + } + } + } +} + fn sync( gctx: &GlobalContext, workspaces: &[&Workspace<'_>], @@ -101,6 +130,8 @@ fn sync( } } + let mut source_replacement_cache = SourceReplacementCache::new(gctx)?; + // First up attempt to work around rust-lang/cargo#5956. Apparently build // artifacts sprout up in Cargo's global cache for whatever reason, although // it's unsure what tool is causing these issues at this time. For now we @@ -121,20 +152,31 @@ fn sync( .context("failed to download packages")?; for pkg in resolve.iter() { + let sid = if opts.respect_source_config { + source_replacement_cache.get(pkg.source_id())? + } else { + pkg.source_id() + }; + // Don't delete actual source code! - if pkg.source_id().is_path() { - if let Ok(path) = pkg.source_id().url().to_file_path() { + if sid.is_path() { + if let Ok(path) = sid.url().to_file_path() { if let Ok(path) = try_canonicalize(path) { to_remove.remove(&path); } } continue; } - if pkg.source_id().is_git() { + if sid.is_git() { continue; } - if let Ok(pkg) = packages.get_one(pkg) { - drop(fs::remove_dir_all(pkg.root())); + + // Only delete sources that are safe to delete, i.e. they are caches. + if sid.is_registry() { + if let Ok(pkg) = packages.get_one(pkg) { + drop(fs::remove_dir_all(pkg.root())); + } + continue; } } } diff --git a/tests/testsuite/vendor.rs b/tests/testsuite/vendor.rs index 4256d1de3..dbf956eed 100644 --- a/tests/testsuite/vendor.rs +++ b/tests/testsuite/vendor.rs @@ -1965,17 +1965,18 @@ fn dont_delete_non_registry_sources_with_respect_source_config() { add_crates_io_vendor_config(&p); p.cargo("vendor --respect-source-config new-vendor-dir") - .with_status(101) .with_stderr_data(str![[r#" -[ERROR] failed to sync + Vendoring log v0.3.5 ([ROOT]/foo/vendor/log) to new-vendor-dir/log +To use vendored sources, add this to your .cargo/config.toml for this project: -Caused by: - failed to load pkg lockfile -Caused by: - no matching package named `log` found - location searched: directory source `[ROOT]/foo/vendor` (which is replacing registry `crates-io`) - required by package `foo v0.1.0 ([ROOT]/foo)` +"#]]) + .with_stdout_data(str![[r#" +[source.crates-io] +replace-with = "vendored-sources" + +[source.vendored-sources] +directory = "new-vendor-dir" "#]]) .run();