From 4f37bb5429879a5f5d0bd2ff7301bb70f95e71d8 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 18 Sep 2026 21:27:37 +0200 Subject: [PATCH] fix(install): keep the repeat-install fast path for deduped siblings and large lockfiles (#15073) Two gates of the repeat-install check refused every run in common Bit workspaces, and one of them affects plain pnpm workspaces too. modules_dirs_present treated a sibling project without node_modules as never installed. Under dedupeDirectDeps a sibling whose every direct dependency the root declares identically gets nothing linked, so the linker never creates that directory; the sibling is installed all the same. The gate now lets such a sibling through when the root's modules directory exists and, for every alias the sibling declares in a group the install materializes, the wanted lockfile records one target on each side and the two agree, which is what the linker compares. link: targets are resolved against each importer's directory; an alias a side declares with differing targets across groups proves nothing, since the linker picks one by group order and the check does not reproduce that; a modules directory has to be a directory. The TypeScript checkDepsStatus had the same gate and gets the same rule. The merge-conflict scan of a changed lockfile refused files of 16 MiB or more as unverifiable. Every changed lockfile that passes the scan is parsed in full right after, so the size budget could only refuse what the parse would read anyway; the scan now streams the whole file through its fixed 8 KiB buffer. The symlink and non-regular-file refusals stay. Co-Authored-By: Claude Fable 5.1 --- .changeset/repeat-install-deduped-siblings.md | 8 + .../repeat-install-large-lockfile-scan.md | 6 + .../src/optimistic_repeat_install.rs | 2 +- .../conflict_markers.rs | 39 ++-- .../optimistic_repeat_install/deps_status.rs | 3 +- .../src/optimistic_repeat_install/settle.rs | 192 +++++++++++++--- .../tests/lockfile.rs | 53 +++-- .../tests/workspace.rs | 209 +++++++++++++++++- pnpm11/deps/status/src/checkDepsStatus.ts | 82 ++++++- .../deps/status/test/checkDepsStatus.test.ts | 189 +++++++++++++++- 10 files changed, 704 insertions(+), 79 deletions(-) create mode 100644 .changeset/repeat-install-deduped-siblings.md create mode 100644 .changeset/repeat-install-large-lockfile-scan.md diff --git a/.changeset/repeat-install-deduped-siblings.md b/.changeset/repeat-install-deduped-siblings.md new file mode 100644 index 0000000000..c2b92936b7 --- /dev/null +++ b/.changeset/repeat-install-deduped-siblings.md @@ -0,0 +1,8 @@ +--- +"pacquet": patch +"@pnpm/napi": patch +"@pnpm/deps.status": patch +"pnpm": patch +--- + +`pnpm install` now returns "Already up to date" in a workspace where `dedupeDirectDeps` left a project without a `node_modules` directory of its own. Such a project forced a full install on every run. diff --git a/.changeset/repeat-install-large-lockfile-scan.md b/.changeset/repeat-install-large-lockfile-scan.md new file mode 100644 index 0000000000..4be7af3762 --- /dev/null +++ b/.changeset/repeat-install-large-lockfile-scan.md @@ -0,0 +1,6 @@ +--- +"pacquet": patch +"@pnpm/napi": patch +--- + +`pnpm install` no longer refuses the repeat-install fast path just because a changed `pnpm-lock.yaml` is 16 MiB or larger. Such a lockfile forced a full install on the run after every change. diff --git a/pnpm/crates/package-manager/src/optimistic_repeat_install.rs b/pnpm/crates/package-manager/src/optimistic_repeat_install.rs index 7e501d3512..5a428dbf18 100644 --- a/pnpm/crates/package-manager/src/optimistic_repeat_install.rs +++ b/pnpm/crates/package-manager/src/optimistic_repeat_install.rs @@ -434,7 +434,7 @@ fn settings_block_fast_path( // overrides yet, so check the install-time `config.modules_dir` // for the root + `/node_modules` for siblings, // matching the `isolated`-linker default. - if !modules_dirs_present(config, node_linker, project_manifests) { + if !modules_dirs_present(check) { return Some("project has dependencies but no node_modules directory"); } None diff --git a/pnpm/crates/package-manager/src/optimistic_repeat_install/conflict_markers.rs b/pnpm/crates/package-manager/src/optimistic_repeat_install/conflict_markers.rs index 57d95e34fc..533b491951 100644 --- a/pnpm/crates/package-manager/src/optimistic_repeat_install/conflict_markers.rs +++ b/pnpm/crates/package-manager/src/optimistic_repeat_install/conflict_markers.rs @@ -40,8 +40,6 @@ pub(crate) const CONFLICT_MARKER: &[u8] = b"<<<<<<<"; pub(crate) const LOCKFILE_CONFLICT_SCAN_BUFFER_SIZE: usize = 8 * 1024; -pub(crate) const MAX_LOCKFILE_CONFLICT_SCAN_BYTES: u64 = 16 * 1024 * 1024; - pub(crate) fn lockfile_conflict_check_failure( path: &Path, last_validated_timestamp: i64, @@ -60,9 +58,6 @@ pub(crate) fn lockfile_conflict_check_failure( if !lockfile_modified_since(mtime, last_validated_timestamp) { return None; } - if metadata.len() >= MAX_LOCKFILE_CONFLICT_SCAN_BYTES { - return Some(LockfileConflictCheckFailure::Unsafe); - } modified_lockfile_conflict_check_failure(path) } @@ -72,22 +67,20 @@ pub(crate) fn modified_lockfile_conflict_check_failure( let Some(mut file) = open_for_conflict_scan(path) else { return Some(LockfileConflictCheckFailure::Unsafe); }; + // The scan streams the whole file through one buffer, whatever its + // size: every changed lockfile that passes it is parsed in full next, + // so a size budget here could only refuse what the parse would read + // anyway. let mut buffer = [0; LOCKFILE_CONFLICT_SCAN_BUFFER_SIZE + CONFLICT_MARKER.len() - 1]; let mut carried = 0; - let mut scanned = 0_u64; loop { - let remaining = MAX_LOCKFILE_CONFLICT_SCAN_BYTES.saturating_sub(scanned); - if remaining == 0 { - return Some(LockfileConflictCheckFailure::Unsafe); - } - let read_capacity = LOCKFILE_CONFLICT_SCAN_BUFFER_SIZE.min(remaining as usize); - let read = match file.read(&mut buffer[carried..carried + read_capacity]) { - Ok(0) => return None, - Ok(read) => read, - Err(error) if error.kind() == ErrorKind::Interrupted => continue, - Err(_) => return Some(LockfileConflictCheckFailure::Unsafe), - }; - scanned += read as u64; + let read = + match file.read(&mut buffer[carried..carried + LOCKFILE_CONFLICT_SCAN_BUFFER_SIZE]) { + Ok(0) => return None, + Ok(read) => read, + Err(error) if error.kind() == ErrorKind::Interrupted => continue, + Err(_) => return Some(LockfileConflictCheckFailure::Unsafe), + }; let end = carried + read; if chunk_contains_marker(&buffer[..end]) { return Some(LockfileConflictCheckFailure::MergeConflict); @@ -100,14 +93,14 @@ pub(crate) fn modified_lockfile_conflict_check_failure( } /// The lockfile, opened for the conflict scan. `None` for anything the scan -/// cannot read to a verdict: a missing or unreadable path, a non-file, or a -/// file too large to scan within the budget. +/// cannot read to a verdict: a missing or unreadable path, or a non-file. fn open_for_conflict_scan(path: &Path) -> Option { let file = fs::File::open(path).ok()?; let metadata = file.metadata().ok()?; - (metadata.file_type().is_file() && metadata.len() < MAX_LOCKFILE_CONFLICT_SCAN_BYTES).then_some( - file, - ) + metadata + .file_type() + .is_file() + .then_some(file) } fn chunk_contains_marker(bytes: &[u8]) -> bool { diff --git a/pnpm/crates/package-manager/src/optimistic_repeat_install/deps_status.rs b/pnpm/crates/package-manager/src/optimistic_repeat_install/deps_status.rs index 0371131957..cf631fe98a 100644 --- a/pnpm/crates/package-manager/src/optimistic_repeat_install/deps_status.rs +++ b/pnpm/crates/package-manager/src/optimistic_repeat_install/deps_status.rs @@ -135,7 +135,6 @@ fn first_workspace_drift( config, project_manifests, is_workspace_install, - layout: crate::RepeatInstallLayout { node_linker, .. }, .. } = check; if !project_structure_matches(state, project_manifests) { @@ -144,7 +143,7 @@ fn first_workspace_drift( // A filtered install legitimately leaves unselected projects // without a modules directory. if !state.filtered_install - && let Some(id) = first_project_missing_modules_dir(config, node_linker, project_manifests) + && let Some(id) = first_project_missing_modules_dir(check) { return Some(format!( "Workspace package {id} has dependencies but does not have a modules directory", diff --git a/pnpm/crates/package-manager/src/optimistic_repeat_install/settle.rs b/pnpm/crates/package-manager/src/optimistic_repeat_install/settle.rs index 4c435ea1c7..78a36c28f7 100644 --- a/pnpm/crates/package-manager/src/optimistic_repeat_install/settle.rs +++ b/pnpm/crates/package-manager/src/optimistic_repeat_install/settle.rs @@ -4,9 +4,13 @@ use super::{ manifest_has_runtime_deps, manifest_string_field, }; use pnpm_config::{Config, LinkWorkspacePackages, NodeLinker}; -use pnpm_lockfile::Lockfile; -use pnpm_modules_yaml::Host; -use pnpm_package_manifest::PackageManifest; +use pnpm_fs::lexical_normalize; +use pnpm_lockfile::{ + Lockfile, MaybeLazyLockfile, PkgName, ProjectSnapshot, ResolvedDependencySpec, +}; +use pnpm_modules_yaml::{Host, IncludedDependencies}; +use pnpm_package_manifest::{DependencyGroup, PackageManifest}; +use pnpm_workspace::importer_id_from_root_dir; use pnpm_workspace_state::{WorkspaceState, update_workspace_state}; use std::{ fs, @@ -206,50 +210,174 @@ pub(super) fn project_structure_matches( == manifest_string_field(manifest, "version").as_deref().unwrap_or("0.0.0") }) } -pub(super) fn modules_dirs_present( - config: &Config, - node_linker: NodeLinker, - project_manifests: &[(PathBuf, &PackageManifest)], -) -> bool { - first_project_missing_modules_dir(config, node_linker, project_manifests).is_none() +pub(super) fn modules_dirs_present(check: &OptimisticRepeatInstallCheck<'_>) -> bool { + first_project_missing_modules_dir(check).is_none() } /// The id (`name` field, falling back to the root dir) of the first /// project that declares dependencies but has no modules directory, or /// `None` when every project with dependencies has one. +/// +/// Under `dedupeDirectDeps` a sibling whose every direct dependency +/// resolves to the same target as the root's gets nothing linked, so the +/// linker never creates its modules directory; such a sibling is installed +/// all the same and does not count as missing one. pub(super) fn first_project_missing_modules_dir( - config: &Config, - node_linker: NodeLinker, - project_manifests: &[(PathBuf, &PackageManifest)], + check: &OptimisticRepeatInstallCheck<'_>, ) -> Option { - let root_modules_dir_exists = config.modules_dir.exists(); + let &OptimisticRepeatInstallCheck { + workspace_root, + config, + project_manifests, + lockfile, + layout: crate::RepeatInstallLayout { node_linker, included, .. }, + .. + } = check; + let root_modules_dir_exists = config.modules_dir.is_dir(); project_manifests .iter() .find_map(|(root_dir, manifest)| { - if !manifest_has_runtime_deps(manifest) { - return None; - } - // The root importer uses `config.modules_dir`; siblings use - // their own `/node_modules`. Matches the isolated-linker - // default — `config.modules_dir` is `/node_modules` - // unless the user overrode it explicitly. - let modules_dir_exists = match node_linker { - NodeLinker::Hoisted => root_modules_dir_exists, - NodeLinker::Isolated | NodeLinker::Pnp => { - if *root_dir == workspace_dir_of(config, root_dir) { - root_modules_dir_exists - } else { - root_dir.join("node_modules").exists() - } - } - }; - - (!modules_dir_exists).then(|| { + let root_project_dir = workspace_dir_of(config, root_dir); + let is_root = *root_dir == root_project_dir; + let installed = !manifest_has_runtime_deps(manifest) + || modules_dir_exists(node_linker, root_dir, is_root, root_modules_dir_exists) + || (!is_root + && root_modules_dir_exists + && config.dedupe_direct_deps + && dedupe_links_nothing( + lockfile, + &included_groups(included), + DedupeImporters { + lockfile_root: workspace_root, + root_dir: &root_project_dir, + sibling_dir: root_dir, + }, + )); + (!installed).then(|| { manifest_string_field(manifest, "name") .unwrap_or_else(|| root_dir.to_string_lossy().into_owned()) }) }) } + +/// The root importer uses `config.modules_dir`; siblings use their own +/// `/node_modules`. Matches the isolated-linker default — +/// `config.modules_dir` is `/node_modules` unless the user +/// overrode it explicitly. +fn modules_dir_exists( + node_linker: NodeLinker, + root_dir: &Path, + is_root: bool, + root_modules_dir_exists: bool, +) -> bool { + match node_linker { + NodeLinker::Hoisted => root_modules_dir_exists, + NodeLinker::Isolated | NodeLinker::Pnp => { + if is_root { + root_modules_dir_exists + } else { + root_dir.join("node_modules").is_dir() + } + } + } +} + +/// The dependency groups this install materializes, the only ones the +/// linker links and dedupes. +fn included_groups(included: IncludedDependencies) -> Vec { + [ + (included.dependencies, DependencyGroup::Prod), + (included.dev_dependencies, DependencyGroup::Dev), + (included.optional_dependencies, DependencyGroup::Optional), + ] + .into_iter() + .filter_map(|(included, group)| included.then_some(group)) + .collect() +} + +/// The two importers a dedupe verdict compares, as directories. +#[derive(Clone, Copy)] +struct DedupeImporters<'a> { + /// The directory importer ids are relative to. + lockfile_root: &'a Path, + root_dir: &'a Path, + sibling_dir: &'a Path, +} + +/// Whether `dedupeDirectDeps` links nothing into the sibling: for every +/// alias the sibling declares in a materialized group, the wanted lockfile +/// records one target on each side, and the two are the same, which is what +/// the linker compares. An alias declared with differing targets in several +/// groups has one effective target the linker picks by group order; that +/// choice is not reproduced here, so such an alias proves nothing. Neither +/// does a lockfile that cannot be loaded or lacks either importer. +fn dedupe_links_nothing( + lockfile: MaybeLazyLockfile<'_>, + groups: &[DependencyGroup], + importers: DedupeImporters<'_>, +) -> bool { + let DedupeImporters { + lockfile_root, + root_dir, + sibling_dir, + } = importers; + let Ok(Some(lockfile)) = lockfile.get() else { return false }; + let importer = + |dir: &Path| lockfile.importers.get(&importer_id_from_root_dir(lockfile_root, dir)); + let (Some(root), Some(sibling)) = (importer(root_dir), importer(sibling_dir)) else { + return false; + }; + let mut seen = std::collections::HashSet::new(); + sibling + .dependencies_by_groups(groups.iter().copied()) + .all(|(alias, _)| { + if !seen.insert(alias) { + return true; + } + let (Some(dep), Some(root_dep)) = + (sole_target(sibling, groups, alias), sole_target(root, groups, alias)) + else { + return false; + }; + resolves_to_same_target(root_dir, root_dep, sibling_dir, dep) + }) +} + +/// The one target `importer` resolves `alias` to across `groups`, or `None` +/// when it declares the alias nowhere or with differing targets. +fn sole_target<'a>( + importer: &'a ProjectSnapshot, + groups: &[DependencyGroup], + alias: &PkgName, +) -> Option<&'a ResolvedDependencySpec> { + let mut declarations = importer + .dependencies_by_groups(groups.iter().copied()) + .filter(|(declared, _)| *declared == alias) + .map(|(_, dep)| dep); + let first = declarations.next()?; + declarations + .all(|dep| dep.version == first.version) + .then_some(first) +} + +/// Whether two importer dependencies resolve to one target: the same +/// snapshot, or `link:` paths that name the same directory once resolved +/// against their own importer directories. +fn resolves_to_same_target( + root_dir: &Path, + root_dep: &ResolvedDependencySpec, + sibling_dir: &Path, + dep: &ResolvedDependencySpec, +) -> bool { + match (root_dep.version.as_link_target(), dep.version.as_link_target()) { + (Some(root_target), Some(target)) => { + lexical_normalize(&root_dir.join(root_target)) + == lexical_normalize(&sibling_dir.join(target)) + } + (None, None) => root_dep.version == dep.version, + _ => false, + } +} /// Recover the workspace root from `config.modules_dir`. The root /// importer's `root_dir` equals `config.modules_dir.parent()` because /// `config.modules_dir` is `/node_modules`. Used by diff --git a/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/lockfile.rs b/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/lockfile.rs index bf42c433cb..cd82a6b6f1 100644 --- a/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/lockfile.rs +++ b/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/lockfile.rs @@ -1,7 +1,6 @@ use super::{ super::{ Decision, OptimisticRepeatInstallCheck, check_optimistic_repeat_install, - conflict_markers::MAX_LOCKFILE_CONFLICT_SCAN_BYTES, deps_status::{RunDepsStatus, check_deps_status_before_run}, settings::current_settings, timestamps::{FileMtime, lockfile_modified_since, modified_at_or_after}, @@ -286,16 +285,35 @@ fn returns_skipped_without_following_a_lockfile_symlink() { Decision::Skipped { reason } if reason.contains("cannot be checked") )); } +/// A changed lockfile is scanned whole, however large: every one that +/// passes the scan is parsed in full next, so no size budget can save +/// anything. Here the file is a valid lockfile padded past 16 MiB with a +/// comment. #[test] -fn returns_skipped_without_scanning_an_oversized_changed_lockfile() { +fn scans_a_large_changed_lockfile_to_the_end() { let (dir, config, manifest) = setup_fresh_install(pnpm_config::NodeLinker::Isolated, "root", "1.0.0", ""); - let lockfile = fs::OpenOptions::new() - .write(true) - .truncate(true) - .open(dir.path().join(Lockfile::FILE_NAME)) - .expect("open lockfile"); - lockfile.set_len(MAX_LOCKFILE_CONFLICT_SCAN_BYTES).expect("resize lockfile"); + fs::write(dir.path().join(Lockfile::FILE_NAME), large_lockfile(None)).expect("write lockfile"); + let lockfile = Lockfile::load_wanted_from_dir(dir.path()) + .expect("parse the padded lockfile") + .expect("padded lockfile on disk"); + + let decision = check_with_lockfile( + dir.path(), + config, + pnpm_config::NodeLinker::Isolated, + &[(dir.path().to_path_buf(), &manifest)], + &lockfile, + ); + + assert_eq!(decision, Decision::UpToDate); +} +#[test] +fn finds_a_conflict_marker_deep_in_a_large_changed_lockfile() { + let (dir, config, manifest) = + setup_fresh_install(pnpm_config::NodeLinker::Isolated, "root", "1.0.0", ""); + fs::write(dir.path().join(Lockfile::FILE_NAME), large_lockfile(Some("<<<<<<< HEAD\n"))) + .expect("write lockfile"); let decision = check( dir.path(), @@ -304,11 +322,20 @@ fn returns_skipped_without_scanning_an_oversized_changed_lockfile() { &[(dir.path().to_path_buf(), &manifest)], ); - dbg!(&decision); - assert!(matches!( - decision, - Decision::Skipped { reason } if reason.contains("cannot be checked") - )); + assert!(matches!(decision, Decision::Skipped { reason } if reason.contains("conflict"))); +} +/// An empty lockfile followed by 17 MiB of comment lines, then `tail`. +fn large_lockfile(tail: Option<&str>) -> String { + const COMMENT_LINE: &str = + "# padding padding padding padding padding padding padding padding\n"; + let mut lockfile = String::from("lockfileVersion: '9.0'\n"); + let target = 17 * 1024 * 1024; + lockfile.reserve(target + COMMENT_LINE.len()); + while lockfile.len() < target { + lockfile.push_str(COMMENT_LINE); + } + lockfile.push_str(tail.unwrap_or_default()); + lockfile } #[test] fn returns_skipped_when_current_lockfile_missing_for_non_empty_wanted_lockfile() { diff --git a/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/workspace.rs b/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/workspace.rs index 77690ab0e0..7e4ced1e81 100644 --- a/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/workspace.rs +++ b/pnpm/crates/package-manager/src/optimistic_repeat_install/tests/workspace.rs @@ -6,10 +6,12 @@ use super::{ FOO_MANIFEST, assert_content_check_converges_after_collision, backdate_validated_files, check, check_with_lockfile, collide_mtimes_with_recorded_state, content_check_decision, isolated_included, linked_sibling_decision_for_spec, setup_content_check_project, - setup_fresh_install, validate_existing_files, write_local_tarball_lockfile, write_state, + setup_fresh_install, setup_fresh_install_with_config, validate_existing_files, + write_local_tarball_lockfile, write_state, }; use pnpm_config::Config; -use pnpm_lockfile::MaybeLazyLockfile; +use pnpm_lockfile::{Lockfile, MaybeLazyLockfile}; +use pnpm_modules_yaml::IncludedDependencies; use pnpm_package_manifest::PackageManifest; use pnpm_testing_utils::fs::set_mtime; use pnpm_workspace_state::{ProjectEntry, load_workspace_state, update_workspace_state}; @@ -517,3 +519,206 @@ fn returns_up_to_date_for_registry_resolution_when_workspace_linking_is_off() { Decision::UpToDate, ); } +/// Under `dedupeDirectDeps` a sibling whose direct dependencies resolve to +/// the root's targets gets nothing linked and no modules directory, so the +/// missing directory is not evidence of a missing install. +#[test] +fn returns_up_to_date_when_a_deduped_sibling_has_no_node_modules() { + assert_eq!(deduped_sibling_decision(true, "1.0.0", "1.0.0"), Decision::UpToDate); +} +#[test] +fn returns_skipped_when_a_sibling_without_dedupe_has_no_node_modules() { + let decision = deduped_sibling_decision(false, "1.0.0", "1.0.0"); + assert!(matches!(decision, Decision::Skipped { reason } if reason.contains("node_modules"))); +} +/// The same specifier can resolve to another peer set for the sibling; the +/// linker then links it into the sibling, so its missing `node_modules` is +/// real damage. +#[test] +fn returns_skipped_when_a_sibling_resolves_a_shared_specifier_to_another_peer_set() { + let decision = deduped_sibling_decision(true, "1.0.0", "1.0.0(bar@1.0.0)"); + assert!(matches!(decision, Decision::Skipped { reason } if reason.contains("node_modules"))); +} +/// `link:` targets are compared where they point, not as strings: the root's +/// `link:libs/lib` and the sibling's `link:../libs/lib` are one directory. +#[test] +fn returns_up_to_date_when_a_deduped_sibling_links_the_same_directory_by_another_path() { + assert_eq!( + deduped_sibling_decision(true, "link:libs/lib", "link:../libs/lib"), + Decision::UpToDate, + ); +} +#[test] +fn returns_skipped_when_a_sibling_links_another_directory_under_the_same_specifier() { + let decision = deduped_sibling_decision(true, "link:libs/lib", "link:libs/lib"); + assert!(matches!(decision, Decision::Skipped { reason } if reason.contains("node_modules"))); +} +/// The root declares `foo` in two groups with one target: still one target, +/// so the sibling matches it. +#[test] +fn returns_up_to_date_when_the_root_declares_the_alias_in_two_groups_with_one_target() { + assert_eq!( + deduped_sibling_decision_with_root_dev("1.0.0", Some("1.0.0"), "1.0.0"), + Decision::UpToDate, + ); +} +/// Two root declarations with differing targets have one effective target +/// the linker picks by group order; the check does not reproduce that +/// choice and falls through. +#[test] +fn returns_skipped_when_the_root_declares_the_alias_with_differing_targets() { + let decision = deduped_sibling_decision_with_root_dev("1.0.0", Some("2.0.0"), "2.0.0"); + assert!(matches!(decision, Decision::Skipped { reason } if reason.contains("node_modules"))); +} +/// A production-only install never links dev dependencies, so a sibling dev +/// dependency the root lacks does not make the sibling incomplete. +#[test] +fn returns_up_to_date_when_an_unmatched_dependency_is_in_an_excluded_group() { + let production_only = IncludedDependencies { + dependencies: true, + dev_dependencies: false, + optional_dependencies: false, + }; + assert_eq!( + deduped_sibling_decision_in(DedupedSibling { + dedupe_direct_deps: true, + root_version: "1.0.0", + root_dev_version: None, + sibling_version: "1.0.0", + sibling_dev_bar_version: Some("2.0.0"), + included: production_only, + }), + Decision::UpToDate, + ); +} +/// The same sibling dev dependency blocks the exemption once dev +/// dependencies are installed. +#[test] +fn returns_skipped_when_an_unmatched_dependency_is_in_an_included_group() { + let decision = deduped_sibling_decision_in(DedupedSibling { + dedupe_direct_deps: true, + root_version: "1.0.0", + root_dev_version: None, + sibling_version: "1.0.0", + sibling_dev_bar_version: Some("2.0.0"), + included: isolated_included(), + }); + assert!(matches!(decision, Decision::Skipped { reason } if reason.contains("node_modules"))); +} +fn deduped_sibling_decision( + dedupe_direct_deps: bool, + root_version: &str, + sibling_version: &str, +) -> Decision { + deduped_sibling_decision_in(DedupedSibling { + dedupe_direct_deps, + root_version, + root_dev_version: None, + sibling_version, + sibling_dev_bar_version: None, + included: isolated_included(), + }) +} +fn deduped_sibling_decision_with_root_dev( + root_version: &str, + root_dev_version: Option<&str>, + sibling_version: &str, +) -> Decision { + deduped_sibling_decision_in(DedupedSibling { + dedupe_direct_deps: true, + root_version, + root_dev_version, + sibling_version, + sibling_dev_bar_version: None, + included: isolated_included(), + }) +} +#[derive(Clone, Copy)] +struct DedupedSibling<'a> { + dedupe_direct_deps: bool, + /// The root's `dependencies.foo`. + root_version: &'a str, + /// The root's `devDependencies.foo`, when it declares one. + root_dev_version: Option<&'a str>, + /// The sibling's `devDependencies.foo`. + sibling_version: &'a str, + /// The sibling's `devDependencies.bar`, which the root never declares. + sibling_dev_bar_version: Option<&'a str>, + included: IncludedDependencies, +} +fn deduped_sibling_decision_in(sibling: DedupedSibling<'_>) -> Decision { + let DedupedSibling { + dedupe_direct_deps, + root_version, + root_dev_version, + sibling_version, + sibling_dev_bar_version, + included, + } = sibling; + let (dir, config, root_manifest) = setup_fresh_install_with_config( + pnpm_config::NodeLinker::Isolated, + "root", + "1.0.0", + r#""dependencies":{"foo":"1.0.0"}"#, + |config| config.dedupe_direct_deps = dedupe_direct_deps, + ); + let sibling_dir = dir.path().join("pkg-a"); + fs::create_dir_all(&sibling_dir).unwrap(); + let sibling_manifest_path = sibling_dir.join("package.json"); + let sibling_bar_manifest = sibling_dev_bar_version.map_or("", |_| r#","bar":"2.0.0""#); + fs::write( + &sibling_manifest_path, + format!( + r#"{{"name":"pkg-a","version":"1.0.0","devDependencies":{{"foo":"1.0.0"{sibling_bar_manifest}}}}}"#, + ), + ) + .unwrap(); + let sibling_manifest = PackageManifest::from_path(sibling_manifest_path).unwrap(); + let root_dev_block = root_dev_version.map_or_else(String::new, |version| { + format!(" devDependencies:\n foo:\n specifier: 1.0.0\n version: {version}\n") + }); + let sibling_bar_block = sibling_dev_bar_version.map_or_else(String::new, |version| { + format!(" bar:\n specifier: 2.0.0\n version: {version}\n") + }); + fs::write( + dir.path().join(Lockfile::FILE_NAME), + format!( + "lockfileVersion: '9.0'\n\nimporters:\n\n .:\n dependencies:\n foo:\n specifier: 1.0.0\n version: {root_version}\n{root_dev_block}\n pkg-a:\n devDependencies:\n foo:\n specifier: 1.0.0\n version: {sibling_version}\n{sibling_bar_block}", + ), + ) + .unwrap(); + let lockfile = Lockfile::load_wanted_from_dir(dir.path()) + .expect("parse the two-importer lockfile") + .expect("lockfile on disk"); + let settings = current_settings(config, pnpm_config::NodeLinker::Isolated, included, None); + let mut projects = BTreeMap::new(); + projects.insert( + dir.path() + .to_string_lossy() + .into_owned(), + ProjectEntry { name: Some("root".into()), version: Some("1.0.0".into()) }, + ); + projects.insert( + sibling_dir.to_string_lossy().into_owned(), + ProjectEntry { name: Some("pkg-a".into()), version: Some("1.0.0".into()) }, + ); + write_state(dir.path(), backdate_validated_files(dir.path()), settings, projects); + + check_optimistic_repeat_install(&OptimisticRepeatInstallCheck { + workspace_root: dir.path(), + config, + project_manifests: &[ + (dir.path().to_path_buf(), &root_manifest), + (sibling_dir, &sibling_manifest), + ], + is_workspace_install: true, + lockfile: MaybeLazyLockfile::Loaded(Some(&lockfile)), + catalogs: &BTreeMap::default(), + layout: crate::RepeatInstallLayout { + node_linker: pnpm_config::NodeLinker::Isolated, + included, + supported_architectures: None, + }, + manifest_freshness: crate::ManifestFreshness::Mtime, + }) +} diff --git a/pnpm11/deps/status/src/checkDepsStatus.ts b/pnpm11/deps/status/src/checkDepsStatus.ts index e10b382a54..c53f10ca1e 100644 --- a/pnpm11/deps/status/src/checkDepsStatus.ts +++ b/pnpm11/deps/status/src/checkDepsStatus.ts @@ -15,8 +15,10 @@ import { getLockfileImporterId, getWantedLockfileName, type LockfileObject, + type ProjectSnapshot, readCurrentLockfile, readWantedLockfile, + type ResolvedDependencies, wantedLockfileHasMergeConflictsSync, } from '@pnpm/lockfile.fs' import { @@ -52,6 +54,7 @@ import { statManifestFile } from './statManifestFile.js' export type CheckDepsStatusOptions = Pick + modulesDirStats?.isDirectory() !== true && !isEmpty({ ...project.manifest.dependencies, ...project.manifest.devDependencies, - })) continue + })) + // Under dedupeDirectDeps a project whose direct dependencies resolve to + // the root's targets gets nothing linked, so the linker never creates + // its modules directory; it is installed all the same. + const rootModulesDirExists = allManifestStats.some(({ modulesDirStats, project }) => + modulesDirStats?.isDirectory() === true && project.rootDir === rootProjectManifestDir) + const dedupeLockfileDir = opts.lockfileDir ?? workspaceDir ?? rootProjectManifestDir + const mayBeDeduped = (project: Project): boolean => + opts.dedupeDirectDeps === true && rootModulesDirExists && project.rootDir !== rootProjectManifestDir + const wantedLockfileForDedupe = withoutModulesDir.some(({ project }) => mayBeDeduped(project)) + ? await readWantedLockfile(dedupeLockfileDir, { + ignoreIncompatible: false, + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + }) + : null + for (const { project } of withoutModulesDir) { + if ( + wantedLockfileForDedupe != null && + mayBeDeduped(project) && + dedupeLinksNothing(wantedLockfileForDedupe, dedupeLockfileDir, rootProjectManifestDir, project.rootDir, opts.include) + ) continue const id = project.manifest.name ?? project.rootDir return { upToDate: false, @@ -980,3 +1003,54 @@ function modifiedAtOrAfter (stats: fs.Stats, referenceMs: number): boolean { const mtimeMs = stats.mtime.valueOf() return wholeSecond ? mtimeMs + 1000 > referenceMs : mtimeMs > referenceMs } + +/** + * Whether `dedupeDirectDeps` links nothing into the project at `projectDir`: + * for every alias the project declares in a materialized group, the wanted + * lockfile records one target on each side, and the two are the same, which + * is what the linker compares. An alias declared with differing targets in + * several groups has one effective target the linker picks by group order; + * that choice is not reproduced here, so such an alias proves nothing. + * Neither does a lockfile that lacks either importer. + */ +function dedupeLinksNothing ( + lockfile: LockfileObject, + lockfileDir: string, + rootDir: string, + projectDir: string, + include?: IncludedDependencies +): boolean { + const root = lockfile.importers[getLockfileImporterId(lockfileDir, rootDir)] + const project = lockfile.importers[getLockfileImporterId(lockfileDir, projectDir)] + if (root == null || project == null) return false + const materializedGroups = (importer: ProjectSnapshot): Array => [ + include?.dependencies === false ? undefined : importer.dependencies, + include?.devDependencies === false ? undefined : importer.devDependencies, + include?.optionalDependencies === false ? undefined : importer.optionalDependencies, + ] + const soleTarget = (importer: ProjectSnapshot, alias: string): string | undefined => { + const versions = materializedGroups(importer) + .map((deps) => deps?.[alias]) + .filter((version): version is string => version != null) + return versions.length > 0 && versions.every((version) => version === versions[0]) ? versions[0] : undefined + } + const aliases = new Set(materializedGroups(project).flatMap((deps) => Object.keys(deps ?? {}))) + return [...aliases].every((alias) => { + const version = soleTarget(project, alias) + const rootVersion = soleTarget(root, alias) + return version != null && rootVersion != null && resolvesToSameTarget(rootDir, rootVersion, projectDir, version) + }) +} + +/** + * Whether two importer dependency versions resolve to one target: the same + * snapshot, or `link:` paths that name the same directory once resolved + * against their own importer directories. + */ +function resolvesToSameTarget (rootDir: string, rootVersion: string, projectDir: string, version: string): boolean { + const rootLink = rootVersion.startsWith('link:') + const projectLink = version.startsWith('link:') + if (rootLink !== projectLink) return false + if (!rootLink) return rootVersion === version + return path.resolve(rootDir, rootVersion.slice('link:'.length)) === path.resolve(projectDir, version.slice('link:'.length)) +} diff --git a/pnpm11/deps/status/test/checkDepsStatus.test.ts b/pnpm11/deps/status/test/checkDepsStatus.test.ts index f090893076..c55654fb58 100644 --- a/pnpm11/deps/status/test/checkDepsStatus.test.ts +++ b/pnpm11/deps/status/test/checkDepsStatus.test.ts @@ -6,7 +6,7 @@ import path from 'node:path' import { beforeEach, describe, expect, it, jest } from '@jest/globals' import type { CheckDepsStatusOptions } from '@pnpm/deps.status' import type { LockfileObject } from '@pnpm/lockfile.fs' -import type { ProjectId, ProjectRootDir, ProjectRootDirRealPath } from '@pnpm/types' +import type { IncludedDependencies, ProjectId, ProjectRootDir, ProjectRootDirRealPath } from '@pnpm/types' import type { WorkspaceState } from '@pnpm/workspace.state' { @@ -485,7 +485,8 @@ describe('checkDepsStatus - pnpmfile modification', () => { return { mtime: new Date(beforeLastValidation), mtimeMs: beforeLastValidation, - } as Stats + isDirectory: () => true, + } as unknown as Stats }) jest.mocked(statManifestFileUtils.statManifestFile).mockImplementation(async () => ({ mtime: new Date(beforeLastValidation), @@ -1777,3 +1778,187 @@ describe('checkDepsStatus - workspace discovery', () => { } }) }) + +describe('checkDepsStatus - deduped sibling without a modules directory', () => { + beforeEach(() => { + jest.resetModules() + jest.clearAllMocks() + }) + + interface DedupedSibling { + dedupeDirectDeps: boolean + /** The root's `dependencies.foo` as the lockfile resolved it. */ + rootVersion?: string + /** The root's `devDependencies.foo`, when it declares one. */ + rootDevVersion?: string + /** The sibling's `devDependencies.foo` as the lockfile resolved it. */ + siblingVersion?: string + /** The sibling's `devDependencies.bar`, which the root never declares. */ + siblingDevBarVersion?: string + include?: IncludedDependencies + } + + // A root and a sibling that both declare foo; only the root has a + // node_modules directory, which is what dedupeDirectDeps leaves behind. + async function checkWithDedupe ({ + dedupeDirectDeps, + rootVersion = '1.0.0', + rootDevVersion, + siblingVersion = '1.0.0', + siblingDevBarVersion, + include, + }: DedupedSibling) { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), 'pnpm-check-deps-dedupe-')) + try { + const lastValidatedTimestamp = Date.now() - 10_000 + const beforeLastValidation = lastValidatedTimestamp - 10_000 + const rootDir = workspaceDir as ProjectRootDir + const rootDirRealPath = await fs.realpath(workspaceDir) as ProjectRootDirRealPath + const siblingDir = path.join(workspaceDir, 'pkg-a') as ProjectRootDir + const rootManifest = { name: 'root', version: '1.0.0', dependencies: { foo: '1.0.0' } } + const siblingManifest = { + name: 'pkg-a', + version: '1.0.0', + devDependencies: { foo: '1.0.0', ...(siblingDevBarVersion == null ? {} : { bar: '2.0.0' }) }, + } + const mockWorkspaceState: WorkspaceState = { + lastValidatedTimestamp, + pnpmfiles: [], + settings: { + dedupeDirectDeps, + excludeLinksFromLockfile: false, + linkWorkspacePackages: true, + preferWorkspacePackages: true, + }, + projects: { + [rootDir]: { name: 'root', version: '1.0.0' }, + [siblingDir]: { name: 'pkg-a', version: '1.0.0' }, + }, + filteredInstall: false, + } + const lockfilePath = path.join(workspaceDir, 'pnpm-lock.yaml') + await fs.writeFile(lockfilePath, "lockfileVersion: '9.0'\n") + await fs.utimes(lockfilePath, beforeLastValidation / 1000, beforeLastValidation / 1000) + + const beforeValidation = { + mtime: new Date(beforeLastValidation), + mtimeMs: beforeLastValidation, + isDirectory: () => true, + } as unknown as Stats + jest.mocked(loadWorkspaceState).mockReturnValue(mockWorkspaceState) + jest.mocked(fsUtils.safeStatSync).mockImplementation((filePath: string) => + filePath.endsWith('pnpm-lock.yaml') ? beforeValidation : undefined) + jest.mocked(fsUtils.safeStat).mockImplementation(async (filePath: string) => { + if (filePath === path.join(workspaceDir, 'node_modules')) return beforeValidation + if (filePath.endsWith('pnpm-lock.yaml')) return beforeValidation + return undefined + }) + jest.mocked(statManifestFileUtils.statManifestFile).mockResolvedValue(beforeValidation) + const lockfile: LockfileObject = { + lockfileVersion: '9.0', + importers: { + ['.' as ProjectId]: { + specifiers: { foo: '1.0.0' }, + dependencies: { foo: rootVersion }, + ...(rootDevVersion == null ? {} : { devDependencies: { foo: rootDevVersion } }), + }, + ['pkg-a' as ProjectId]: { + specifiers: { foo: '1.0.0' }, + devDependencies: { + foo: siblingVersion, + ...(siblingDevBarVersion == null ? {} : { bar: siblingDevBarVersion }), + }, + }, + }, + } + jest.mocked(lockfileFs.readWantedLockfile).mockResolvedValue(lockfile) + jest.mocked(lockfileFs.readCurrentLockfile).mockResolvedValue(lockfile) + + const opts: CheckDepsStatusOptions = { + allProjects: [ + { rootDir, rootDirRealPath, manifest: rootManifest, writeProjectManifest: async () => {} }, + { + rootDir: siblingDir, + rootDirRealPath: siblingDir as unknown as ProjectRootDirRealPath, + manifest: siblingManifest, + writeProjectManifest: async () => {}, + }, + ], + workspaceDir, + rootProjectManifest: rootManifest, + rootProjectManifestDir: workspaceDir, + pnpmfile: [], + include, + ...mockWorkspaceState.settings, + } + return await checkDepsStatus(opts) + } finally { + await fs.rm(workspaceDir, { force: true, recursive: true }) + } + } + + const MISSING_MODULES_DIR = 'Workspace package pkg-a has dependencies but does not have a modules directory' + + it('is up to date when dedupeDirectDeps left the sibling nothing to link', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true }) + expect(result.issue).toBeUndefined() + expect(result.upToDate).toBe(true) + }) + + it('is outdated when the sibling was not deduped', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: false }) + expect(result.upToDate).toBe(false) + expect(result.issue).toBe(MISSING_MODULES_DIR) + }) + + // The same specifier resolved to another peer set for the sibling: the + // linker links it into the sibling, so the missing directory is real damage. + it('is outdated when the shared specifier resolves to another peer set for the sibling', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true, siblingVersion: '1.0.0(bar@1.0.0)' }) + expect(result.upToDate).toBe(false) + expect(result.issue).toBe(MISSING_MODULES_DIR) + }) + + // link: targets are compared where they point, not as strings. + it('is up to date when the sibling links the same directory by another relative path', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true, rootVersion: 'link:libs/lib', siblingVersion: 'link:../libs/lib' }) + expect(result.issue).toBeUndefined() + expect(result.upToDate).toBe(true) + }) + + it('is outdated when equal link strings point at different directories', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true, rootVersion: 'link:libs/lib', siblingVersion: 'link:libs/lib' }) + expect(result.upToDate).toBe(false) + expect(result.issue).toBe(MISSING_MODULES_DIR) + }) + + it('is up to date when the root declares the alias in two groups with one target', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true, rootDevVersion: '1.0.0' }) + expect(result.issue).toBeUndefined() + expect(result.upToDate).toBe(true) + }) + + // Two root declarations with differing targets have one effective target + // the linker picks by group order; the check does not reproduce that choice. + it('is outdated when the root declares the alias with differing targets', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true, rootDevVersion: '2.0.0', siblingVersion: '2.0.0' }) + expect(result.upToDate).toBe(false) + expect(result.issue).toBe(MISSING_MODULES_DIR) + }) + + it('ignores a dependency in a group the install does not materialize', async () => { + const result = await checkWithDedupe({ + dedupeDirectDeps: true, + siblingDevBarVersion: '2.0.0', + include: { dependencies: true, devDependencies: false, optionalDependencies: false }, + }) + expect(result.issue).toBeUndefined() + expect(result.upToDate).toBe(true) + }) + + it('is outdated when a dependency in a materialized group has no root counterpart', async () => { + const result = await checkWithDedupe({ dedupeDirectDeps: true, siblingDevBarVersion: '2.0.0' }) + expect(result.upToDate).toBe(false) + expect(result.issue).toBe(MISSING_MODULES_DIR) + }) +})