mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-19 04:02:32 -04:00
fix(pacquet): validate workspace lockfile importers
Check every workspace project manifest against its matching lockfile importer before taking the frozen path. This makes normal installs refresh stale member importers and makes frozen installs reject them. Also restore the empty intent list in the versioning ledger so release-plan validation can parse the committed file. Closes pnpm/pnpm#13080.
This commit is contained in:
@@ -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:
|
||||
|
||||
5
.changeset/pacquet-workspace-lockfile-freshness.md
Normal file
5
.changeset/pacquet-workspace-lockfile-freshness.md
Normal file
@@ -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).
|
||||
@@ -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
|
||||
|
||||
@@ -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 <https://github.com/pnpm/pacquet/issues/431> 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
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
|
||||
@@ -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 (<https://github.com/pnpm/pacquet/issues/431> 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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user