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.

Sign up for GitHub

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

Jump to bottom

pnpm: 9.12.1 -> 9.12.2 #349093

Merged
merged 6 commits into from
Oct 20, 2024
Merged

pnpm: 9.12.1 -> 9.12.2 #349093

merged 6 commits into from
Oct 20, 2024

Conversation

gepbird
Copy link
Contributor

@gepbird gepbird commented Oct 16, 2024

Release: https://github.com/pnpm/pnpm/releases/tag/v9.12.2

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@gepbird
Copy link
Contributor Author

gepbird commented Oct 16, 2024

Most packages fail with  ERR_PNPM_NO_OFFLINE_TARBALL  A package is missing from the store but cannot download it in offline mode. The missing package may be downloaded from https://registry.npmjs.org/xxx/-/xxx.tgz.
I'll investigate this on the weekend when I'll have time.

Result of nixpkgs-review pr 349093 run on x86_64-linux 1

42 packages failed to build:
  • astro-language-server
  • authelia
  • autoprefixer
  • cdxgen
  • clash-verge-rev
  • en-croissant
  • follow
  • gitbutler
  • goofcord
  • heroic
  • heroic-unwrapped
  • homebox
  • legcord
  • lemmy-ui
  • metacubexd
  • misskey
  • n8n
  • overlayed
  • pgrok
  • pgrok.server
  • postcss-cli
  • renovate
  • rsshub
  • siyuan
  • sketchybar-app-font
  • stylelint-lsp
  • surrealist
  • syncyomi
  • taler-challenger
  • taler-merchant
  • taler-sync
  • taler-wallet-core
  • teleport (teleport_16)
  • teleport.client (teleport_16.client)
  • vencord
  • vencord-web-extension
  • vesktop
  • vikunja
  • webcord-vencord
  • wrangler
  • youtube-music
  • zenn-cli
2 packages built:
  • pnpm
  • pot

@NyCodeGHG
Copy link
Member

Looks like upstream changed their store format, which broke most of our FOD hashes :/

@gepbird
Copy link
Contributor Author

gepbird commented Oct 18, 2024

Looks like upstream changed their store format, which broke most of our FOD hashes :/

Running a diff for example on the old and new astro-language-server.pnpmDeps, shows only a file name change, file content is the same:

Only in /nix/store/mbawgip8msszvjkqhsjnhv0bwxj89cqv-astro-language-server-pnpm-deps/v3/files/8a: 4e547ef22ddf054acadfe74ec7e8a083af03d7fdfe918fcf0b41d155f0e3163c58f94097e4fffeffa628ded2de364f9620d0208bbe7e5ac9c01073c374aa26
Only in /nix/store/kn62rkbq14mkah57sadr3kzc58hn8dm4-astro-language-server-pnpm-deps/v3/files/8a: 4e547ef22ddf054acadfe74ec7e8a083af03d7fdfe918fcf0b41d155f0e3163c58f94097e4fffeffa628ded2de364f9620d0208bbe7e5ac9c01073c374aa26-exec

I'll update all the pnpm hashes soon.

@gepbird
Copy link
Contributor Author

gepbird commented Oct 19, 2024

teleport failure is unrelated: https://hydra.nixos.org/build/274984883

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349093

x86_64-linux

❌ 2 packages failed to build:
  • teleport (teleport_16)
  • teleport.client (teleport_16.client)
✅ 42 packages built:
  • astro-language-server
  • authelia
  • autoprefixer
  • cdxgen
  • clash-verge-rev
  • en-croissant
  • follow
  • gitbutler
  • goofcord
  • heroic
  • heroic-unwrapped
  • homebox
  • legcord
  • lemmy-ui
  • metacubexd
  • misskey
  • n8n
  • overlayed
  • pgrok
  • pgrok.server
  • pnpm
  • postcss-cli
  • pot
  • renovate
  • rsshub
  • siyuan
  • sketchybar-app-font
  • stylelint-lsp
  • surrealist
  • syncyomi
  • taler-challenger
  • taler-merchant
  • taler-sync
  • taler-wallet-core
  • vencord
  • vencord-web-extension
  • vesktop
  • vikunja
  • webcord-vencord
  • wrangler
  • youtube-music
  • zenn-cli

@gepbird gepbird marked this pull request as ready for review October 19, 2024 07:20
@gepbird gepbird requested a review from Scrumplex October 19, 2024 07:20
@Scrumplex
Copy link
Member

podman-desktop from #343648 needs to be updated

@Scrumplex
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349093

x86_64-linux

❌ 3 packages failed to build:
  • podman-desktop
  • teleport (teleport_16)
  • teleport.client (teleport_16.client)
✅ 42 packages built:
  • astro-language-server
  • authelia
  • autoprefixer
  • cdxgen
  • clash-verge-rev
  • en-croissant
  • follow
  • gitbutler
  • goofcord
  • heroic
  • heroic-unwrapped
  • homebox
  • legcord
  • lemmy-ui
  • metacubexd
  • misskey
  • n8n
  • overlayed
  • pgrok
  • pgrok.server
  • pnpm (pnpm_9)
  • postcss-cli
  • pot
  • renovate
  • rsshub
  • siyuan
  • sketchybar-app-font
  • stylelint-lsp
  • surrealist
  • syncyomi
  • taler-challenger
  • taler-merchant
  • taler-sync
  • taler-wallet-core
  • vencord
  • vencord-web-extension
  • vesktop
  • vikunja
  • webcord-vencord
  • wrangler
  • youtube-music
  • zenn-cli

@gepbird gepbird marked this pull request as draft October 19, 2024 21:41
@gepbird gepbird marked this pull request as ready for review October 19, 2024 21:52
@gepbird
Copy link
Contributor Author

gepbird commented Oct 19, 2024

Updated podman-desktop and metacubexd hashes

@Scrumplex
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349093

x86_64-linux

❌ 4 packages failed to build:
  • metacubexd
  • podman-desktop
  • teleport (teleport_16)
  • teleport.client (teleport_16.client)
✅ 41 packages built:
  • astro-language-server
  • authelia
  • autoprefixer
  • cdxgen
  • clash-verge-rev
  • en-croissant
  • follow
  • gitbutler
  • goofcord
  • heroic
  • heroic-unwrapped
  • homebox
  • legcord
  • lemmy-ui
  • misskey
  • n8n
  • overlayed
  • pgrok
  • pgrok.server
  • pnpm
  • postcss-cli
  • pot
  • renovate
  • rsshub
  • siyuan
  • sketchybar-app-font
  • stylelint-lsp
  • surrealist
  • syncyomi
  • taler-challenger
  • taler-merchant
  • taler-sync
  • taler-wallet-core
  • vencord
  • vencord-web-extension
  • vesktop
  • vikunja
  • webcord-vencord
  • wrangler
  • youtube-music
  • zenn-cli

Scrumplex
Scrumplex approved these changes Oct 20, 2024
View reviewed changes
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

LGTM. We should notify open PRs about this change so that they can update their hashes.

@gepbird
Copy link
Contributor Author

gepbird commented Oct 20, 2024

LGTM. We should notify open PRs about this change so that they can update their hashes.

I'll do that after this is merged, also they should get a merge conflict for the hashes too.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 20, 2024
@Scrumplex Scrumplex merged commit feb995a into NixOS:master Oct 20, 2024
31 of 32 checks passed
This was referenced Oct 20, 2024
Merged
Open
Draft
Open
Draft
@gepbird gepbird deleted the pnpm-9.12.2 branch October 20, 2024 18:43
@gepbird
Copy link
Contributor Author

gepbird commented Oct 20, 2024 •
edited
Loading

Thanks for notifying them about the hash changes!

I also checked which PR got broken by this and found the same ones as you did, except for dorion.

Edit: nevermind, dorion was just quickly fixed
Edit2: turns out GitHub search is not as good as I thought and we missed some, I'll check for all packages this PR touched later

This was referenced Oct 20, 2024
Open
Closed
@Scrumplex Scrumplex mentioned this pull request Oct 20, 2024
Merged
13 tasks
@Atemu Atemu mentioned this pull request Oct 21, 2024
Merged
13 tasks
@NotAShelf NotAShelf mentioned this pull request Oct 26, 2024
Merged
13 tasks
kira-bruneau added a commit to kira-bruneau/nur-packages that referenced this pull request Oct 26, 2024
@kira-bruneau
ggt: update pnpm dependency hash
4a153b9 
The latest release of pnpm (9.12.2) includes a fix that changes the
output hash of dependencies

- NixOS/nixpkgs#349093
- pnpm/pnpm#8625
@gepbird gepbird mentioned this pull request Oct 29, 2024
Merged
13 tasks
@Aleksanaa
Copy link
Member

Is this hash change unavoidable? This type of behavior should be kept to a minimum.

@dotlambda
Copy link
Member

Looks like upstream changed their store format, which broke most of our FOD hashes :/

