This repository was archived by the owner on May 14, 2026. It is now read-only.
add version-pin#4
Merged
Merged
Conversation
5 tasks
zkochan
added a commit
that referenced
this pull request
Apr 24, 2026
Two perf fixes from investigations/pacquet-macos-perf.md that both collapse redundant passes over the tarball bytes. 1. Stream the HTTP body through the gzip decoder (doc fix #4). Replace `response.bytes().await` + `decompress_gzip` (sync zune-inflate) + separate `Integrity::check(&buffer)` with a streaming pipeline: `bytes_stream()` teed through `ssri::IntegrityChecker` into `async_compression::tokio::write:: GzipDecoder`. This removes one full pass over the compressed body (no separate `.check()`), never holds compressed + decompressed buffers at the same time, and lets decompression overlap with network awaits. Integrity is still verified end-to-end before any CAFS blob is written, so a mismatch aborts the install with the same `TarballError::Checksum` behavior as before. 2. Interleave SHA-512 with the per-entry read (doc fix #2). Add `StoreDir::write_cas_file_prehashed`; the tar extraction loop reads + hashes 64 KiB chunks in one pass instead of `read_to_end` followed by a separate `Sha512::digest(buffer)` inside `write_cas_file`. The buffer still has to exist (CAFS filename is hash-derived), but two passes over the decompressed entry bytes collapse into one. Mirrors pnpm's `parseTarball` + `addBufferToCafs` in `store/cafs/src/addFilesFromTarball.ts`. Why we diverge from pnpm on (1): upstream `fetching/tarball-fetcher/src/remoteTarballFetcher.ts` + `worker/src/start.ts` + `store/cafs/src/addFilesFromTarball.ts` buffers the whole tarball into a Node `SharedArrayBuffer` and runs `crypto.hash` + `zlib.gunzipSync` sequentially. Observable behavior here is identical — same integrity errors on mismatch, same decompressed bytes, same CAFS layout, same `index.db` rows — so per the "Internal performance divergence is allowed" clause added in the preceding commit, the streaming topology is a pacquet-specific implementation-detail optimization rather than a shape port. Dep changes: swap `zune-inflate` for `async-compression` (gzip + tokio features) + `futures-util`; enable reqwest's `stream` feature; add `sha2` to the tarball crate. Tests: existing `packages_under_orgs_should_work` (real-network end-to-end) and `should_throw_error_on_checksum_mismatch` both pass unchanged, proving the streaming pipeline produces the same decompressed bytes and fails integrity checks the same way.
zkochan
added a commit
that referenced
this pull request
Apr 24, 2026
Two perf fixes from investigations/pacquet-macos-perf.md that both collapse redundant passes over the tarball bytes. 1. Stream the HTTP body through the gzip decoder (doc fix #4). Replace `response.bytes().await` + `decompress_gzip` (sync zune-inflate) + separate `Integrity::check(&buffer)` with a streaming pipeline: `bytes_stream()` teed through `ssri::IntegrityChecker` into `async_compression::tokio::write:: GzipDecoder`. This removes one full pass over the compressed body (no separate `.check()`), never holds compressed + decompressed buffers at the same time, and lets decompression overlap with network awaits. Integrity is still verified end-to-end before any CAFS blob is written, so a mismatch aborts the install with the same `TarballError::Checksum` behavior as before. 2. Interleave SHA-512 with the per-entry read (doc fix #2). Add `StoreDir::write_cas_file_prehashed`; the tar extraction loop reads + hashes 64 KiB chunks in one pass instead of `read_to_end` followed by a separate `Sha512::digest(buffer)` inside `write_cas_file`. The buffer still has to exist (CAFS filename is hash-derived), but two passes over the decompressed entry bytes collapse into one. Mirrors pnpm's `parseTarball` + `addBufferToCafs` in `store/cafs/src/addFilesFromTarball.ts`. Why we diverge from pnpm on (1): upstream `fetching/tarball-fetcher/src/remoteTarballFetcher.ts` + `worker/src/start.ts` + `store/cafs/src/addFilesFromTarball.ts` buffers the whole tarball into a Node `SharedArrayBuffer` and runs `crypto.hash` + `zlib.gunzipSync` sequentially. Observable behavior here is identical — same integrity errors on mismatch, same decompressed bytes, same CAFS layout, same `index.db` rows — so per the "Internal performance divergence is allowed" clause added in the preceding commit, the streaming topology is a pacquet-specific implementation-detail optimization rather than a shape port. Dep changes: swap `zune-inflate` for `async-compression` (gzip + tokio features) + `futures-util`; enable reqwest's `stream` feature; add `sha2` to the tarball crate. Tests: existing `packages_under_orgs_should_work` (real-network end-to-end) and `should_throw_error_on_checksum_mismatch` both pass unchanged, proving the streaming pipeline produces the same decompressed bytes and fails integrity checks the same way.
zkochan
added a commit
that referenced
this pull request
Apr 25, 2026
- Move `configure_rayon_pool` to after `CliArgs::parse()`. clap's `--help` / `--version` paths exit inside `parse`, so they never reach the rayon pool init now — `pacquet --help` no longer spawns worker threads it'll throw away. Other subcommand paths still pay the (small) init cost; we can move it into the install path specifically as a follow-up if non-install commands are ever added that don't need rayon. - Drop the local `CasPathsArc` / `WarmEntry` aliases in `create_virtual_store::run`. They duplicated the value type of `pacquet_tarball::PrefetchedCasPaths`; if that map's value type changes, the local aliases would silently drift. Letting `warm`'s element type be inferred from its `push` calls keeps the binding to the upstream type. - Add three unit tests for `prefetch_cas_paths`: * `prefetch_cas_paths_returns_hits_for_live_index_rows` — happy path: index row written, CAFS blob present, verify on; result contains the expected file map. * `prefetch_cas_paths_omits_failed_integrity_entries` — index row whose digest matches no on-disk file; verify on; row is dropped from the result rather than surfacing a half-populated map. * `prefetch_cas_paths_skips_filesystem_checks_when_verify_disabled` — same shape as the failing-integrity test but with `verify_store_integrity = false`; the row surfaces (no fs checks), matching pnpm's `verify-store-integrity: false` semantics.
Merged
KSXGitHub
pushed a commit
that referenced
this pull request
Apr 30, 2026
#4 (cmd-shim — chmod target swallowed all errors): the post-write \`ensure_executable_bits\` call used \`let _ = ...\` which dropped \`PermissionDenied\`, \`EROFS\`, AppArmor deny and other real failures alongside the intentionally-ignored \`NotFound\`. Match \`NotFound\` explicitly and propagate everything else as \`LinkBinsError::Chmod\`, mirroring pnpm's \`fixBin\` ENOENT guard. Adds two DI-driven regression tests: - \`link_bins_propagates_target_chmod_error_via_di\` — \`PermissionDenied\` must surface as \`Chmod\`. - \`link_bins_swallows_target_chmod_not_found_via_di\` — \`NotFound\` must remain swallowed. #5 (package-manager — peer-resolved slot name parsing): \`find_slot_ own_package_dir\` used \`slot_name.rfind('@')\` to strip the version tail. That works for \`parent@1.0.0\` and \`@scope+parent@1.0.0\` but breaks for peer-resolved slots: \`to_virtual_store_name\` produces shapes like \`ts-node@10.9.1_@types+node@18.7.19_typescript@5.1.6\` where the last \`@\` is inside a peer's version, not at the package-name boundary. \`rfind\` then split inside the peer spec, the slot's own package wasn't found, and the slot was silently skipped — bins of children of every peer-resolved slot were never linked. Parse from the left instead, skipping a single leading \`@\` that belongs to a scoped package. The package-name half can never contain \`@\` after \`to_virtual_store_name\`'s scope normalization (\`/\` → \`+\`), so the first non-leading \`@\` is the right boundary. Adds \`link_virtual_store_bins_handles_peer_resolved_slot_name\` using the exact \`ts-node@10.9.1_@types+node@18.7.19_typescript@5.1.6\` shape verified by \`pacquet_lockfile::pkg_name_ver_peer::tests::to_virtual_ store_name\`. Both fixes verified by re-breaking the relevant code path and confirming the regression tests fail, then reverting. Drive-by from #9 (style nit on the chain split in \`SymlinkDirectDependencies::run\`): the chain was broken in the earlier \`feat(cmd-shim): port directories.bin discovery and Windows shim formats\` commit (dfdd33a) because \`direct_deps\` is consumed twice — once by \`symlink_direct_deps_into_node_modules\`, once by \`link_direct_dep_bins\`'s \`dep_names\` derivation. That can't sit inside a single chain. Documenting it here in lieu of amending the earlier commit. Test count: cmd-shim 70 → 72, package-manager (lib) 38 → 39.
Merged
4 tasks
zkochan
added a commit
that referenced
this pull request
May 12, 2026
The Windows CI runner caught two test-only Windows regressions
introduced by the shell-selection change earlier in this branch:
- `extend_path::tests::pnpm_virtual_store_layout_yields_three_bins_deepest_first`
hard-coded `/proj/...` expected values. `path::absolute("/proj")`
on Windows resolves against the current drive (`C:\proj`), making
those expectations racy with the runner. Split into a Unix-gated
test with literal expectations and a new platform-neutral test
that asserts the structural invariants (count + deepest-first
ordering) without anchoring to any absolute root.
- `lifecycle::tests::lifecycle_emits_script_stdio_and_exit_in_order`
and the new `child_sees_stamped_npm_package_and_no_leaked_npm_config`
both use POSIX-only script syntax (`;`, `1>&2`, `printf`, `$VAR`).
Before this branch the spawn used a hard-coded `sh -c` so Git
Bash on the Windows runner satisfied them; once the shell
selector started picking `cmd /d /s /c` on Windows (per item #4
/ parity with pnpm) those tests can no longer run there. Gate
them on `cfg(unix)`. The cmd-path shell selection itself is
covered by `crate::shell` unit tests; env stamping by
`crate::make_env` units.
No production-code change.
zkochan
added a commit
that referenced
this pull request
May 12, 2026
…ecycle scripts (#397) (#418) ## Summary Closes three of the critical items in #397 by porting the corresponding behaviors from `@pnpm/npm-lifecycled2d8e790` and `pnpm/pnpm@b4f8f47ac2`: - **#1 — Lifecycle env vars.** New `make_env` module ports `makeEnv` and the surrounding env block in `lifecycle()` ([npm-lifecycle/index.js:73-104](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L73-L104), [:354-414](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L354-L414)) plus the pnpm wrapper's `extraEnv` additions ([runLifecycleHook.ts:119-124](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/exec/lifecycle/src/runLifecycleHook.ts#L119-L124)). Lifecycle scripts now see `npm_lifecycle_event`, `npm_lifecycle_script`, `npm_node_execpath`/`NODE`, `npm_package_json`, `npm_execpath`, `npm_package_*` (`name`, `version`, and recursive `config`/`engines`/`bin`), `npm_config_node_gyp`, `npm_config_user_agent`, `INIT_CWD`, `PNPM_SCRIPT_SRC_DIR`, and `TMPDIR` (when `unsafe_perm` is false). The spawn now strips inherited env (`env_clear()`) so leftover `npm_*` keys from a wrapping invocation cannot leak through. - **#2 — PATH ancestor walk.** New `extend_path` module ports `extendPath` ([npm-lifecycle/lib/extendPath.js:5-27](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/lib/extendPath.js#L5-L27)) plus the tri-state `scriptsPrependNodePath` gating ([:29-61](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/lib/extendPath.js#L29-L61)). For a dep at `<root>/node_modules/.pnpm/<slot>/node_modules/<pkg>`, `PATH` now contains the dep's own `.bin`, the `.pnpm` slot's `.bin`, the project root's `.bin`, the bundled `node-gyp-bin` (when supplied), `extra_bin_paths`, and finally the inherited PATH. - **#4 — Shell selection.** New `shell` module ports the [shell-selection block](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/index.js#L241-L252) and the [pnpm-side `.bat`/`.cmd` guard](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/exec/lifecycle/src/runLifecycleHook.ts#L63-L71). `cmd /d /s /c` on Windows, custom `scriptShell` on either platform, otherwise `sh -c`. The Windows-batch-file `scriptShell` case surfaces as `ERR_PNPM_INVALID_SCRIPT_SHELL_WINDOWS` (matching upstream's error code). `RunPostinstallHooks` grows seven new fields to surface these knobs; `BuildModules` passes safe defaults (`None` / `true` / `Never`) for all of them — full config plumbing for `user-agent`, `unsafe-perm`, `scripts-prepend-node-path`, `node-gyp` bundling, and `script-shell` are tracked as separate items in #397 (#14, #15) or follow-ups. ### Explicit non-goals in this PR Three caveats called out in #397 that are deliberately deferred: - `windowsVerbatimArguments` (Rust equivalent: `CommandExt::raw_arg`) is signalled by `SelectedShell.windows_verbatim_args` but not yet applied to the spawned `Command`. - `@yarnpkg/shell` / `shellEmulator: true` has no clean Rust port; pacquet ignores the flag for now. - `unsafe_perm` uid/gid drop (#14) — `BuildModules` passes `true`, which keeps current behavior (no TMPDIR creation, no privilege drop). ### Test parity Per the project guide ("port the relevant pnpm tests too whenever they translate"), this branch ports: - `test('makeEnv')` from [npm-lifecycle/test/index.js:97-124](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/test/index.js#L97-L124). - The `extendPath` ordering test from [npm-lifecycle/test/extendPath.test.js:5-8](https://github.com/pnpm/npm-lifecycle/blob/d2d8e790/test/extendPath.test.js#L5-L8). - `runLifecycleHook() does not set npm_config env vars` from [pnpm/exec/lifecycle/test/index.ts:65-77](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/exec/lifecycle/test/index.ts#L65-L77), adapted to a file-dump model so we don't need the IPC fixture. - `onlyOnWindows('pnpm shows error if script-shell is .cmd')` from [pnpm/exec/commands/test/index.ts:509-542](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/exec/commands/test/index.ts#L509-L542) and the custom-shell case from [:478-508](https://github.com/pnpm/pnpm/blob/b4f8f47ac2/exec/commands/test/index.ts#L478-L508). Plus fills the gaps where upstream coverage is thin: tri-state `scriptsPrependNodePath`, two-level pnpm virtual-store PATH walks, `extra_bin_paths` ordering, TMPDIR gating on `unsafe_perm`, and `extra_env` precedence vs `npm_lifecycle_script`.
This was referenced May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.