From e186bf4e7ae23a15446f22b77ff9f5c96469f8a8 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 26 Mar 2024 16:52:24 -0400 Subject: [PATCH 1/3] test(generate-lockfile): verify `--offline` panics --- tests/testsuite/generate_lockfile.rs | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/testsuite/generate_lockfile.rs b/tests/testsuite/generate_lockfile.rs index 0a3e1395a58..aa4aed1980a 100644 --- a/tests/testsuite/generate_lockfile.rs +++ b/tests/testsuite/generate_lockfile.rs @@ -237,3 +237,41 @@ fn duplicate_entries_in_lockfile() { ) .run(); } + +#[cargo_test] +fn generate_lockfile_holds_lock_and_offline() { + Package::new("syn", "1.0.0").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + + [dependencies] + syn = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile") + .with_stderr( + "\ +[UPDATING] `[..]` index +[LOCKING] 2 packages +", + ) + .run(); + + p.cargo("generate-lockfile --offline") + .with_status(101) + .with_stderr_contains( + "\ +[..]thread 'main' panicked[..] +[..]package cache lock is not currently held[..] +", + ) + .run(); +} From 5587af7caf1f4e96a56c6dc35b13f84c99987f20 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 26 Mar 2024 16:20:19 -0400 Subject: [PATCH 2/3] refactor: remove unnecessary export so no need to deal with lock acquisitions --- src/cargo/ops/cargo_generate_lockfile.rs | 2 +- src/cargo/ops/mod.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 87aa7242395..d6fdc3d94a0 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -331,7 +331,7 @@ fn print_lockfile_sync( Ok(()) } -pub fn print_lockfile_updates( +fn print_lockfile_updates( gctx: &GlobalContext, previous_resolve: &Resolve, resolve: &Resolve, diff --git a/src/cargo/ops/mod.rs b/src/cargo/ops/mod.rs index 9a751793da2..3b4c74c8922 100644 --- a/src/cargo/ops/mod.rs +++ b/src/cargo/ops/mod.rs @@ -9,7 +9,6 @@ pub use self::cargo_doc::{doc, DocOptions, OutputFormat}; pub use self::cargo_fetch::{fetch, FetchOptions}; pub use self::cargo_generate_lockfile::generate_lockfile; pub use self::cargo_generate_lockfile::print_lockfile_changes; -pub use self::cargo_generate_lockfile::print_lockfile_updates; pub use self::cargo_generate_lockfile::update_lockfile; pub use self::cargo_generate_lockfile::UpdateOptions; pub use self::cargo_install::{install, install_list}; From f1c139624f0fe85fdf34e43919e3c9d506bca3f7 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 26 Mar 2024 16:53:45 -0400 Subject: [PATCH 3/3] fix(generate-lockfile): hold lock before querying index --- src/cargo/ops/cargo_generate_lockfile.rs | 5 +++++ src/cargo/ops/resolve.rs | 5 ----- tests/testsuite/generate_lockfile.rs | 4 +--- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index d6fdc3d94a0..e303d868da4 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -166,12 +166,17 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes Ok(()) } +/// Prints lockfile change statuses. +/// +/// This would acquire the package-cache lock, as it may update the index to +/// show users latest available versions. pub fn print_lockfile_changes( gctx: &GlobalContext, previous_resolve: Option<&Resolve>, resolve: &Resolve, registry: &mut PackageRegistry<'_>, ) -> CargoResult<()> { + let _lock = gctx.acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; if let Some(previous_resolve) = previous_resolve { print_lockfile_sync(gctx, previous_resolve, resolve, registry) } else { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 1042c897bcf..31a37551414 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -256,11 +256,6 @@ fn resolve_with_registry<'gctx>( false }; if print { - // We only want one Cargo at a time resolving a crate graph since this can - // involve a lot of frobbing of the global caches. - let _lock = ws - .gctx() - .acquire_package_cache_lock(CacheLockMode::DownloadExclusive)?; ops::print_lockfile_changes(ws.gctx(), prev.as_ref(), &resolve, registry)?; } Ok(resolve) diff --git a/tests/testsuite/generate_lockfile.rs b/tests/testsuite/generate_lockfile.rs index aa4aed1980a..2c0b65a0db1 100644 --- a/tests/testsuite/generate_lockfile.rs +++ b/tests/testsuite/generate_lockfile.rs @@ -266,11 +266,9 @@ fn generate_lockfile_holds_lock_and_offline() { .run(); p.cargo("generate-lockfile --offline") - .with_status(101) .with_stderr_contains( "\ -[..]thread 'main' panicked[..] -[..]package cache lock is not currently held[..] +[LOCKING] 2 packages ", ) .run();