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) + }) +})