diff --git a/.changeset/ledger.yaml b/.changeset/ledger.yaml index 0504f52671..cb2d195b40 100644 --- a/.changeset/ledger.yaml +++ b/.changeset/ledger.yaml @@ -161,7 +161,7 @@ pacquet@12.0.0-alpha.12: - pacquet-symlink-dangling-junction-parent pacquet@12.0.0-alpha.13: dir: pnpm/npm/pnpm - intents: + intents: [] pacquet@12.0.0-alpha.9: dir: pnpm/npm/pnpm intents: diff --git a/.changeset/pacquet-workspace-lockfile-freshness.md b/.changeset/pacquet-workspace-lockfile-freshness.md new file mode 100644 index 0000000000..7d79e0a039 --- /dev/null +++ b/.changeset/pacquet-workspace-lockfile-freshness.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +Fixed installs to detect manifest changes in workspace members and reject stale lockfiles when using `--frozen-lockfile` [pnpm/pnpm#13080](https://github.com/pnpm/pnpm/issues/13080). diff --git a/pnpm/crates/cli/tests/workspace_install.rs b/pnpm/crates/cli/tests/workspace_install.rs index 5796091d9e..3603196393 100644 --- a/pnpm/crates/cli/tests/workspace_install.rs +++ b/pnpm/crates/cli/tests/workspace_install.rs @@ -19,7 +19,11 @@ use pacquet_testing_utils::{ bin::{AddMockedRegistry, CommandTempCwd}, fs::is_symlink_or_junction, }; -use std::fs; +use std::{fs, path::Path, process::Command}; + +fn pacquet_at(workspace: &Path) -> Command { + Command::cargo_bin("pnpm").expect("find the pnpm binary").with_current_dir(workspace) +} /// A workspace with two sibling projects, each pulling in a /// different mocked package, runs through the fresh-resolve path and @@ -141,6 +145,75 @@ fn fresh_resolve_walks_every_workspace_importer() { drop((root, mock_instance)); } +#[test] +fn changed_workspace_importer_invalidates_lockfile() { + let CommandTempCwd { root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + fs::write( + workspace.join("package.json"), + serde_json::json!({ "name": "root", "private": true }).to_string(), + ) + .expect("write root package.json"); + + let workspace_yaml_path = workspace.join("pnpm-workspace.yaml"); + let mut workspace_yaml = + fs::read_to_string(&workspace_yaml_path).expect("read pnpm-workspace.yaml"); + workspace_yaml.push_str("packages:\n - 'pkg-a'\n - 'pkg-b'\n"); + fs::write(&workspace_yaml_path, workspace_yaml).expect("write pnpm-workspace.yaml"); + + fs::create_dir(workspace.join("pkg-a")).expect("mkdir pkg-a"); + fs::write( + workspace.join("pkg-a/package.json"), + serde_json::json!({ "name": "pkg-a", "version": "1.0.0" }).to_string(), + ) + .expect("write pkg-a/package.json"); + fs::create_dir(workspace.join("pkg-b")).expect("mkdir pkg-b"); + fs::write( + workspace.join("pkg-b/package.json"), + serde_json::json!({ "name": "pkg-b", "version": "1.0.0" }).to_string(), + ) + .expect("write pkg-b/package.json"); + + pacquet_at(&workspace).with_arg("install").assert().success(); + + fs::write( + workspace.join("pkg-a/package.json"), + serde_json::json!({ + "name": "pkg-a", + "version": "1.0.0", + "dependencies": { "pkg-b": "workspace:*" }, + }) + .to_string(), + ) + .expect("update pkg-a/package.json"); + + let frozen_output = pacquet_at(&workspace) + .with_args(["install", "--frozen-lockfile"]) + .output() + .expect("run frozen install"); + let frozen_stderr = String::from_utf8_lossy(&frozen_output.stderr); + assert!( + !frozen_output.status.success(), + "frozen install accepted a stale workspace importer\nstderr:\n{frozen_stderr}", + ); + assert!( + frozen_stderr.contains("pacquet_package_manager::outdated_lockfile"), + "frozen install returned the wrong error\nstderr:\n{frozen_stderr}", + ); + + pacquet_at(&workspace).with_arg("install").assert().success(); + let linked_pkg = workspace.join("pkg-a/node_modules/pkg-b"); + assert!( + is_symlink_or_junction(&linked_pkg).expect("query pkg-b link"), + "normal install did not link the dependency added to pkg-a", + ); + assert!(linked_pkg.join("package.json").exists(), "pkg-b link is dangling"); + + drop((root, mock_instance)); +} + /// When the workspace root and a non-root importer both depend on the /// same workspace package via `workspace:*`, each importer's resolved /// `link:` target is relative to *its own* directory — pnpm writes diff --git a/pnpm/crates/lockfile/src/freshness.rs b/pnpm/crates/lockfile/src/freshness.rs index 361aab043f..3c36242f79 100644 --- a/pnpm/crates/lockfile/src/freshness.rs +++ b/pnpm/crates/lockfile/src/freshness.rs @@ -372,9 +372,6 @@ fn all_catalogs_are_up_to_date( /// when the lockfile is up-to-date; returns `Err(StalenessReason)` /// describing the first detected mismatch otherwise. /// -/// Single-importer only today (pacquet doesn't have workspace support -/// — see [#431]). Callers thread the root importer entry directly. -/// /// What is checked (in order, short-circuiting on the first failure): /// /// 1. Flat-record specifier diff against `devDependencies ∪ @@ -389,18 +386,13 @@ fn all_catalogs_are_up_to_date( /// Scoped to what pacquet supports today: no `auto-install-peers` /// pre-pass (pacquet has no separate auto-install-peers mode), no /// `excludeLinksFromLockfile` (`link:` resolutions aren't supported -/// yet — [#431] territory), and no version-range-satisfies check for +/// yet), and no version-range-satisfies check for /// file: / tarball deps (out of scope here). -/// -/// [#431]: https://github.com/pnpm/pacquet/issues/431 pub fn satisfies_package_manifest( importer: &ProjectSnapshot, manifest: &PackageManifest, - importer_id: &str, is_ignored_optional: &dyn Fn(&str) -> bool, ) -> Result<(), StalenessReason> { - let _ = importer_id; // reserved for the multi-importer path once lands. - // Phase 1: flat-record diff against the manifest's union of // dependency fields. Compares the importer's specifiers to the // manifest's existing deps (devs + prod + optional flattened diff --git a/pnpm/crates/lockfile/src/freshness/tests.rs b/pnpm/crates/lockfile/src/freshness/tests.rs index 3faad40bfb..335d2e5f93 100644 --- a/pnpm/crates/lockfile/src/freshness/tests.rs +++ b/pnpm/crates/lockfile/src/freshness/tests.rs @@ -56,7 +56,7 @@ fn matching_manifest_and_lockfile_satisfies() { } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } #[test] @@ -82,7 +82,7 @@ fn manifest_adds_dep_returns_specifier_diff() { } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); let StalenessReason::SpecifiersDiffer(diff) = err else { panic!("expected SpecifiersDiffer, got {err:?}"); @@ -117,7 +117,7 @@ fn manifest_drops_dep_returns_specifier_diff() { } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); let StalenessReason::SpecifiersDiffer(diff) = err else { panic!("expected SpecifiersDiffer, got {err:?}"); @@ -147,7 +147,7 @@ fn manifest_bumps_specifier_returns_specifier_diff() { } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); let StalenessReason::SpecifiersDiffer(diff) = err else { panic!("expected SpecifiersDiffer, got {err:?}"); @@ -187,7 +187,7 @@ fn matching_across_all_three_dep_fields_satisfies() { "optionalDependencies": { "fsevents": "^2.0.0" } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } /// Lockfile has no `importers["."]` entry — even though pacquet's @@ -230,7 +230,7 @@ fn dep_moves_between_fields_returns_dep_specifier_mismatch() { "dependencies": { "typescript": "^5.0.0" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::DepSpecifierMismatch { .. }), @@ -280,7 +280,7 @@ fn cross_field_swap_with_same_cardinalities_caught_by_per_field_check() { "devDependencies": { "react": "^17.0.2" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::DepSpecifierMismatch { .. }), @@ -312,7 +312,7 @@ fn publish_directory_mismatch_returns_publish_directory_mismatch() { "dependencies": { "react": "^17.0.2" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::PublishDirectoryMismatch { .. }), @@ -343,7 +343,7 @@ fn dependencies_meta_mismatch_returns_dependencies_meta_mismatch() { "dependencies": { "foo": "^1.0.0" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("should be stale"); assert!( matches!(err, StalenessReason::DependenciesMetaMismatch { .. }), @@ -415,7 +415,7 @@ fn dependencies_meta_empty_object_equivalent_to_absent() { "dependenciesMeta": {} }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } #[test] @@ -440,7 +440,7 @@ fn publish_directory_match_satisfies() { "dependencies": { "foo": "1.0.0" } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } #[test] @@ -465,7 +465,7 @@ fn same_dep_in_prod_and_dev_counts_under_prod() { }"#, ); assert!( - satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok(), + satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok(), "manifest listing foo in prod+dev must satisfy a lockfile that records it under prod only", ); } @@ -492,7 +492,7 @@ fn same_dep_in_prod_and_optional_counts_under_optional() { }"#, ); assert!( - satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok(), + satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok(), "manifest listing foo in prod+optional must satisfy a lockfile that records it under optional only", ); } @@ -518,7 +518,7 @@ fn importer_empty_dev_dependencies_equivalent_to_absent() { "dependencies": { "foo": "^1.0.0" } }"#, ); - assert!(satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, &|_: &str| false).is_ok()); } // --------------------------------------------------------------------------- @@ -1296,7 +1296,7 @@ fn ignored_optional_filtered_out_of_manifest_diff() { }"#, ); let is_ignored: &dyn Fn(&str) -> bool = &|name: &str| name == "foo"; - assert!(satisfies_package_manifest(importer, &manifest, ".", is_ignored).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, is_ignored).is_ok()); } /// Polarity: without the filter the same fixture must fail. @@ -1323,7 +1323,7 @@ fn ignored_optional_without_filter_surfaces_as_drift() { "optionalDependencies": { "foo": "^1.0.0" } }"#, ); - let err = satisfies_package_manifest(importer, &manifest, ".", &|_: &str| false) + let err = satisfies_package_manifest(importer, &manifest, &|_: &str| false) .expect_err("without the filter the manifest's extra `foo` must surface as drift"); assert!( matches!(err, StalenessReason::SpecifiersDiffer(_)), @@ -1377,7 +1377,7 @@ fn ignored_optional_does_not_apply_to_dev_dependencies() { }"#, ); let is_ignored: &dyn Fn(&str) -> bool = &|name: &str| name == "foo"; - assert!(satisfies_package_manifest(importer, &manifest, ".", is_ignored).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, is_ignored).is_ok()); } /// Pins the group gate: removing the `matches!(... Prod | Optional)` @@ -1404,5 +1404,5 @@ fn ignored_optional_dev_only_lockfile_entry_kept() { }"#, ); let is_ignored: &dyn Fn(&str) -> bool = &|name: &str| name == "foo"; - assert!(satisfies_package_manifest(importer, &manifest, ".", is_ignored).is_ok()); + assert!(satisfies_package_manifest(importer, &manifest, is_ignored).is_ok()); } diff --git a/pnpm/crates/package-manager/src/install.rs b/pnpm/crates/package-manager/src/install.rs index dd8aa827ee..bb0b590fd6 100644 --- a/pnpm/crates/package-manager/src/install.rs +++ b/pnpm/crates/package-manager/src/install.rs @@ -813,7 +813,8 @@ where current_lockfile.as_ref().and_then(|current| { check_lockfile_freshness( current, - manifest, + &workspace_root, + &project_manifests, config, &catalogs, ignore_manifest_check, @@ -956,10 +957,17 @@ where // Run the freshness gates; on failure surface a fatal // InstallError via `FreshnessCheckError`'s `From` impl. // The check is run for its side effect (the typed - // outcome) — the borrowed lockfile / manifest are consumed + // outcome) — the borrowed lockfile / manifests are consumed // again inside the frozen branch below. - check_lockfile_freshness(lockfile, manifest, config, &catalogs, ignore_manifest_check) - .map_err(InstallError::from)?; + check_lockfile_freshness( + lockfile, + &workspace_root, + &project_manifests, + config, + &catalogs, + ignore_manifest_check, + ) + .map_err(InstallError::from)?; true } else if update_checksums { false @@ -974,7 +982,8 @@ where if prefer_frozen_lockfile { match check_lockfile_freshness( lockfile, - manifest, + &workspace_root, + &project_manifests, config, &catalogs, ignore_manifest_check, @@ -1778,7 +1787,8 @@ where /// `ignoredOptionalDependencies`) still runs. fn check_lockfile_freshness( lockfile: &Lockfile, - manifest: &PackageManifest, + workspace_root: &Path, + project_manifests: &[(PathBuf, &PackageManifest)], config: &Config, catalogs: &Catalogs, ignore_manifest_check: bool, @@ -1790,17 +1800,17 @@ fn check_lockfile_freshness( return Ok(()); } - // Pacquet has only one importer today ( tracks workspaces), - // so the root project is the only thing to verify; once - // workspaces land this becomes a per-project loop over - // `lockfile.importers`. - check_importer_satisfies( - lockfile, - manifest, - Lockfile::ROOT_IMPORTER_KEY, - config, - parsed_overrides_opt.as_deref(), - ) + for (project_dir, manifest) in project_manifests { + let importer_id = pacquet_workspace::importer_id_from_root_dir(workspace_root, project_dir); + check_importer_satisfies( + lockfile, + manifest, + &importer_id, + config, + parsed_overrides_opt.as_deref(), + )?; + } + Ok(()) } /// Parse `pnpm.overrides` from the config. Values can use the @@ -1916,7 +1926,7 @@ pub(crate) fn check_importer_satisfies( .unwrap_or_default(); let is_ignored_optional: &dyn Fn(&str) -> bool = &|name: &str| ignored_set.contains(name); - satisfies_package_manifest(importer, manifest_for_freshness, importer_id, is_ignored_optional) + satisfies_package_manifest(importer, manifest_for_freshness, is_ignored_optional) .map_err(FreshnessCheckError::Stale) } diff --git a/pnpr/crates/pnpr/src/resolver/resolve.rs b/pnpr/crates/pnpr/src/resolver/resolve.rs index 196b103cb7..02821c4e49 100644 --- a/pnpr/crates/pnpr/src/resolver/resolve.rs +++ b/pnpr/crates/pnpr/src/resolver/resolve.rs @@ -272,8 +272,7 @@ pub fn fresh_frozen_input_lockfile(config: &Config, request: &ResolveRequest) -> }); std::fs::write(&manifest_path, serde_json::to_vec(&manifest_json).ok()?).ok()?; let manifest = PackageManifest::from_path(manifest_path).ok()?; - satisfies_package_manifest(importer, &manifest, Lockfile::ROOT_IMPORTER_KEY, &|_: &str| false) - .ok()?; + satisfies_package_manifest(importer, &manifest, &|_: &str| false).ok()?; Some(lockfile.clone()) }