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

pnpm.fetchDeps: ensure consistent hashes by setting file permissions after fetching #350063

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obreitwi
Copy link
Contributor

@obreitwi obreitwi commented Oct 20, 2024

For reasons not yet completely understood, pnpm might create dependency files with inconsistent file permissions. Since file permissions influence the resulting hash, this PR attempts a workaround by ensuring consistency after the fact.

NOTE: I am unsure that this should be merged prior to understanding what causes pnpm to generate inconsistent file permissions in the first place.
Maybe this PR already helps others facing the same issue.

More details:
We tried to build a small pnpm-based application locally versus within Github actions.
Here we observed build failures due to differing hashes for pnpmDeps. After investigation of the actual derivation contents we discovered that on a local, Ubuntu 24.04.1 LTS-based laptops with multi-user nix installs, all files in the derivation either had 444 or 555, the same derivation within the ubuntu-24.04-based runner (with single user install via nix-quick-install) had 644/755 as permissions and in different quantities.
To make things even more confusing: The ubuntu-22.04 agent runner showed the same behavior as our local environment.

Detailed statistics.
environment permission count
Ubuntu 24.04 LTS-based laptop/ubuntu-22.04 Github actions runner 444 27511
Ubuntu 24.04 LTS-based laptop/ubuntu-22.04 Github actions runner 555 108
ubuntu-24.04 Github actions runner 444 27497
ubuntu-24.04 Github actions runner 555 122

Since file permissions are stored in the NAR-archives used to derive the hash of a fixed output derivation, this leads to inconsistencies depending on where a derivation is built.

Hence, we ensure a consistent file permission schema:

  • All files with -exec suffix have 555.
  • All other files have 444.
  • All folders have 555.

This schema was chosen because it as already upheld in most environments we tested (i.e. local multi user installs on Ubuntu 24.04.1 LTS and single user install via nix-quick-install in ubuntu-22.04-based Github action runners).

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.

@obreitwi obreitwi requested a review from Scrumplex October 20, 2024 17:19
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 20, 2024
@gepbird
Copy link
Contributor

gepbird commented Oct 20, 2024

A related bug in pnpm recently got fixed in pnpm/pnpm#8625, and got released in pnpm 9.12.2 4 days ago.
This pnpm version bump recently got merged into nixpkgs, which made pnpmDeps hashes change, see #349093 (comment).

Were you using the same pnpm version on the 22.04 and 24.04 ubuntu machines? Would you mind testing it with pnpm 9.12.2?

(The merge conflict was caused by a formatting change, nothing to worry about when rebasing)

@nix-owners nix-owners bot requested a review from gepbird October 20, 2024 21:26
@obreitwi
Copy link
Contributor Author

Thanks for the hint!

Were you using the same pnpm version on the 22.04 and 24.04 ubuntu machines? Would you mind testing it with pnpm 9.12.2?

Yes, we were using the same version in all cases, but it was 9.12.1.

I will check if the inconsistency still occurs with 9.12.2 and report back.

@obreitwi
Copy link
Contributor Author

Finally got around to check: pnpm v9.12.2 does not fix the issue, making this permissions fix still necessary to ensure compatibility between Github actions and local development.

For reasons not yet completely understood, `pnpm` might create
dependency files with inconsistent file permissions.

Since file permissions are stored in the NAR-archive used to derive the
hash of a fixed output derivation, this leads to inconsistencies
depending on where a derivation is built.

Hence, we ensure a consistent file permission scheme:
* All files with `-exec` suffix have 555.
* All other files have 444.
* All folders have 555.

This schema was chosen because it as already upheld in most environments
we tested.
@obreitwi obreitwi force-pushed the ojb/fix/fetch_pnpm_deps_permissions branch from aa29fd1 to 8bfd975 Compare December 1, 2024 14:11
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 1, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nodejs 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants