Skip to content

fix: install packages with incompatible libc#8569

Merged
zkochan merged 13 commits into
mainfrom
install-multiple-bindings-of-different-libc-7362
Sep 27, 2024
Merged

fix: install packages with incompatible libc#8569
zkochan merged 13 commits into
mainfrom
install-multiple-bindings-of-different-libc-7362

Conversation

@KSXGitHub

@KSXGitHub KSXGitHub commented Sep 25, 2024

Copy link
Copy Markdown
Contributor

Fixes #7362

Comment thread pkg-manager/plugin-commands-installation/src/install.ts Outdated
cert: opts.cert,
fullMetadata,
filterMetadata: fullMetadata,
filterMetadata,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you should filter metadata always. Just update the code that filters to keep the libc field.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does mean that the bug won't immediately be fixed unless the user clears the old metadata cache.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can bump the version in FULL_FILTERED_META_DIR

@KSXGitHub KSXGitHub requested a review from zkochan September 26, 2024 09:49
@KSXGitHub KSXGitHub marked this pull request as ready for review September 26, 2024 09:50
workspace?: boolean
includeOnlyPackageFiles?: boolean
prepareExecutionEnv: PrepareExecutionEnv
fetchFullMetadata?: boolean | null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is null needed? This already support undefined.

Suggested change
fetchFullMetadata?: boolean | null
fetchFullMetadata?: boolean

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think ? true : null would be clearer than ?? true.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why would you use ?? true? That wouldn't work correctly. You'd use ? true : undefined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I meant && true. (I got the symbol mixed up)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, well use ? true : undefined. That is less confusing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I chose instead to explicitly specify the type as true | undefined (7cd1b89).

I think ? true : undefined would risk future me going back and do refactor or even somebody even change it to actual boolean due to misunderstanding underfined as false.

@KSXGitHub KSXGitHub requested a review from zkochan September 27, 2024 15:45
@zkochan zkochan merged commit 83681da into main Sep 27, 2024
@zkochan zkochan deleted the install-multiple-bindings-of-different-libc-7362 branch September 27, 2024 20:46
u1f992 added a commit to vivliostyle/vivliostyle-cli that referenced this pull request May 11, 2026
pnpm/pnpm#9654 (and the earlier pnpm/pnpm#7362, partially addressed by
pnpm/pnpm#8569) leaves every optional native binding from the lockfile
on disk when installing via --frozen-lockfile: pnpm honours `os` and
`cpu` from the binding's package.json but ignores `libc`, so a glibc
slim image ends up shipping both `@napi-rs/canvas-linux-x64-gnu` and
`@napi-rs/canvas-linux-x64-musl` (and the same for @rollup/rollup,
etc.). The lockfile pins the musl variants only as a side effect of
the optional-dep declaration -- it never intended them to ship to a
glibc target -- so we drop them by hand after pnpm finishes and the
--frozen-lockfile reproducibility contract is unaffected.

Peer-dep dribble (e.g. valibot@1.2.0's optional `typescript` peer that
pnpm 8+'s default `auto-install-peers=true` baked into the lockfile)
is intentionally NOT pruned here. pnpm pins `autoInstallPeers` in the
lockfile and refuses --frozen-lockfile if it changes
(ERR_PNPM_LOCKFILE_CONFIG_MISMATCH), so deleting those entries would
quietly break the lockfile contract this slim build still claims.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pnpm install multiple bindings of different libc

2 participants