This is why pnpm.fetchDeps should never have been merged. Imagine thousands of packages using it and a change like this quickly becomes unsustainable.
Somebody urgently needs to write a different FOD fetcher, simiilar to the one for npm, whose hash doesn't change upon version bumps.

@dotlambda
Copy link
Member

dotlambda commented Oct 29, 2024 •
edited
Loading

Alternatively, can we ask upstream to commit to not breaking the hashes outside of major version bumps? And write a CI test for upstream that ensures this?

@Aleksanaa
Copy link
Member

Running a diff for example on the old and new astro-language-server.pnpmDeps, shows only a file name change, file content is the same:

Only in /nix/store/mbawgip8msszvjkqhsjnhv0bwxj89cqv-astro-language-server-pnpm-deps/v3/files/8a: 4e547ef22ddf054acadfe74ec7e8a083af03d7fdfe918fcf0b41d155f0e3163c58f94097e4fffeffa628ded2de364f9620d0208bbe7e5ac9c01073c374aa26
Only in /nix/store/kn62rkbq14mkah57sadr3kzc58hn8dm4-astro-language-server-pnpm-deps/v3/files/8a: 4e547ef22ddf054acadfe74ec7e8a083af03d7fdfe918fcf0b41d155f0e3163c58f94097e4fffeffa628ded2de364f9620d0208bbe7e5ac9c01073c374aa26-exec

But is it possible for us to at least "fixup" them to something simpler, so it doesn't matter?

Or is this completely happening in upstream servers and we don't have any control of it?

@Scrumplex
Copy link
Member

But is it possible for us to at least "fixup" them to something simpler, so it doesn't matter?

If we had renamed these files back to what pnpm < 9.12.2 used, the configure hook would fail installing dependencies as pnpm 9.12.2 wouldn't find the expected files in its store.

Maybe we could have fixed them up and then unfixed them during configuration. But I think that wouldn't be very maintainable in the long run.

Alternatively, can we ask upstream to commit to not breaking the hashes outside of major version bumps? And write a CI test for upstream that ensures this?

I think this is a good way to go. If upstream agrees that a change to the store is a breaking change, they might just bump to a new major version every time the store format changes. In those cases, we can just release a new pnpm_<major> package and update all references in Nixpkgs to the previous version until packages update.

@Scrumplex Scrumplex mentioned this pull request Oct 29, 2024
Open
4 tasks
@Scrumplex
Copy link
Member

I created an issue upstream to discuss this: pnpm/pnpm#8713

@Aleksanaa
Copy link
Member

One more thing: we may need a release note for this, as it's somehow breaking.

@gepbird
Copy link
Contributor Author

gepbird commented Oct 29, 2024 •
edited
Loading

This is why pnpm.fetchDeps should never have been merged. Imagine thousands of packages using it and a change like this quickly becomes unsustainable.
Somebody urgently needs to write a different FOD fetcher, simiilar to the one for npm, whose hash doesn't change upon version bumps.

Until we don't have that many pnpm packages (currently there are about 50), mass hash changes are not the end of the world if it only happens 1 or 2 times in a year.

Package update PRs that use pnpm.fetchDeps get a merge conflict, so most likely the PR author will regenerate the hash. From what I've seen it didn't cause that big of an issue and was dealt with by active PR authors.

Hovewer package init PRs won't get merge conflicts and we need to hunt these down and notify the author, this is where we could've done better. Searching for pnpm.fetchDeps or even pnpm in nixpkgs/pulls is not sufficient. With a tool that could properly search in PR diffs, this could've been mitigated (at least in nixpkgs and not in nur or other projects relying on this). Do we have anything like that for searching PR diffs?

Alternatively, can we ask upstream to commit to not breaking the hashes outside of major version bumps? And write a CI test for upstream that ensures this?

Exactly, this should've been only released in the next major version.

One more thing: we may need a release note for this, as it's somehow breaking.

Mostly for people using pnpm.fetchDeps outside of nixpkgs, this will be very helpful. Can someone make a PR? I won't have the time for that in the near future.

This was referenced Nov 1, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Scrumplex Scrumplex Scrumplex approved these changes

@infinisil infinisil Awaiting requested review from infinisil

@LeSuisse LeSuisse Awaiting requested review from LeSuisse

@mweinelt mweinelt Awaiting requested review from mweinelt

@philiptaron philiptaron Awaiting requested review from philiptaron

@risicle risicle Awaiting requested review from risicle

Assignees
No one assigned
Labels
6.topic: nodejs 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gepbird @NyCodeGHG @Scrumplex @Aleksanaa @dotlambda @wegank