diff --git a/.changeset/hoisted-interrupted-install-reuse.md b/.changeset/hoisted-interrupted-install-reuse.md new file mode 100644 index 0000000000..bd7e22b524 --- /dev/null +++ b/.changeset/hoisted-interrupted-install-reuse.md @@ -0,0 +1,7 @@ +--- +"@pnpm/installing.deps-restorer": patch +"pnpm": patch +"pacquet": patch +--- + +With `nodeLinker: hoisted`, an install that stops partway no longer leaves the next install trusting the packages it was replacing. Before, reverting the change that started the stopped install, such as a patch edit, kept the half-replaced package in `node_modules`. diff --git a/pnpm/crates/cli/tests/suite/hoisted_node_linker/repeat_install.rs b/pnpm/crates/cli/tests/suite/hoisted_node_linker/repeat_install.rs index 273d7f9be6..4554e4ad4e 100644 --- a/pnpm/crates/cli/tests/suite/hoisted_node_linker/repeat_install.rs +++ b/pnpm/crates/cli/tests/suite/hoisted_node_linker/repeat_install.rs @@ -982,3 +982,66 @@ fn removing_a_linked_dependency_does_not_unlink_through_a_symlinked_scope() { drop((root, mock_instance)); } + +/// An install that stops while the linker replaces a recorded location must +/// not leave that location recorded, or the next install would trust whatever +/// the interrupted import left there. +#[cfg(unix)] +#[test] +fn an_interrupted_hoisted_install_forgets_the_locations_it_replaces() { + use std::os::unix::fs::PermissionsExt; + const REPLACED: &str = "@pnpm.e2e/dep-of-pkg-with-1-dep"; + let CommandTempCwd { + pacquet, + root, + workspace, + npmrc_info, + .. + } = CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + write_manifest(&workspace, serde_json::json!({ REPLACED: "100.0.0", "is-positive": "1.0.0" })); + write_workspace_yaml(&workspace, "nodeLinker: hoisted\noptimisticRepeatInstall: false\n"); + pacquet + .with_args(["install"]) + .assert() + .success(); + let replaced = DirWitness::take(&workspace.join("node_modules").join(REPLACED)); + + // A read-only scope directory stops the linker before it replaces the package. + let scope_dir = workspace.join("node_modules/@pnpm.e2e"); + let set_mode = |mode: u32| { + fs::set_permissions(&scope_dir, fs::Permissions::from_mode(mode)) + .expect("set the scope dir mode"); + }; + write_manifest(&workspace, serde_json::json!({ REPLACED: "100.1.0", "is-positive": "1.0.0" })); + set_mode(0o555); + let output = pacquet_at(&workspace) + .with_args(["install"]) + .output() + .expect("run pnpm install"); + set_mode(0o755); + assert!(!output.status.success(), "the install was expected to fail: {output:?}"); + + let hoisted_locations = read_modules_manifest::(&workspace.join("node_modules")) + .expect("read .modules.yaml") + .expect(".modules.yaml exists") + .hoisted_locations + .unwrap_or_default(); + dbg!(&hoisted_locations); + assert!(!hoisted_locations.contains_key(&format!("{REPLACED}@100.0.0"))); + assert_eq!( + hoisted_locations.get("is-positive@1.0.0"), + Some(&vec!["node_modules/is-positive".to_string()]), + ); + + write_manifest(&workspace, serde_json::json!({ REPLACED: "100.0.0", "is-positive": "1.0.0" })); + pacquet_at(&workspace) + .with_args(["install"]) + .assert() + .success(); + assert!(!replaced.is_intact(), "the forgotten location was imported again"); + assert_eq!(read_pkg_version(&workspace, &format!("node_modules/{REPLACED}")), "100.0.0"); + + drop((root, mock_instance)); +} diff --git a/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs b/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs index 218f8a1ddb..95daa22729 100644 --- a/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs +++ b/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs @@ -184,6 +184,12 @@ pub enum InstallFrozenLockfileError { #[diagnostic(transparent)] LinkHoistedModules(#[error(source)] LinkHoistedModulesError), + #[diagnostic(transparent)] + ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError), + + #[diagnostic(transparent)] + WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError), + #[display("failed to write package map: {_0}")] #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))] WritePackageMap(#[error(source)] crate::WritePackageMapError), @@ -235,6 +241,12 @@ impl From for InstallFrozenLockfileError { HoistedLinkerError::SymlinkDirectDependencies(error) => { InstallFrozenLockfileError::SymlinkDirectDependencies(error) } + HoistedLinkerError::ReadModules(error) => { + InstallFrozenLockfileError::ReadModules(error) + } + HoistedLinkerError::WriteModules(error) => { + InstallFrozenLockfileError::WriteModules(error) + } HoistedLinkerError::WritePackageMap(error) => { InstallFrozenLockfileError::WritePackageMap(error) } diff --git a/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted.rs b/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted.rs index 26e0d66a89..5b6db5491b 100644 --- a/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted.rs +++ b/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted.rs @@ -10,9 +10,11 @@ pub use hoist_plan::{ mod direct_links; mod hoist_plan; mod package_root_links; +mod replaced_locations; mod stale_links; mod workspace_links; +use replaced_locations::forget_replaced_hoisted_locations; use stale_links::{unlink_prior_workspace_hoists, unlink_stale_direct_links}; use workspace_links::link_hoisted_workspace_dependencies; @@ -106,6 +108,10 @@ pub enum HoistedLinkerError { HoistSymlink(#[error(source)] SymlinkPackageError), #[diagnostic(transparent)] HoistLinkBins(#[error(source)] pnpm_cmd_shim::LinkBinsError), + #[diagnostic(transparent)] + ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError), + #[diagnostic(transparent)] + WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError), #[display("failed to write package map: {_0}")] #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))] WritePackageMap(#[error(source)] crate::WritePackageMapError), @@ -131,6 +137,7 @@ pub fn run_hoisted_linker( let build_present = inputs.prior.build_present_packages; let unbuilt = inputs.prior.unbuilt_builds; let walked = walk_hoisted_graph(inputs, lockfile, skipped)?; + forget_replaced_hoisted_locations(inputs, &walked)?; unlink_prior_workspace_hoists(inputs)?; unlink_stale_direct_links(inputs)?; let (mut bins, hoisted_dependencies) = diff --git a/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations.rs b/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations.rs new file mode 100644 index 0000000000..6a52e1ce0b --- /dev/null +++ b/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations.rs @@ -0,0 +1,72 @@ +//! Recorded hoisted locations the linker is about to replace, forgotten +//! before it starts. + +#[cfg(test)] +mod tests; + +use super::{HoistedLinkerError, HoistedLinkerInputs}; +use crate::{DependenciesGraph, HoistedLocations}; +use std::path::Path; + +/// Remove from `.modules.yaml` every recorded location this install does not +/// reuse as it is, before the linker replaces or removes it. The walker trusts +/// a recorded location, so an install interrupted after this point must not +/// leave a record of the content it was replacing. Writes nothing when every +/// recorded location is reused. +pub(super) fn forget_replaced_hoisted_locations( + inputs: &HoistedLinkerInputs<'_>, + walked: &crate::hoisted_dep_graph::LockfileToDepGraphResult, +) -> Result<(), HoistedLinkerError> { + let Some(recorded) = inputs.prior.current_hoisted_locations else { return Ok(()) }; + let Some(kept) = + reused_hoisted_locations(recorded, &walked.graph, inputs.projects.walker_lockfile_dir) + else { + return Ok(()); + }; + let modules_dir = &inputs.config.modules_dir; + let Some(mut modules) = + pnpm_modules_yaml::read_modules_manifest::(modules_dir) + .map_err(HoistedLinkerError::ReadModules)? + else { + return Ok(()); + }; + modules.hoisted_locations = (!kept.is_empty()).then_some(kept); + pnpm_modules_yaml::write_modules_manifest::(modules_dir, modules) + .map_err(HoistedLinkerError::WriteModules) +} + +/// The recorded locations `graph` reuses: a present node of the same dep path +/// at that directory. `None` when that is every recorded location. +fn reused_hoisted_locations( + recorded: &HoistedLocations, + graph: &DependenciesGraph, + lockfile_dir: &Path, +) -> Option { + let is_reused = |dep_path: &str, location: &str| { + graph + .get(&lockfile_dir.join(location)) + .is_some_and(|node| node.present && node.package.dep_path.as_str() == dep_path) + }; + // Checked before building the result, so an install that reuses + // everything allocates nothing. + let all_reused = recorded + .iter() + .all(|(dep_path, locations)| { + locations.iter().all(|location| is_reused(dep_path, location)) + }); + if all_reused { + return None; + } + let mut kept = HoistedLocations::new(); + for (dep_path, locations) in recorded { + let reused: Vec = locations + .iter() + .filter(|location| is_reused(dep_path, location)) + .cloned() + .collect(); + if !reused.is_empty() { + kept.insert(dep_path.clone(), reused); + } + } + Some(kept) +} diff --git a/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations/tests.rs b/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations/tests.rs new file mode 100644 index 0000000000..a3d1f7e8b3 --- /dev/null +++ b/pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations/tests.rs @@ -0,0 +1,99 @@ +use super::reused_hoisted_locations; +use crate::{DependenciesGraph, DependenciesGraphNode, HoistedLocations, HoistedPackageMetadata}; +use pnpm_lockfile::{DirectoryResolution, PkgIdWithPatchHash}; +use pnpm_modules_yaml::DepPath; +use pretty_assertions::assert_eq; +use std::{ + collections::{BTreeMap, BTreeSet}, + path::{Path, PathBuf}, +}; + +fn node(dep_path: &str, dir: PathBuf, present: bool) -> DependenciesGraphNode { + DependenciesGraphNode { + package: HoistedPackageMetadata { + dep_path: DepPath::from(dep_path.to_string()), + pkg_id_with_patch_hash: PkgIdWithPatchHash::from(dep_path), + name: "pkg".to_string(), + version: "1.0.0".to_string(), + has_bin: false, + has_bundled_dependencies: false, + patch: None, + resolution: DirectoryResolution { directory: "pkg".to_string() }.into(), + }, + alias: None, + modules: dir + .parent() + .expect("dir has a parent") + .to_path_buf(), + dir, + optional: false, + optional_dependencies: BTreeSet::new(), + present, + children: BTreeMap::new(), + } +} + +/// Locations are recorded with `/` on every platform, so this also covers +/// their lookup on Windows. +fn graph_with(lockfile_dir: &Path, nodes: &[(&str, &str, bool)]) -> DependenciesGraph { + nodes + .iter() + .map(|&(dep_path, location, present)| { + let dir = lockfile_dir.join(location); + (dir.clone(), node(dep_path, dir, present)) + }) + .collect() +} + +fn locations(entries: &[(&str, &[&str])]) -> HoistedLocations { + entries + .iter() + .map(|(dep_path, dirs)| { + ( + dep_path.to_string(), + dirs.iter() + .map(ToString::to_string) + .collect(), + ) + }) + .collect() +} + +#[test] +fn every_location_reused_needs_no_rewrite() { + let lockfile_dir = Path::new("/project"); + let recorded = locations(&[("a@1.0.0", &["node_modules/a", "node_modules/b/node_modules/a"])]); + let graph = graph_with( + lockfile_dir, + &[("a@1.0.0", "node_modules/a", true), ("a@1.0.0", "node_modules/b/node_modules/a", true)], + ); + assert_eq!(reused_hoisted_locations(&recorded, &graph, lockfile_dir), None); +} + +#[test] +fn replaced_and_removed_locations_are_dropped() { + let lockfile_dir = Path::new("/project"); + let recorded = locations(&[ + ("kept@1.0.0", &["node_modules/kept"]), + ("reimported@1.0.0", &["node_modules/reimported"]), + ("taken-over@1.0.0", &["node_modules/taken-over"]), + ("removed@1.0.0", &["node_modules/removed"]), + ("moved@1.0.0", &["node_modules/moved", "node_modules/x/node_modules/moved"]), + ]); + let graph = graph_with( + lockfile_dir, + &[ + ("kept@1.0.0", "node_modules/kept", true), + ("reimported@1.0.0", "node_modules/reimported", false), + ("taken-over@2.0.0", "node_modules/taken-over", true), + ("moved@1.0.0", "node_modules/moved", true), + ], + ); + assert_eq!( + reused_hoisted_locations(&recorded, &graph, lockfile_dir), + Some(locations(&[ + ("kept@1.0.0", &["node_modules/kept"]), + ("moved@1.0.0", &["node_modules/moved"]), + ])), + ); +} diff --git a/pnpm/crates/deps-restorer/src/linking.rs b/pnpm/crates/deps-restorer/src/linking.rs index f3d03d6e7a..12957f005a 100644 --- a/pnpm/crates/deps-restorer/src/linking.rs +++ b/pnpm/crates/deps-restorer/src/linking.rs @@ -53,6 +53,10 @@ pub enum LinkPhaseError { HoistedDepGraph(#[error(source)] crate::HoistedDepGraphError), #[diagnostic(transparent)] LinkHoistedModules(#[error(source)] crate::LinkHoistedModulesError), + #[diagnostic(transparent)] + ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError), + #[diagnostic(transparent)] + WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError), #[display("failed to write package map: {_0}")] #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))] WritePackageMap(#[error(source)] crate::WritePackageMapError), @@ -74,6 +78,8 @@ impl From for LinkPhaseError { HoistedLinkerError::SymlinkDirectDependencies(error) => { LinkPhaseError::SymlinkDirectDependencies(error) } + HoistedLinkerError::ReadModules(error) => LinkPhaseError::ReadModules(error), + HoistedLinkerError::WriteModules(error) => LinkPhaseError::WriteModules(error), HoistedLinkerError::WritePackageMap(error) => LinkPhaseError::WritePackageMap(error), HoistedLinkerError::PruneStaleLinks(error) => LinkPhaseError::PruneStaleModules(error), HoistedLinkerError::HoistSymlink(error) => LinkPhaseError::HoistSymlink(error), diff --git a/pnpm/crates/package-manager/src/install_with_fresh_lockfile/errors.rs b/pnpm/crates/package-manager/src/install_with_fresh_lockfile/errors.rs index fa6bc2fde3..ead154a1bb 100644 --- a/pnpm/crates/package-manager/src/install_with_fresh_lockfile/errors.rs +++ b/pnpm/crates/package-manager/src/install_with_fresh_lockfile/errors.rs @@ -64,6 +64,12 @@ pub enum InstallWithFreshLockfileError { #[diagnostic(transparent)] LinkHoistedModules(#[error(source)] crate::LinkHoistedModulesError), + #[diagnostic(transparent)] + ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError), + + #[diagnostic(transparent)] + WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError), + #[display("failed to write package map: {_0}")] #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))] WritePackageMap(#[error(source)] crate::WritePackageMapError), @@ -250,6 +256,12 @@ impl From for InstallWithFre HoistedLinkerError::SymlinkDirectDependencies(error) => { InstallWithFreshLockfileError::SymlinkDirectDependencies(error) } + HoistedLinkerError::ReadModules(error) => { + InstallWithFreshLockfileError::ReadModules(error) + } + HoistedLinkerError::WriteModules(error) => { + InstallWithFreshLockfileError::WriteModules(error) + } HoistedLinkerError::WritePackageMap(error) => { InstallWithFreshLockfileError::WritePackageMap(error) } diff --git a/pnpm11/installing/deps-installer/test/hoistedNodeLinker/install.ts b/pnpm11/installing/deps-installer/test/hoistedNodeLinker/install.ts index 14ae08ebeb..b863680d5f 100644 --- a/pnpm11/installing/deps-installer/test/hoistedNodeLinker/install.ts +++ b/pnpm11/installing/deps-installer/test/hoistedNodeLinker/install.ts @@ -401,3 +401,35 @@ test('installing with hoisted node-linker a package that is a peer dependency of const lockfile = project.readLockfile() expect(lockfile.packages['@pnpm.e2e/peer-of-itself@1.0.0'].peerDependencies).toBeFalsy() }) + +const testOnPosixAsNonRoot = process.platform === 'win32' || process.getuid?.() === 0 ? test.skip : test + +// The next install trusts a recorded location, so an install that stops while +// replacing one must not leave it recorded. +testOnPosixAsNonRoot('an interrupted install forgets the hoisted locations it replaces', async () => { + prepareEmpty() + const manifest = (version: string) => ({ + dependencies: { + '@pnpm.e2e/dep-of-pkg-with-1-dep': version, + 'is-positive': '1.0.0', + }, + }) + await install(manifest('100.0.0'), testDefaults({ nodeLinker: 'hoisted' })) + fs.writeFileSync('node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/witness', '') + + // A read-only scope directory stops the linker before it replaces the package. + fs.chmodSync('node_modules/@pnpm.e2e', 0o555) + try { + await expect(install(manifest('100.1.0'), testDefaults({ nodeLinker: 'hoisted' }))).rejects.toThrow() + } finally { + fs.chmodSync('node_modules/@pnpm.e2e', 0o755) + } + + const { hoistedLocations } = loadJsonFileSync<{ hoistedLocations: Record }>('node_modules/.modules.yaml') + expect(hoistedLocations['@pnpm.e2e/dep-of-pkg-with-1-dep@100.0.0']).toBeUndefined() + expect(hoistedLocations['is-positive@1.0.0']).toStrictEqual([path.join('node_modules', 'is-positive')]) + + await install(manifest('100.0.0'), testDefaults({ nodeLinker: 'hoisted' })) + expect(fs.existsSync('node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/witness')).toBe(false) + expect(loadJsonFileSync<{ version: string }>('node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/package.json').version).toBe('100.0.0') +}) diff --git a/pnpm11/installing/deps-restorer/src/forgetReplacedHoistedLocations.ts b/pnpm11/installing/deps-restorer/src/forgetReplacedHoistedLocations.ts new file mode 100644 index 0000000000..6dc3c3ee87 --- /dev/null +++ b/pnpm11/installing/deps-restorer/src/forgetReplacedHoistedLocations.ts @@ -0,0 +1,31 @@ +import path from 'node:path' + +import type { DependenciesGraph } from '@pnpm/deps.graph-builder' +import { writeModulesManifest } from '@pnpm/installing.modules-yaml' + +import type { HeadlessContext } from './context.js' + +/** + * Removes from `.modules.yaml` every recorded location this install does not + * reuse as it is, before the linker replaces or removes it. The next install + * trusts a recorded location, so an install interrupted after this point must + * not leave a record of the content it was replacing. Writes nothing when every + * recorded location is reused. + */ +export async function forgetReplacedHoistedLocations ({ opts, rootModulesDir }: HeadlessContext, graph: DependenciesGraph): Promise { + const recorded = opts.modulesFile?.hoistedLocations + if (recorded == null) return + const isReused = (depPath: string, location: string): boolean => { + const node = graph[path.join(opts.lockfileDir, location)] + return node != null && node.fetching == null && node.depPath === depPath + } + const kept: Record = {} + let droppedAny = false + for (const [depPath, locations] of Object.entries(recorded)) { + const reused = locations.filter((location) => isReused(depPath, location)) + if (reused.length !== locations.length) droppedAny = true + if (reused.length > 0) kept[depPath] = reused + } + if (!droppedAny) return + await writeModulesManifest(rootModulesDir, { ...opts.modulesFile!, hoistedLocations: kept }) +} diff --git a/pnpm11/installing/deps-restorer/src/linkHoistedLayout.ts b/pnpm11/installing/deps-restorer/src/linkHoistedLayout.ts index 597f12da5f..0240814230 100644 --- a/pnpm11/installing/deps-restorer/src/linkHoistedLayout.ts +++ b/pnpm11/installing/deps-restorer/src/linkHoistedLayout.ts @@ -12,6 +12,7 @@ import type { DependencyManifest, DepPath, HoistedDependencies, ProjectId } from import type { HeadlessContext, HeadlessDepGraph, LinkedDependencies } from './context.js' import { getDirectDependencyAliases } from './directDependencyAliases.js' +import { forgetReplacedHoistedLocations } from './forgetReplacedHoistedLocations.js' import { linkHoistedModules } from './linkHoistedModules.js' import { reportDirectDependencyChanges } from './reportDirectDependencyChanges.js' import { symlinkDirectDependencies } from './symlinkDirectDependencies.js' @@ -28,6 +29,7 @@ export async function linkHoistedLayout (ctx: HeadlessContext, depGraph: Headles if (ctx.skipPostImportLinking) { return { heldBackBinsDirs: [], linkedToRoot: 0, newHoistedDependencies: opts.hoistedDependencies } } + await forgetReplacedHoistedLocations(ctx, depGraph.graph) const hoistsWorkspacePackages = opts.ignorePackageManifest !== true if (hoistsWorkspacePackages) { // Unlinked before the linker writes the tree: a package that takes over