Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: arborist libc manifest #7126

Merged
merged 2 commits into from
Jan 23, 2024
Merged

fix: arborist libc manifest #7126

merged 2 commits into from
Jan 23, 2024

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Jan 10, 2024

@styfle styfle marked this pull request as ready for review January 10, 2024 23:38
@styfle styfle requested a review from a team as a code owner January 10, 2024 23:38
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@wraithgar
Copy link
Member

A branch to fix the npm tests for install is in the works so things like this can be asserted. We have decided not to let that block this from shipping.

@wraithgar wraithgar merged commit 8382fb3 into npm:latest Jan 23, 2024
20 checks passed
@github-actions github-actions bot mentioned this pull request Jan 23, 2024
@styfle styfle deleted the fix-libc-arborist branch January 23, 2024 20:15
styfle added a commit to styfle/packagephobia that referenced this pull request Jan 24, 2024
Bumps to [`npm@10.4.0`](https://github.com/npm/cli/releases/tag/v10.4.0)
which adds a fix for [arborist](npm/cli#7126)
which will avoid installing multiple libc binaries and [correctly select
glibc vs musl](npm/rfcs#438) from the
optionalDependencies.
@rigor789
Copy link

rigor789 commented Apr 9, 2024

This change introduces issues when installing global packages that have transient dependencies like fsevents causing them to fail. I believe this to be an unexpected side-effect of this change. Provided more details in this issue: #7355

lukekarrys added a commit that referenced this pull request May 3, 2024
Somewhat related to #7276 and #7463. I don't think there is a reason to cache the promise here. And if we ever did choose to replace this with an LRUCache we would need to know the size of what we are caching which will be easier if we only cache the resulting manifest.

I also added a comment about why I think we are removing the license from manifests here. License was removed in #7126 which looks to be a purposeful change but I could not find a reason. Adding the license back in causes many snapshots to fail because the license is now present in lockfiles, so that's how I came up with the comment.
lukekarrys added a commit that referenced this pull request May 6, 2024
Somewhat related to #7276 and #7463. I don't think there is a reason to cache the promise here. And if we ever did choose to replace this with an LRUCache we would need to know the size of what we are caching which will be easier if we only cache the resulting manifest.

I also added a comment about why I think we are removing the license from manifests here. License was removed in #7126 which looks to be a purposeful change but I could not find a reason. Adding the license back in causes many snapshots to fail because the license is now present in lockfiles, so that's how I came up with the comment.
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.

4 participants