Skip to content

Fetching sub-dependencies sooner #456

Merged
zkochan merged 10 commits into
masterfrom
issue453
Nov 3, 2016
Merged

Fetching sub-dependencies sooner #456
zkochan merged 10 commits into
masterfrom
issue453

Conversation

@zkochan

@zkochan zkochan commented Nov 3, 2016

Copy link
Copy Markdown
Member

Approximately 3.5% faster. I thought it would be more significant, but better than nothing

@zkochan zkochan changed the title Issue453 Fetching sub-dependencies sooner Nov 3, 2016
Comment thread src/install/installMultiple.ts Outdated
function addInstalledPkg (installs: InstalledPackages, newPkg: InstalledPackage) {
if (!installs[newPkg.id]) {
installs[newPkg.id] = newPkg
installs[newPkg.id] = Object.assign({}, newPkg)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are trying to achieve with Object.assign?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert it. I thought there was a mutation problem but there was not

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, because anyway it will not help to prevent mutation in nested fields

@iamstarkov iamstarkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small style comments, worth mentioning, but irrelevant to end goal

Comment thread src/install/installMultiple.ts Outdated
await options.fetchingFiles

await mkdirp(modules)
await Promise.all(installedPkgs.filter(subdep => !subdep.fromCache).map(subdep => symlinkToModules(subdep.path, modules)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite long line

Comment thread src/install/fetch.ts Outdated
? Promise.resolve(res.pkg)
: fetchingFiles.then(() => {
return requireJson(path.join(pkgPath, 'package.json'))
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe? its already arrow function

fetchingFiles.then(() => requireJson(path.join(pkgPath, 'package.json')))

@zkochan

zkochan commented Nov 3, 2016

Copy link
Copy Markdown
Member Author

Thanks, changed everything as you proposed

@iamstarkov

Copy link
Copy Markdown
Contributor

oh gosh, i love new github's code review ui

Comment thread package.json Outdated
"lint": "standard && tslint -c tslint.json --project .",
"sinopia": "sinopia -c ./sinopia/config.yaml",
"test:tap": "ts-node test | tap-diff",
"test:tap": "ts-node test | tap-pessimist",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does it look?

@zkochan zkochan Nov 3, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it should show only the part of the raw tap output that describes fails. But I checked and it strips out too much, doesn't show the expected/actual text and the row where the error happened. So I'll try some other alternative

I want this because there are too many tap logs and when something fails it is very hard to find the error message. Especially in AppVeyor

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tap-dot seems to be good

@zkochan zkochan force-pushed the issue453 branch 2 times, most recently from 59da4a5 to b50aff9 Compare November 3, 2016 18:47
@zkochan zkochan merged commit b50aff9 into master Nov 3, 2016
@zkochan zkochan deleted the issue453 branch December 24, 2016 12:43
pull Bot pushed a commit to dwongdev/pnpm that referenced this pull request May 14, 2026
…bc detection (pnpm#434 slice 2) (pnpm#456)

Slice 2 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slice 1 (pnpm#439) wired `pacquet-package-is-installable` into the install pipeline but the `SupportedArchitectures` input was always `None`, so every install behaved as if the user had opted into \"host triple only\". This PR closes that gap and replaces the slice 1 libc stub with a real Linux probe.

Mirrors the three upstream input sources pinned at [pnpm/pnpm@94240bc046](https://github.com/pnpm/pnpm/tree/94240bc046):

- **`Config.supported_architectures` from `pnpm-workspace.yaml`** — same shape as upstream's `getOptionsFromRootManifest.ts` ([`config/reader/src/getOptionsFromRootManifest.ts#L23`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/getOptionsFromRootManifest.ts#L23)).
- **`--cpu` / `--os` / `--libc` flags on `install` / `add`** — multi-valued, comma-separated. Per-axis CLI override replaces the yaml axis wholesale, matching upstream's [`overrideSupportedArchitecturesWithCLI.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/overrideSupportedArchitecturesWithCLI.ts).
- **Real Linux libc detection** — replaces the slice 1 `\"unknown\"` stub with a `/lib*/ld-musl-*` probe cached via `OnceLock`. Mirrors `detect-libc.familySync()` semantics: `\"musl\"` / `\"glibc\"` on Linux, `\"unknown\"` everywhere else. No new dep — open-coded `read_dir` is cheaper than spawning `ldd --version` and works in slim containers without `ldd` on PATH.

Threaded through `Install` / `InstallFrozenLockfile` / `Add` as an explicit field instead of mutating `State.config`, because `State.config` is `&'static Config` — the CLI override merge happens at the CLI layer and the fully-resolved value lands on the install pipeline.

## Test plan

Unit tests added; full suite ran via `just ready` + `cargo doc --no-deps` + `taplo format --check` + `just dylint`.

- [x] `cargo test -p pacquet-config workspace_yaml::tests::*supported_architectures*` — yaml round-trip across `os` / `cpu` / `libc`, omission, partial-axis (3 tests).
- [x] `cargo test -p pacquet-package-manager --lib installability::tests::supported_architectures*` — `supportedArchitectures` widens the accept set so a darwin-only package stays on a linux host when the user lists `darwin`; conversely, listing `linux` keeps the darwin-only package skipped (no implicit host include).
- [x] `just ready` — typos + fmt + check + nextest (792 tests pass) + clippy.
- [x] `taplo format --check`.
- [x] `just dylint` (perfectionist).
- [x] `RUSTDOCFLAGS=\"-D warnings\" cargo doc --no-deps --workspace`.

## Out of scope

- `.modules.yaml.skipped` persistence (umbrella slice 3).
- Current-lockfile diffing for `removeOptionalDependenciesThatAreNotUsed` (umbrella slice 6).
- `--no-optional` plumbing (umbrella slice 5).

Closes pnpm#453.
pull Bot pushed a commit to dwongdev/pnpm that referenced this pull request May 14, 2026
…pm#434 slice 3) (pnpm#467)

## Summary

Slice 3 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slice 1 (pnpm#439) computed a `SkippedSnapshots` set on every frozen-lockfile install but never serialized it; slice 2 (pnpm#456) closed the input side. This PR closes the loop by mirroring three upstream sites pinned at [pnpm/pnpm@94240bc046](https://github.com/pnpm/pnpm/tree/94240bc046):

- **Write** — `Modules.skipped` was declared at [`crates/modules-yaml/src/lib.rs:197`](https://github.com/pnpm/pacquet/blob/97010013/crates/modules-yaml/src/lib.rs#L197) with sort-on-write but always serialized empty. `build_modules_manifest` now takes `&SkippedSnapshots` and produces the depPath string list pnpm writes at [`installing/deps-installer/src/install/index.ts:1625`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/index.ts#L1625).

- **Seed** — `install_frozen_lockfile::run` reads `.modules.yaml.skipped` before computing the in-memory set and passes the parsed `PackageKey`s as the seed to `compute_skipped_snapshots`. Mirrors upstream's load at [`installing/read-projects-context/src/index.ts:79`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/read-projects-context/src/index.ts#L79) plus the early return at [`deps/graph-builder/src/lockfileToDepGraph.ts:194`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L194).

- **Short-circuit** — `compute_skipped_snapshots` returns the seed verbatim on the fast path (no constraints in the lockfile) and, on the slow path, skips the per-snapshot re-check for keys already in the seed without emitting a duplicate `pnpm:skipped-optional-dependency` event. Non-seeded snapshots still re-run `package_is_installable`, covering the \"host arch changed since last install\" case upstream guards at [`:206-215`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L206-L215).

`InstallFrozenLockfile::run` now returns a small `InstallFrozenLockfileOutput { hoisted_dependencies, skipped }` struct so `Install::run` can thread both into `.modules.yaml`. Read errors on `.modules.yaml` degrade to an empty seed — the file is a cache artifact, not authoritative.
pull Bot pushed a commit to dwongdev/pnpm that referenced this pull request May 14, 2026
…e payload (pnpm#434 slice 4) (pnpm#474)

Slice 4 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slices 1–3 (pnpm#439, pnpm#456, pnpm#467) close the installability-driven skip path; this slice adds the second skip pathway upstream handles in the frozen install: an `optional: true` snapshot whose **fetch / extract** step fails at install time must not abort the install. Local-materialization errors (`CreateVirtualDir`) and config-shape errors still abort even for optional snapshots — matching upstream's `linkPkg` path which sits outside the catch.

Mirrors the silent catch sites at [`deps/graph-builder/src/lockfileToDepGraph.ts:294-298`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L294-L298) and [`installing/deps-restorer/src/index.ts:912-921`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L912-L921):

```ts
try { ... fetch ... }
catch (err) { if (pkgSnapshot.optional) return; throw err }
```

## Changes

- **Reporter**: `SkippedOptionalPackage` becomes a `#[serde(untagged)]` enum with `Installed { id, name, version }` and `ResolutionFailure { name?, version?, bareSpecifier }` variants. Models upstream's discriminated union at [`core-loggers/src/skippedOptionalDependencyLogger.ts:10-31`](https://github.com/pnpm/pnpm/blob/94240bc046/core/core-loggers/src/skippedOptionalDependencyLogger.ts#L10-L31). The new variant is wire-shape-only in slice 4 — pacquet has no resolver yet, but a future resolver port at the upstream emit site ([`resolveDependencies.ts:1376-1383`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-resolver/src/resolveDependencies.ts#L1376-L1383)) lands without re-touching this type.
- **`SkippedSnapshots`**: gains a `fetch_failed` subset disjoint from the existing `installability` subset. `contains` / `iter` return the union (downstream consumers treat both as absent); `iter_installability` returns only the persistent subset that `.modules.yaml.skipped` records, matching upstream's behavior of never updating `opts.skipped` at the fetch-failure catch sites.
- **`CreateVirtualStore`**: cold-batch dispatch swallows per-snapshot errors when `snapshot.optional` is true **and** the error is fetch-side. A new `is_fetch_side_failure` helper restricts the swallow to `InstallPackageBySnapshotError::DownloadTarball` and `::GitFetch` — the same surface upstream wraps in `storeController.fetchPackage`. Local-materialization (`CreateVirtualDir`) and config-shape errors (`MissingTarballIntegrity` / `UnsupportedResolution`) propagate even for optional snapshots, matching upstream's `linkPkg` path which sits outside the catch. Side benefit: the warm-batch path (which only surfaces `CreateVirtualDir` errors) stays consistently abort-on-error without a parallel swallow site. The per-install `fetch_failed: HashSet<PackageKey>` rides out on `CreateVirtualStoreOutput`.
- **`InstallFrozenLockfile::run`**: folds the returned `fetch_failed` into its mutable `SkippedSnapshots` so downstream phases (`SymlinkDirectDependencies`, `LinkVirtualStoreBins`, `BuildModules`, hoist) treat the dropped snapshots as absent through the same gate they already use for installability skips.

## Test plan

- [x] `reporter::tests::skipped_optional_resolution_failure_event_matches_pnpm_wire_shape` — full payload (`bareSpecifier` camelCase, no `id`).
- [x] `reporter::tests::skipped_optional_resolution_failure_omits_absent_name_and_version` — optional-field shape.
- [x] `install::tests::frozen_install_silently_swallows_unreachable_optional_tarball` — ports [`installing/deps-restorer/test/index.ts:340-360`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/test/index.ts#L340-L360): unreachable optional tarball (`http://127.0.0.1:1/...`), install succeeds, slot not created, `.modules.yaml.skipped` left empty. **Test-the-test verified** by inverting the `optional` guard in the cold-batch match arm. Runs with `enable_global_virtual_store = false` so the slot-path assertion targets the legacy layout.
- [x] `install::tests::frozen_install_propagates_non_optional_fetch_failure` — pins the polarity: same fixture, non-optional snapshot, install must error.
- [x] `just ready` (typos + fmt + check + nextest + clippy).
- [x] `taplo format --check`.
- [x] `just dylint` (perfectionist).
- [x] `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace`.

## Out of scope

- Resolver-side `resolution_failure` emit site — needs a resolver, tracked separately.
- `--no-optional` / `--omit=optional` plumbing — umbrella slice 5.
- Surfacing fetch failures to the reporter on the frozen path — upstream itself is silent here; only the resolver-side emit fires `pnpm:skipped-optional-dependency`.

Closes pnpm#471.
pull Bot pushed a commit to dwongdev/pnpm that referenced this pull request May 14, 2026
…e 5) (pnpm#485)

Slice 5 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slices 1–4 (pnpm#439, pnpm#456, pnpm#467, pnpm#474) handle installability + the fetch-failure swallow. This slice closes the user-facing `--no-optional` flag: the CLI flag already exists and threads through `Install::dependency_groups`, but only `SymlinkDirectDependencies` and the on-disk `included` field actually honored it. The rest of the install pipeline walked `optional_dependencies` unconditionally, so the optional subtree was still extracted, linked, and built.

Mirrors upstream's depNode filter at [`installing/deps-installer/src/install/link.ts:109-111`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/link.ts#L109-L111):

```ts
if (!opts.include.optionalDependencies) {
  depNodes = depNodes.filter(({ optional }) => !optional)
}
```

## Changes

- **`SkippedSnapshots`**: gains a third disjoint subset `optional_excluded` alongside `installability` (slice 1) and `fetch_failed` (slice 4). The existing `contains` / `iter` already return the union, so every downstream consumer that filters by skip-set (`CreateVirtualStore`, `SymlinkDirectDependencies`, `BuildModules`, hoist, `link_bins`) now respects `--no-optional` through the same gate. `iter_installability` still returns only the persistent subset for `.modules.yaml.skipped` writes — `--no-optional` exclusions are transient (matching upstream's behavior of never updating `opts.skipped` at the filter site).
- **`InstallFrozenLockfile::run`**: iterates the lockfile snapshots once and inserts every `snap.optional == true` entry into the new subset when the dispatch list doesn't contain `DependencyGroup::Optional`. The `SnapshotEntry::optional` flag is set by the resolver only when the snapshot is reachable **exclusively** through optional edges, so a snapshot reachable through any non-optional edge carries `optional: false` and survives the filter — covers [`optionalDependencies.ts:712`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/optionalDependencies.ts#L712) (`dependency that is both optional and non-optional is installed`).
github-actions Bot pushed a commit to Eyalm321/pnpm that referenced this pull request May 18, 2026
…bc detection (pnpm#434 slice 2) (pnpm#456)

Slice 2 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slice 1 (pnpm#439) wired `pacquet-package-is-installable` into the install pipeline but the `SupportedArchitectures` input was always `None`, so every install behaved as if the user had opted into \"host triple only\". This PR closes that gap and replaces the slice 1 libc stub with a real Linux probe.

Mirrors the three upstream input sources pinned at [pnpm/pnpm@94240bc046](https://github.com/pnpm/pnpm/tree/94240bc046):

- **`Config.supported_architectures` from `pnpm-workspace.yaml`** — same shape as upstream's `getOptionsFromRootManifest.ts` ([`config/reader/src/getOptionsFromRootManifest.ts#L23`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/getOptionsFromRootManifest.ts#L23)).
- **`--cpu` / `--os` / `--libc` flags on `install` / `add`** — multi-valued, comma-separated. Per-axis CLI override replaces the yaml axis wholesale, matching upstream's [`overrideSupportedArchitecturesWithCLI.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/overrideSupportedArchitecturesWithCLI.ts).
- **Real Linux libc detection** — replaces the slice 1 `\"unknown\"` stub with a `/lib*/ld-musl-*` probe cached via `OnceLock`. Mirrors `detect-libc.familySync()` semantics: `\"musl\"` / `\"glibc\"` on Linux, `\"unknown\"` everywhere else. No new dep — open-coded `read_dir` is cheaper than spawning `ldd --version` and works in slim containers without `ldd` on PATH.

Threaded through `Install` / `InstallFrozenLockfile` / `Add` as an explicit field instead of mutating `State.config`, because `State.config` is `&'static Config` — the CLI override merge happens at the CLI layer and the fully-resolved value lands on the install pipeline.

## Test plan

Unit tests added; full suite ran via `just ready` + `cargo doc --no-deps` + `taplo format --check` + `just dylint`.

- [x] `cargo test -p pacquet-config workspace_yaml::tests::*supported_architectures*` — yaml round-trip across `os` / `cpu` / `libc`, omission, partial-axis (3 tests).
- [x] `cargo test -p pacquet-package-manager --lib installability::tests::supported_architectures*` — `supportedArchitectures` widens the accept set so a darwin-only package stays on a linux host when the user lists `darwin`; conversely, listing `linux` keeps the darwin-only package skipped (no implicit host include).
- [x] `just ready` — typos + fmt + check + nextest (792 tests pass) + clippy.
- [x] `taplo format --check`.
- [x] `just dylint` (perfectionist).
- [x] `RUSTDOCFLAGS=\"-D warnings\" cargo doc --no-deps --workspace`.

## Out of scope

- `.modules.yaml.skipped` persistence (umbrella slice 3).
- Current-lockfile diffing for `removeOptionalDependenciesThatAreNotUsed` (umbrella slice 6).
- `--no-optional` plumbing (umbrella slice 5).

Closes pnpm#453.
github-actions Bot pushed a commit to Eyalm321/pnpm that referenced this pull request May 18, 2026
…pm#434 slice 3) (pnpm#467)

## Summary

Slice 3 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slice 1 (pnpm#439) computed a `SkippedSnapshots` set on every frozen-lockfile install but never serialized it; slice 2 (pnpm#456) closed the input side. This PR closes the loop by mirroring three upstream sites pinned at [pnpm/pnpm@94240bc046](https://github.com/pnpm/pnpm/tree/94240bc046):

- **Write** — `Modules.skipped` was declared at [`crates/modules-yaml/src/lib.rs:197`](https://github.com/pnpm/pacquet/blob/97010013/crates/modules-yaml/src/lib.rs#L197) with sort-on-write but always serialized empty. `build_modules_manifest` now takes `&SkippedSnapshots` and produces the depPath string list pnpm writes at [`installing/deps-installer/src/install/index.ts:1625`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/index.ts#L1625).

- **Seed** — `install_frozen_lockfile::run` reads `.modules.yaml.skipped` before computing the in-memory set and passes the parsed `PackageKey`s as the seed to `compute_skipped_snapshots`. Mirrors upstream's load at [`installing/read-projects-context/src/index.ts:79`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/read-projects-context/src/index.ts#L79) plus the early return at [`deps/graph-builder/src/lockfileToDepGraph.ts:194`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L194).

- **Short-circuit** — `compute_skipped_snapshots` returns the seed verbatim on the fast path (no constraints in the lockfile) and, on the slow path, skips the per-snapshot re-check for keys already in the seed without emitting a duplicate `pnpm:skipped-optional-dependency` event. Non-seeded snapshots still re-run `package_is_installable`, covering the \"host arch changed since last install\" case upstream guards at [`:206-215`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L206-L215).

`InstallFrozenLockfile::run` now returns a small `InstallFrozenLockfileOutput { hoisted_dependencies, skipped }` struct so `Install::run` can thread both into `.modules.yaml`. Read errors on `.modules.yaml` degrade to an empty seed — the file is a cache artifact, not authoritative.
github-actions Bot pushed a commit to Eyalm321/pnpm that referenced this pull request May 18, 2026
…e payload (pnpm#434 slice 4) (pnpm#474)

Slice 4 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slices 1–3 (pnpm#439, pnpm#456, pnpm#467) close the installability-driven skip path; this slice adds the second skip pathway upstream handles in the frozen install: an `optional: true` snapshot whose **fetch / extract** step fails at install time must not abort the install. Local-materialization errors (`CreateVirtualDir`) and config-shape errors still abort even for optional snapshots — matching upstream's `linkPkg` path which sits outside the catch.

Mirrors the silent catch sites at [`deps/graph-builder/src/lockfileToDepGraph.ts:294-298`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L294-L298) and [`installing/deps-restorer/src/index.ts:912-921`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L912-L921):

```ts
try { ... fetch ... }
catch (err) { if (pkgSnapshot.optional) return; throw err }
```

## Changes

- **Reporter**: `SkippedOptionalPackage` becomes a `#[serde(untagged)]` enum with `Installed { id, name, version }` and `ResolutionFailure { name?, version?, bareSpecifier }` variants. Models upstream's discriminated union at [`core-loggers/src/skippedOptionalDependencyLogger.ts:10-31`](https://github.com/pnpm/pnpm/blob/94240bc046/core/core-loggers/src/skippedOptionalDependencyLogger.ts#L10-L31). The new variant is wire-shape-only in slice 4 — pacquet has no resolver yet, but a future resolver port at the upstream emit site ([`resolveDependencies.ts:1376-1383`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-resolver/src/resolveDependencies.ts#L1376-L1383)) lands without re-touching this type.
- **`SkippedSnapshots`**: gains a `fetch_failed` subset disjoint from the existing `installability` subset. `contains` / `iter` return the union (downstream consumers treat both as absent); `iter_installability` returns only the persistent subset that `.modules.yaml.skipped` records, matching upstream's behavior of never updating `opts.skipped` at the fetch-failure catch sites.
- **`CreateVirtualStore`**: cold-batch dispatch swallows per-snapshot errors when `snapshot.optional` is true **and** the error is fetch-side. A new `is_fetch_side_failure` helper restricts the swallow to `InstallPackageBySnapshotError::DownloadTarball` and `::GitFetch` — the same surface upstream wraps in `storeController.fetchPackage`. Local-materialization (`CreateVirtualDir`) and config-shape errors (`MissingTarballIntegrity` / `UnsupportedResolution`) propagate even for optional snapshots, matching upstream's `linkPkg` path which sits outside the catch. Side benefit: the warm-batch path (which only surfaces `CreateVirtualDir` errors) stays consistently abort-on-error without a parallel swallow site. The per-install `fetch_failed: HashSet<PackageKey>` rides out on `CreateVirtualStoreOutput`.
- **`InstallFrozenLockfile::run`**: folds the returned `fetch_failed` into its mutable `SkippedSnapshots` so downstream phases (`SymlinkDirectDependencies`, `LinkVirtualStoreBins`, `BuildModules`, hoist) treat the dropped snapshots as absent through the same gate they already use for installability skips.

## Test plan

- [x] `reporter::tests::skipped_optional_resolution_failure_event_matches_pnpm_wire_shape` — full payload (`bareSpecifier` camelCase, no `id`).
- [x] `reporter::tests::skipped_optional_resolution_failure_omits_absent_name_and_version` — optional-field shape.
- [x] `install::tests::frozen_install_silently_swallows_unreachable_optional_tarball` — ports [`installing/deps-restorer/test/index.ts:340-360`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/test/index.ts#L340-L360): unreachable optional tarball (`http://127.0.0.1:1/...`), install succeeds, slot not created, `.modules.yaml.skipped` left empty. **Test-the-test verified** by inverting the `optional` guard in the cold-batch match arm. Runs with `enable_global_virtual_store = false` so the slot-path assertion targets the legacy layout.
- [x] `install::tests::frozen_install_propagates_non_optional_fetch_failure` — pins the polarity: same fixture, non-optional snapshot, install must error.
- [x] `just ready` (typos + fmt + check + nextest + clippy).
- [x] `taplo format --check`.
- [x] `just dylint` (perfectionist).
- [x] `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace`.

## Out of scope

- Resolver-side `resolution_failure` emit site — needs a resolver, tracked separately.
- `--no-optional` / `--omit=optional` plumbing — umbrella slice 5.
- Surfacing fetch failures to the reporter on the frozen path — upstream itself is silent here; only the resolver-side emit fires `pnpm:skipped-optional-dependency`.

Closes pnpm#471.
github-actions Bot pushed a commit to Eyalm321/pnpm that referenced this pull request May 18, 2026
…e 5) (pnpm#485)

Slice 5 of the [pnpm#434 umbrella](pnpm/pacquet#434) (`Proper optionalDependencies support`). Slices 1–4 (pnpm#439, pnpm#456, pnpm#467, pnpm#474) handle installability + the fetch-failure swallow. This slice closes the user-facing `--no-optional` flag: the CLI flag already exists and threads through `Install::dependency_groups`, but only `SymlinkDirectDependencies` and the on-disk `included` field actually honored it. The rest of the install pipeline walked `optional_dependencies` unconditionally, so the optional subtree was still extracted, linked, and built.

Mirrors upstream's depNode filter at [`installing/deps-installer/src/install/link.ts:109-111`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/link.ts#L109-L111):

```ts
if (!opts.include.optionalDependencies) {
  depNodes = depNodes.filter(({ optional }) => !optional)
}
```

## Changes

- **`SkippedSnapshots`**: gains a third disjoint subset `optional_excluded` alongside `installability` (slice 1) and `fetch_failed` (slice 4). The existing `contains` / `iter` already return the union, so every downstream consumer that filters by skip-set (`CreateVirtualStore`, `SymlinkDirectDependencies`, `BuildModules`, hoist, `link_bins`) now respects `--no-optional` through the same gate. `iter_installability` still returns only the persistent subset for `.modules.yaml.skipped` writes — `--no-optional` exclusions are transient (matching upstream's behavior of never updating `opts.skipped` at the filter site).
- **`InstallFrozenLockfile::run`**: iterates the lockfile snapshots once and inserts every `snap.optional == true` entry into the new subset when the dispatch list doesn't contain `DependencyGroup::Optional`. The `SnapshotEntry::optional` flag is set by the resolver only when the snapshot is reachable **exclusively** through optional edges, so a snapshot reachable through any non-optional edge carries `optional: false` and survives the filter — covers [`optionalDependencies.ts:712`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/optionalDependencies.ts#L712) (`dependency that is both optional and non-optional is installed`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants