From 39a96abcfcd0198cc75b3e2d54db413a9016a329 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 26 Oct 2026 12:17:47 +0100 Subject: [PATCH] perf(git-fetcher): bundle the manifest into git-hosted store-index rows (#17369) The git fetchers wrote store-index rows with no bundled manifest, so the warm prefetch re-derived `requiresBuild` from the row's files on every install, reading and parsing `package.json` once per git dependency, and the cached git prepare check could read it again. `queue_files_index` now reads the manifest once, derives `requiresBuild` from it and the file list, and stores the normalized manifest in the row, as pnpm v11 does. Rows with a bundled manifest keep their stored `requiresBuild`; only rows written by older releases are re-derived. `normalize_bundled_manifest` moves from `pnpm-tarball` to `pnpm-package-manifest` so the git fetcher can use it. The manifest-plus-files build check moves there too as `requires_build_from_files`, next to a new `read_package_json_from_cas_paths` that `requires_build_from_cas_paths` and the warm git prepare check now share. Closes pnpm/tasks#188 --- .changeset/git-deps-bundled-manifest.md | 5 + .../src/create_virtual_store/warm.rs | 7 +- pnpm/crates/git-fetcher/src/fetcher.rs | 17 +- .../git-fetcher/src/tarball_fetcher/tests.rs | 1 + .../package-manifest/src/build_triggers.rs | 33 +++- .../package-manifest/src/bundled_manifest.rs | 81 ++++++++ pnpm/crates/package-manifest/src/lib.rs | 5 +- pnpm/crates/package-manifest/src/tests.rs | 2 + .../src/tests/bundled_manifest.rs | 169 +++++++++++++++++ pnpm/crates/tarball/src/extract.rs | 4 +- pnpm/crates/tarball/src/extract/manifest.rs | 85 +-------- pnpm/crates/tarball/src/lib.rs | 2 +- pnpm/crates/tarball/src/local_tarball.rs | 6 +- pnpm/crates/tarball/src/prefetch.rs | 22 +-- pnpm/crates/tarball/src/tests.rs | 177 +----------------- 15 files changed, 313 insertions(+), 303 deletions(-) create mode 100644 .changeset/git-deps-bundled-manifest.md create mode 100644 pnpm/crates/package-manifest/src/bundled_manifest.rs create mode 100644 pnpm/crates/package-manifest/src/tests/bundled_manifest.rs diff --git a/.changeset/git-deps-bundled-manifest.md b/.changeset/git-deps-bundled-manifest.md new file mode 100644 index 0000000000..bf8c86a076 --- /dev/null +++ b/.changeset/git-deps-bundled-manifest.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +Warm installs no longer read the `package.json` of every git-hosted dependency. pnpm now stores the manifest of a git-hosted package in the store index when it fetches the package. diff --git a/pnpm/crates/deps-restorer/src/create_virtual_store/warm.rs b/pnpm/crates/deps-restorer/src/create_virtual_store/warm.rs index 38dd14301d..6b924cfbe8 100644 --- a/pnpm/crates/deps-restorer/src/create_virtual_store/warm.rs +++ b/pnpm/crates/deps-restorer/src/create_virtual_store/warm.rs @@ -10,13 +10,12 @@ use super::{ use crate::{CasPathsByPkgId, InstallPackageBySnapshotError}; use pnpm_git_fetcher::{GitFetcherError, resolve_package_build_permission}; use pnpm_lockfile::{LockfileResolution, PackageKey, PackageMetadata, PkgName}; -use pnpm_package_manifest::parse_manifest; +use pnpm_package_manifest::read_package_json_from_cas_paths; use pnpm_reporter::Reporter; use pnpm_tarball::PrefetchResult; use std::{ borrow::Cow, collections::{HashMap, HashSet}, - fs, path::{Path, PathBuf}, sync::Arc, }; @@ -102,9 +101,7 @@ pub(super) fn cached_git_manifest<'a>( if let Some(manifest) = prefetch.manifests.get(key) { return Some(Cow::Borrowed(manifest.as_ref())); } - let package_json = cas_paths.get("package.json")?; - let contents = fs::read_to_string(package_json).ok()?; - parse_manifest(&contents).ok().map(Cow::Owned) + read_package_json_from_cas_paths(cas_paths).map(Cow::Owned) } pub(super) fn is_git_hosted_resolution(resolution: &LockfileResolution) -> bool { match resolution { diff --git a/pnpm/crates/git-fetcher/src/fetcher.rs b/pnpm/crates/git-fetcher/src/fetcher.rs index 9867910210..d7d0880be8 100644 --- a/pnpm/crates/git-fetcher/src/fetcher.rs +++ b/pnpm/crates/git-fetcher/src/fetcher.rs @@ -33,8 +33,8 @@ use crate::{ use pnpm_fs_packlist::packlist; use pnpm_network::{redact_and_sanitize, redact_and_sanitize_multiline}; use pnpm_package_manifest::{ - requires_build_from_cas_paths, safe_read_package_json_from_dir, - safe_read_project_manifest_from_dir, + normalize_bundled_manifest, read_package_json_from_cas_paths, requires_build_from_files, + safe_read_package_json_from_dir, safe_read_project_manifest_from_dir, }; use pnpm_reporter::Reporter; use pnpm_store_dir::{CafsFileInfo, PackageFilesIndex, StoreIndexWriter}; @@ -196,8 +196,9 @@ pub(crate) fn packlist_of(pkg_dir: &Path) -> Result, GitFetcherError /// Queue a `PackageFilesIndex` row so a future install's warm prefetch /// finds the snapshot in `index.db` and skips the fetch and prepare re-run. /// -/// `requiresBuild` is read off the stored files, as for any other package, -/// since every virtual-store slot they are imported into runs that build. +/// The row bundles the package's normalized manifest, and `requiresBuild` is +/// read off it and the stored files, as for any other package, since every +/// virtual-store slot they are imported into runs that build. /// `requires_prepare` is whether the package needs `prepare`. pub(crate) fn queue_files_index( writer: Option<&Arc>, @@ -207,11 +208,15 @@ pub(crate) fn queue_files_index( requires_prepare: bool, ) { if let Some(writer) = writer { + let manifest = read_package_json_from_cas_paths(cas_paths); writer.queue( files_index_file.to_string(), PackageFilesIndex { - manifest: None, - requires_build: Some(requires_build_from_cas_paths(cas_paths)), + requires_build: Some(requires_build_from_files( + manifest.as_ref(), + cas_paths.keys(), + )), + manifest: manifest.as_ref().and_then(normalize_bundled_manifest), requires_prepare: Some(requires_prepare), algo: "sha512".to_string(), files, diff --git a/pnpm/crates/git-fetcher/src/tarball_fetcher/tests.rs b/pnpm/crates/git-fetcher/src/tarball_fetcher/tests.rs index dd8bc76f96..6b4fd252de 100644 --- a/pnpm/crates/git-fetcher/src/tarball_fetcher/tests.rs +++ b/pnpm/crates/git-fetcher/src/tarball_fetcher/tests.rs @@ -415,6 +415,7 @@ async fn writes_index_row_when_writer_provided() { .expect("row must exist at the git-hosted key"); assert_eq!(row.algo, "sha512"); assert_eq!(row.requires_build, Some(false)); + assert_eq!(row.manifest, Some(serde_json::json!({ "name": "x", "version": "1.0.0" }))); let keys: Vec<&str> = row.files .keys() .map(String::as_str) diff --git a/pnpm/crates/package-manifest/src/build_triggers.rs b/pnpm/crates/package-manifest/src/build_triggers.rs index b7cbfe0a90..d9ffd9334b 100644 --- a/pnpm/crates/package-manifest/src/build_triggers.rs +++ b/pnpm/crates/package-manifest/src/build_triggers.rs @@ -166,12 +166,33 @@ where /// script in its `package.json`. #[must_use] pub fn requires_build_from_cas_paths(cas_paths: &HashMap) -> bool { - let mut triggers = files_build_triggers(cas_paths.keys()); - if let Some(package_json) = cas_paths.get("package.json") - && let Ok(contents) = fs::read_to_string(package_json) - && let Ok(manifest) = parse_manifest(&contents) - { - triggers.read_manifest(&manifest); + requires_build_from_files( + read_package_json_from_cas_paths(cas_paths).as_ref(), + cas_paths.keys(), + ) +} + +/// The package's `package.json`, read from the store file `cas_paths` maps it +/// to. `None` when the package has none, or it cannot be read or parsed. +#[must_use] +pub fn read_package_json_from_cas_paths(cas_paths: &HashMap) -> Option { + let contents = fs::read_to_string(cas_paths.get("package.json")?).ok()?; + parse_manifest(&contents).ok() +} + +/// Whether a package with this manifest and these files needs a build pass. +#[must_use] +pub fn requires_build_from_files( + manifest: Option<&Value>, + filenames: Filenames, +) -> bool +where + Filenames: IntoIterator, + Filename: AsRef, +{ + let mut triggers = files_build_triggers(filenames); + if let Some(manifest) = manifest { + triggers.read_manifest(manifest); } triggers.requires_build() } diff --git a/pnpm/crates/package-manifest/src/bundled_manifest.rs b/pnpm/crates/package-manifest/src/bundled_manifest.rs new file mode 100644 index 0000000000..f0642a0dbf --- /dev/null +++ b/pnpm/crates/package-manifest/src/bundled_manifest.rs @@ -0,0 +1,81 @@ +/// Pick the `package.json` fields downstream code actually reads — bin +/// linking, dependency resolution, build-script detection — and discard +/// the rest, keeping only the lifecycle hooks pnpm executes out +/// of `scripts`. +/// +/// The subset exists to bound what lands in `index.db`: a full manifest +/// runs to tens of KB, and msgpackr-records tops out at `0x7f` record +/// slots. +/// +/// `None` rather than an empty object when nothing survives, which +/// would otherwise round-trip as a zero-field record def. +#[must_use] +pub fn normalize_bundled_manifest(value: &serde_json::Value) -> Option { + /// Fields kept verbatim from the source manifest. + /// + /// Order matters for the on-wire byte sequence — msgpackr emits + /// fields in JS object insertion order, and pacquet's encoder + /// follows the [`serde_json::Map`] iteration order — but it + /// does *not* matter for property-access correctness on the + /// pnpm side. The order below matches the field order pnpm + /// emits so a side-by-side byte diff against a pnpm-written + /// row is shallower. + const BUNDLED_MANIFEST_FIELDS: &[&str] = &[ + "bin", + "bundledDependencies", + "bundleDependencies", + "cpu", + "dependencies", + "devDependencies", + "directories", + "engines", + "gypfile", + "libc", + "name", + "optionalDependencies", + "os", + "peerDependencies", + "peerDependenciesMeta", + ]; + const LIFECYCLE_SCRIPTS: &[&str] = &["preinstall", "install", "postinstall"]; + + let serde_json::Value::Object(map) = value else { return None }; + let mut picked = serde_json::Map::new(); + + // pnpm emits `version` first regardless of whether it was first + // in the source object. Version normalization + // via `semver.clean(...)` (pnpm only loose-cleans for the bundled + // row, not for resolution) is intentionally skipped: the inputs + // from a real npm tarball are already semver-clean in practice, + // and pulling `node-semver` into this crate purely for + // this normalization would carry more risk than the deviation it + // closes. + pick_fields(&mut picked, map, &["version"]); + pick_fields(&mut picked, map, BUNDLED_MANIFEST_FIELDS); + + if let Some(serde_json::Value::Object(scripts)) = map.get("scripts") { + let mut sub = serde_json::Map::new(); + pick_fields(&mut sub, scripts, LIFECYCLE_SCRIPTS); + if !sub.is_empty() { + picked.insert("scripts".to_string(), serde_json::Value::Object(sub)); + } + } + + if picked.is_empty() { None } else { Some(serde_json::Value::Object(picked)) } +} + +/// Copy the named fields that `source` actually carries, in the order given. +/// A null is treated as absent: pnpm's own row omits it. +fn pick_fields( + picked: &mut serde_json::Map, + source: &serde_json::Map, + keys: &[&str], +) { + for &key in keys { + if let Some(value) = source.get(key) + && !value.is_null() + { + picked.insert(key.to_string(), value.clone()); + } + } +} diff --git a/pnpm/crates/package-manifest/src/lib.rs b/pnpm/crates/package-manifest/src/lib.rs index 395b70cbeb..2c8d7442d1 100644 --- a/pnpm/crates/package-manifest/src/lib.rs +++ b/pnpm/crates/package-manifest/src/lib.rs @@ -11,8 +11,10 @@ pub use bins::{ pub use build_triggers::{ BINDING_GYP, BuildTriggers, files_build_triggers, files_include_install_scripts, manifest_opts_out_of_gyp_build, manifest_requires_build, pkg_build_triggers, - pkg_requires_build, requires_build_from_cas_paths, stored_requires_build_needs_manifest_check, + pkg_requires_build, read_package_json_from_cas_paths, requires_build_from_cas_paths, + requires_build_from_files, stored_requires_build_needs_manifest_check, }; +pub use bundled_manifest::normalize_bundled_manifest; pub use error::PackageManifestError; pub use initialization::{InitAuthor, InitOptions}; pub use metadata::{extract_author, extract_homepage, extract_license}; @@ -46,6 +48,7 @@ use tempfile::NamedTempFile; mod bins; mod blank_lines; mod build_triggers; +mod bundled_manifest; mod error; mod initialization; mod json5; diff --git a/pnpm/crates/package-manifest/src/tests.rs b/pnpm/crates/package-manifest/src/tests.rs index 3017354484..7af16da1e1 100644 --- a/pnpm/crates/package-manifest/src/tests.rs +++ b/pnpm/crates/package-manifest/src/tests.rs @@ -98,3 +98,5 @@ mod lockfile; mod yaml; mod json5; + +mod bundled_manifest; diff --git a/pnpm/crates/package-manifest/src/tests/bundled_manifest.rs b/pnpm/crates/package-manifest/src/tests/bundled_manifest.rs new file mode 100644 index 0000000000..62cb0e1b32 --- /dev/null +++ b/pnpm/crates/package-manifest/src/tests/bundled_manifest.rs @@ -0,0 +1,169 @@ +//! Pacquet's [`normalize_bundled_manifest`] picks the subset of +//! `package.json` fields downstream install code reads (bin lookup, +//! peer extraction, build-script detection) and narrows `scripts` to +//! the lifecycle hooks. Two cases are intentionally NOT covered: +//! `semver.clean` normalization (pacquet keeps version verbatim, per +//! the function's doc comment) and the missing-version default of +//! `0.0.0` (pacquet leaves the field absent rather than synthesizing +//! one). + +use crate::normalize_bundled_manifest; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn returns_none_for_empty_manifest() { + assert_eq!(normalize_bundled_manifest(&json!({})), None); +} + +#[test] +fn returns_none_for_non_object() { + assert_eq!(normalize_bundled_manifest(&json!("not an object")), None); + assert_eq!(normalize_bundled_manifest(&json!(null)), None); + assert_eq!(normalize_bundled_manifest(&json!(42)), None); +} + +#[test] +fn returns_none_when_manifest_has_only_excluded_fields() { + assert_eq!( + normalize_bundled_manifest(&json!({ + "description": "a package", + "keywords": ["test"], + "license": "MIT", + "author": "test", + "repository": "test/test", + })), + None, + ); +} + +#[test] +fn picks_included_fields_and_excludes_others() { + let result = normalize_bundled_manifest(&json!({ + "name": "foo", + "version": "1.0.0", + "description": "should be excluded", + "license": "MIT", + "bin": { "foo": "./bin/foo.js" }, + "engines": { "node": ">=18" }, + "cpu": ["x64"], + "os": ["linux"], + "libc": ["glibc"], + "dependencies": { "bar": "^1.0.0" }, + "devDependencies": { "qux": "^3.0.0" }, + "optionalDependencies": { "baz": "^2.0.0" }, + "peerDependencies": { "react": "^18" }, + "peerDependenciesMeta": { "react": { "optional": true } }, + "bundledDependencies": ["bar"], + "directories": { "bin": "./bin" }, + })) + .expect("non-empty pick"); + let map = result.as_object().expect("object"); + assert_eq!(map.get("name").and_then(|v| v.as_str()), Some("foo")); + assert_eq!(map.get("version").and_then(|v| v.as_str()), Some("1.0.0")); + assert_eq!(map.get("bin"), Some(&json!({ "foo": "./bin/foo.js" }))); + assert_eq!(map.get("engines"), Some(&json!({ "node": ">=18" }))); + assert_eq!(map.get("cpu"), Some(&json!(["x64"]))); + assert_eq!(map.get("os"), Some(&json!(["linux"]))); + assert_eq!(map.get("libc"), Some(&json!(["glibc"]))); + assert_eq!(map.get("dependencies"), Some(&json!({ "bar": "^1.0.0" }))); + assert_eq!(map.get("devDependencies"), Some(&json!({ "qux": "^3.0.0" }))); + assert_eq!(map.get("optionalDependencies"), Some(&json!({ "baz": "^2.0.0" }))); + assert_eq!(map.get("peerDependencies"), Some(&json!({ "react": "^18" }))); + assert_eq!(map.get("peerDependenciesMeta"), Some(&json!({ "react": { "optional": true } }))); + assert_eq!(map.get("bundledDependencies"), Some(&json!(["bar"]))); + assert_eq!(map.get("directories"), Some(&json!({ "bin": "./bin" }))); + // Excluded fields stay out. + assert!(map.get("description").is_none()); + assert!(map.get("license").is_none()); + assert!(map.get("keywords").is_none()); +} + +#[test] +fn only_picks_lifecycle_scripts_not_all_scripts() { + let result = normalize_bundled_manifest(&json!({ + "name": "foo", + "version": "1.0.0", + "scripts": { + "preinstall": "echo pre", + "install": "echo install", + "postinstall": "echo post", + "test": "jest", + "build": "tsc", + "start": "node index.js", + "prepare": "tsc", + }, + })) + .expect("non-empty pick"); + assert_eq!( + result.get("scripts").expect("scripts present"), + &json!({ + "preinstall": "echo pre", + "install": "echo install", + "postinstall": "echo post", + }), + ); +} + +#[test] +fn omits_scripts_key_when_no_lifecycle_scripts_exist() { + let result = normalize_bundled_manifest(&json!({ + "name": "foo", + "version": "1.0.0", + "scripts": { + "test": "jest", + "build": "tsc", + }, + })) + .expect("non-empty pick"); + assert!( + result.get("scripts").is_none(), + "scripts key must be absent when no lifecycle hook is present", + ); +} + +/// `null` and `undefined` fields are skipped. Rust's +/// [`serde_json::Value`] has no `undefined`, but JSON `null` +/// reaches the picker as [`serde_json::Value::Null`] and must be +/// filtered out the same way. +#[test] +fn skips_null_fields() { + let result = normalize_bundled_manifest(&json!({ + "name": "foo", + "version": "1.0.0", + "bin": null, + "engines": null, + })) + .expect("non-empty pick"); + assert!(result.get("bin").is_none(), "null `bin` must be dropped"); + assert!(result.get("engines").is_none(), "null `engines` must be dropped"); + assert_eq!(result.get("name").and_then(|v| v.as_str()), Some("foo")); + assert_eq!(result.get("version").and_then(|v| v.as_str()), Some("1.0.0")); +} + +/// The bundled manifest is downstream-fed into +/// `extract_peer_dependencies` and `extract_children`; dropping +/// `peerDependenciesMeta` or `optionalDependencies` here would +/// replicate the pnpm/pnpm#11934 resolver-side bug on the +/// install-side. Pin the keys explicitly. +#[test] +fn preserves_optional_dependencies_and_peer_dependencies_meta_keys() { + let result = normalize_bundled_manifest(&json!({ + "name": "consumer", + "version": "1.0.0", + "optionalDependencies": { "sharp": "^0.34.0" }, + "peerDependenciesMeta": { + "@vercel/kv": { "optional": true }, + "ioredis": { "optional": true }, + }, + })) + .expect("non-empty pick"); + assert_eq!(result.get("optionalDependencies"), Some(&json!({ "sharp": "^0.34.0" }))); + assert_eq!( + result.get("peerDependenciesMeta"), + Some(&json!({ + "@vercel/kv": { "optional": true }, + "ioredis": { "optional": true }, + })), + ); +} diff --git a/pnpm/crates/tarball/src/extract.rs b/pnpm/crates/tarball/src/extract.rs index a1990462ff..daf73ae9dd 100644 --- a/pnpm/crates/tarball/src/extract.rs +++ b/pnpm/crates/tarball/src/extract.rs @@ -13,9 +13,7 @@ pub(crate) use limits::{ clean_archive_entry_path, oversized_manifest_error, }; pub use manifest::{TarInspection, inspect_tar, read_buffered_tar_entry, tar_requires_build}; -pub(crate) use manifest::{ - apply_append_manifest, apply_placeholder_manifest, normalize_bundled_manifest, -}; +pub(crate) use manifest::{apply_append_manifest, apply_placeholder_manifest}; pub(crate) use streaming::{ BodyChunkReceiver, BodyChunkSender, ExtractedResource, STREAM_ENTRY_BUFFER_MAX, StreamedResource, body_chunk_channel, extract_resource_stream, stream_extract_channel, diff --git a/pnpm/crates/tarball/src/extract/manifest.rs b/pnpm/crates/tarball/src/extract/manifest.rs index 800b1deab9..ca4187325b 100644 --- a/pnpm/crates/tarball/src/extract/manifest.rs +++ b/pnpm/crates/tarball/src/extract/manifest.rs @@ -1,8 +1,8 @@ use std::collections::{BTreeMap, BTreeSet}; use pnpm_package_manifest::{ - PackageBin, ScriptRuntime, manifest_declares_bins, parse_shebang_from_bytes, - resolve_package_bins_with, + PackageBin, ScriptRuntime, manifest_declares_bins, normalize_bundled_manifest, + parse_shebang_from_bytes, resolve_package_bins_with, }; use super::{ @@ -36,87 +36,6 @@ pub fn read_buffered_tar_entry( Ok(data) } -/// Pick the `package.json` fields downstream code actually reads — bin -/// linking, dependency resolution, build-script detection — and discard -/// the rest, keeping only the lifecycle hooks pnpm executes out -/// of `scripts`. -/// -/// The subset exists to bound what lands in `index.db`: a full manifest -/// runs to tens of KB, and msgpackr-records tops out at `0x7f` record -/// slots (see [`pnpm_store_dir::EncodeError::OutOfRecordSlots`]). -/// -/// `None` rather than an empty object when nothing survives, which -/// would otherwise round-trip as a zero-field record def. -pub(crate) fn normalize_bundled_manifest(value: &serde_json::Value) -> Option { - /// Fields kept verbatim from the source manifest. - /// - /// Order matters for the on-wire byte sequence — msgpackr emits - /// fields in JS object insertion order, and pacquet's encoder - /// follows the [`serde_json::Map`] iteration order — but it - /// does *not* matter for property-access correctness on the - /// pnpm side. The order below matches the field order pnpm - /// emits so a side-by-side byte diff against a pnpm-written - /// row is shallower. - const BUNDLED_MANIFEST_FIELDS: &[&str] = &[ - "bin", - "bundledDependencies", - "bundleDependencies", - "cpu", - "dependencies", - "devDependencies", - "directories", - "engines", - "gypfile", - "libc", - "name", - "optionalDependencies", - "os", - "peerDependencies", - "peerDependenciesMeta", - ]; - const LIFECYCLE_SCRIPTS: &[&str] = &["preinstall", "install", "postinstall"]; - - let serde_json::Value::Object(map) = value else { return None }; - let mut picked = serde_json::Map::new(); - - // pnpm emits `version` first regardless of whether it was first - // in the source object. Version normalization - // via `semver.clean(...)` (pnpm only loose-cleans for the bundled - // row, not for resolution) is intentionally skipped: the inputs - // from a real npm tarball are already semver-clean in practice, - // and pulling `node-semver` into `pnpm-tarball` purely for - // this normalization would carry more risk than the deviation it - // closes. - pick_fields(&mut picked, map, &["version"]); - pick_fields(&mut picked, map, BUNDLED_MANIFEST_FIELDS); - - if let Some(serde_json::Value::Object(scripts)) = map.get("scripts") { - let mut sub = serde_json::Map::new(); - pick_fields(&mut sub, scripts, LIFECYCLE_SCRIPTS); - if !sub.is_empty() { - picked.insert("scripts".to_string(), serde_json::Value::Object(sub)); - } - } - - if picked.is_empty() { None } else { Some(serde_json::Value::Object(picked)) } -} - -/// Copy the named fields that `source` actually carries, in the order given. -/// A null is treated as absent: pnpm's own row omits it. -fn pick_fields( - picked: &mut serde_json::Map, - source: &serde_json::Map, - keys: &[&str], -) { - for &key in keys { - if let Some(value) = source.get(key) - && !value.is_null() - { - picked.insert(key.to_string(), value.clone()); - } - } -} - /// Fold a synthesized `package.json` (pnpm's `appendManifest`) into a /// freshly extracted archive's CAFS output. Runtime archives (Node.js / /// Bun / Deno) carry no `package.json` of their own, so the caller diff --git a/pnpm/crates/tarball/src/lib.rs b/pnpm/crates/tarball/src/lib.rs index 6dff6011b9..1c68c1d5ae 100644 --- a/pnpm/crates/tarball/src/lib.rs +++ b/pnpm/crates/tarball/src/lib.rs @@ -17,7 +17,7 @@ pub(crate) use extract::{ STREAM_EXTRACT_DURING_DOWNLOAD_THRESHOLD, allocate_tarball_buffer, apply_append_manifest, apply_placeholder_manifest, body_chunk_channel, clean_archive_entry_path, decompress_gzip, extract_archive_bytes, extract_resource_stream, is_eager_decode_limit_exceeded, - normalize_bundled_manifest, oversized_manifest_error, tar_entry_payload, + oversized_manifest_error, tar_entry_payload, }; pub use fetch_for_resolution::{ FetchTarballForResolution, NotModifiedResponse, ResolutionStore, ResolvedTarball, diff --git a/pnpm/crates/tarball/src/local_tarball.rs b/pnpm/crates/tarball/src/local_tarball.rs index 56026d228c..8750ba67ce 100644 --- a/pnpm/crates/tarball/src/local_tarball.rs +++ b/pnpm/crates/tarball/src/local_tarball.rs @@ -6,11 +6,11 @@ use super::{ Cursor, HashMap, MAX_UNTRUSTED_PREALLOC_BYTES, Path, PathBuf, Read, TarballError, allocate_tarball_buffer, clean_archive_entry_path, decompress_gzip, io, - is_eager_decode_limit_exceeded, normalize_bundled_manifest, oversized_manifest_error, - post_download_semaphore, tar_entry_payload, verify_tarball_integrity, + is_eager_decode_limit_exceeded, oversized_manifest_error, post_download_semaphore, + tar_entry_payload, verify_tarball_integrity, }; use crate::extraction_task::spawn_extraction; -use pnpm_package_manifest::parse_manifest_bytes; +use pnpm_package_manifest::{normalize_bundled_manifest, parse_manifest_bytes}; use ssri::{Integrity, IntegrityChecker}; use tar::Archive; diff --git a/pnpm/crates/tarball/src/prefetch.rs b/pnpm/crates/tarball/src/prefetch.rs index d51e2982b6..001ca28e46 100644 --- a/pnpm/crates/tarball/src/prefetch.rs +++ b/pnpm/crates/tarball/src/prefetch.rs @@ -12,7 +12,8 @@ use super::{ ParallelIterator, PathBuf, TarballError, }; use pnpm_package_manifest::{ - files_build_triggers, requires_build_from_cas_paths, stored_requires_build_needs_manifest_check, + requires_build_from_cas_paths, requires_build_from_files, + stored_requires_build_needs_manifest_check, }; use pnpm_reporter::{GlobalLog, LogEvent, LogLevel}; use pnpm_store_dir::{ @@ -346,23 +347,6 @@ fn decode_matching_prefetch_entry(cache_key: &str, bytes: &[u8]) -> Option( - manifest: Option<&serde_json::Value>, - filenames: Filenames, -) -> bool -where - Filenames: IntoIterator, - Filename: AsRef, -{ - let mut triggers = files_build_triggers(filenames); - if let Some(manifest) = manifest { - triggers.read_manifest(manifest); - } - triggers.requires_build() -} - /// A git-hosted row (one that records `requiresPrepare`) with no bundled /// manifest derives `requiresBuild` from its files and ignores the stored /// value, which existing stores may hold wrong @@ -381,7 +365,7 @@ fn resolve_row_requires_build( requires_build_from_cas_paths(cas_paths) } Some(stored) => stored, - None => row_requires_build(manifest, cas_paths.keys()), + None => requires_build_from_files(manifest, cas_paths.keys()), } } diff --git a/pnpm/crates/tarball/src/tests.rs b/pnpm/crates/tarball/src/tests.rs index c4ea52173d..1309d07f49 100644 --- a/pnpm/crates/tarball/src/tests.rs +++ b/pnpm/crates/tarball/src/tests.rs @@ -17,8 +17,7 @@ use super::{ apply_append_manifest, apply_placeholder_manifest, decompress_gzip, extract_gzipped_tarball_with_tar, extract_tarball_entries_with_bins, gzip::{bounded_gzip_size_hint, gzip_isize_hint, should_stream_extract}, - is_eager_decode_limit_exceeded, normalize_bundled_manifest, - stream_extract_gzipped_tarball_with_tar, + is_eager_decode_limit_exceeded, stream_extract_gzipped_tarball_with_tar, }, local_tarball::{ allocate_local_tarball_buffer, local_file_tarball_path, open_local_tarball, @@ -381,180 +380,6 @@ impl Read for EndlessReader { } } -/// Pacquet's [`normalize_bundled_manifest`] picks the subset of -/// `package.json` fields downstream install code reads (bin lookup, -/// peer extraction, build-script detection) and narrows `scripts` to -/// the lifecycle hooks. Two cases are intentionally NOT covered: -/// `semver.clean` normalization (pacquet keeps version verbatim, per -/// the function's doc comment) and the missing-version default of -/// `0.0.0` (pacquet leaves the field absent rather than synthesizing -/// one). -mod normalize_bundled_manifest_tests { - use super::normalize_bundled_manifest; - use pretty_assertions::assert_eq; - use serde_json::json; - - #[test] - fn returns_none_for_empty_manifest() { - assert_eq!(normalize_bundled_manifest(&json!({})), None); - } - - #[test] - fn returns_none_for_non_object() { - assert_eq!(normalize_bundled_manifest(&json!("not an object")), None); - assert_eq!(normalize_bundled_manifest(&json!(null)), None); - assert_eq!(normalize_bundled_manifest(&json!(42)), None); - } - - #[test] - fn returns_none_when_manifest_has_only_excluded_fields() { - assert_eq!( - normalize_bundled_manifest(&json!({ - "description": "a package", - "keywords": ["test"], - "license": "MIT", - "author": "test", - "repository": "test/test", - })), - None, - ); - } - - #[test] - fn picks_included_fields_and_excludes_others() { - let result = normalize_bundled_manifest(&json!({ - "name": "foo", - "version": "1.0.0", - "description": "should be excluded", - "license": "MIT", - "bin": { "foo": "./bin/foo.js" }, - "engines": { "node": ">=18" }, - "cpu": ["x64"], - "os": ["linux"], - "libc": ["glibc"], - "dependencies": { "bar": "^1.0.0" }, - "devDependencies": { "qux": "^3.0.0" }, - "optionalDependencies": { "baz": "^2.0.0" }, - "peerDependencies": { "react": "^18" }, - "peerDependenciesMeta": { "react": { "optional": true } }, - "bundledDependencies": ["bar"], - "directories": { "bin": "./bin" }, - })) - .expect("non-empty pick"); - let map = result.as_object().expect("object"); - assert_eq!(map.get("name").and_then(|v| v.as_str()), Some("foo")); - assert_eq!(map.get("version").and_then(|v| v.as_str()), Some("1.0.0")); - assert_eq!(map.get("bin"), Some(&json!({ "foo": "./bin/foo.js" }))); - assert_eq!(map.get("engines"), Some(&json!({ "node": ">=18" }))); - assert_eq!(map.get("cpu"), Some(&json!(["x64"]))); - assert_eq!(map.get("os"), Some(&json!(["linux"]))); - assert_eq!(map.get("libc"), Some(&json!(["glibc"]))); - assert_eq!(map.get("dependencies"), Some(&json!({ "bar": "^1.0.0" }))); - assert_eq!(map.get("devDependencies"), Some(&json!({ "qux": "^3.0.0" }))); - assert_eq!(map.get("optionalDependencies"), Some(&json!({ "baz": "^2.0.0" }))); - assert_eq!(map.get("peerDependencies"), Some(&json!({ "react": "^18" }))); - assert_eq!( - map.get("peerDependenciesMeta"), - Some(&json!({ "react": { "optional": true } })), - ); - assert_eq!(map.get("bundledDependencies"), Some(&json!(["bar"]))); - assert_eq!(map.get("directories"), Some(&json!({ "bin": "./bin" }))); - // Excluded fields stay out. - assert!(map.get("description").is_none()); - assert!(map.get("license").is_none()); - assert!(map.get("keywords").is_none()); - } - - #[test] - fn only_picks_lifecycle_scripts_not_all_scripts() { - let result = normalize_bundled_manifest(&json!({ - "name": "foo", - "version": "1.0.0", - "scripts": { - "preinstall": "echo pre", - "install": "echo install", - "postinstall": "echo post", - "test": "jest", - "build": "tsc", - "start": "node index.js", - "prepare": "tsc", - }, - })) - .expect("non-empty pick"); - assert_eq!( - result.get("scripts").expect("scripts present"), - &json!({ - "preinstall": "echo pre", - "install": "echo install", - "postinstall": "echo post", - }), - ); - } - - #[test] - fn omits_scripts_key_when_no_lifecycle_scripts_exist() { - let result = normalize_bundled_manifest(&json!({ - "name": "foo", - "version": "1.0.0", - "scripts": { - "test": "jest", - "build": "tsc", - }, - })) - .expect("non-empty pick"); - assert!( - result.get("scripts").is_none(), - "scripts key must be absent when no lifecycle hook is present", - ); - } - - /// `null` and `undefined` fields are skipped. Rust's - /// [`serde_json::Value`] has no `undefined`, but JSON `null` - /// reaches the picker as [`serde_json::Value::Null`] and must be - /// filtered out the same way. - #[test] - fn skips_null_fields() { - let result = normalize_bundled_manifest(&json!({ - "name": "foo", - "version": "1.0.0", - "bin": null, - "engines": null, - })) - .expect("non-empty pick"); - assert!(result.get("bin").is_none(), "null `bin` must be dropped"); - assert!(result.get("engines").is_none(), "null `engines` must be dropped"); - assert_eq!(result.get("name").and_then(|v| v.as_str()), Some("foo")); - assert_eq!(result.get("version").and_then(|v| v.as_str()), Some("1.0.0")); - } - - /// The bundled manifest is downstream-fed into - /// `extract_peer_dependencies` and `extract_children`; dropping - /// `peerDependenciesMeta` or `optionalDependencies` here would - /// replicate the pnpm/pnpm#11934 resolver-side bug on the - /// install-side. Pin the keys explicitly. - #[test] - fn preserves_optional_dependencies_and_peer_dependencies_meta_keys() { - let result = normalize_bundled_manifest(&json!({ - "name": "consumer", - "version": "1.0.0", - "optionalDependencies": { "sharp": "^0.34.0" }, - "peerDependenciesMeta": { - "@vercel/kv": { "optional": true }, - "ioredis": { "optional": true }, - }, - })) - .expect("non-empty pick"); - assert_eq!(result.get("optionalDependencies"), Some(&json!({ "sharp": "^0.34.0" }))); - assert_eq!( - result.get("peerDependenciesMeta"), - Some(&json!({ - "@vercel/kv": { "optional": true }, - "ioredis": { "optional": true }, - })), - ); - } -} - /// Build a gzipped tar whose *compressed* body is at least `min_bytes`, /// so the response carries a `Content-Length` over the in-progress /// threshold. The payload is LCG noise because gzip would otherwise