pnpm.fetchDeps: ensure consistent permissions, add versioning#422975
pnpm.fetchDeps: ensure consistent permissions, add versioning#422975Scrumplex merged 4 commits intoNixOS:masterfrom
Conversation
912d318 to
ebfe10f
Compare
|
Do we need to also version (I'm still a bit short on time to do proper research) |
|
What if we store |
Great idea, much better than having to duplicate the fetcher version and doing something like this: nativeBuildInputs = [
(pnpm_10.configHook { fetcherVersion = 2; })
];
# or this:
PNPM_FETCHER_VERSION = 2; |
ebfe10f to
e8f1627
Compare
Scrumplex
left a comment
There was a problem hiding this comment.
Currently, the first commit causes every pnpm package to fail evaluating. Perhaps we should first add the arg and default it to 1, then do the treewide change and then enforce setting the arg
e8f1627 to
efe2575
Compare
252fe9b to
4c5f683
Compare
4c5f683 to
764f78a
Compare
|
Looks good from my side 👍 ❓ Out of curiosity: Did you already evaluate how many hashes would actually change if Since the original issue seemed OS-dependent and only occurred under specific circumstances, I would have expected Hydra builds to have failed if wrong hashes were submitted in PRs. |
|
I think this should be backported to 25.05. Because of the release notes change, this will have to be done manually. |
| # NOTE: For reasons not yet fully understood, pnpm might create files with | ||
| # inconsistent permissions, for example inside the ubuntu-24.04 | ||
| # github actions runner. |
There was a problem hiding this comment.
This is probably related to the sandbox. What installed was used? The cachix one with the sanbox or another one? I think I've hit the same problem before, see NuschtOS/search@2ab7909
There was a problem hiding this comment.
In #350063 nix-quick-install was used and the pnpm.fetchDeps generated result had 555 permissions on a ubuntu-24.04 runner and 444 permissions on ubuntu-22.04 runner (and their local env)
I'm not sure which runner was used in #422889, cc @jfraudeau @jmoggr
There was a problem hiding this comment.
Yep, when we tried it with nix-quick-install we also got the issue. cachix/install-nix works. I think the major difference is the sandbox.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/86 |
Required following: NixOS/nixpkgs#422975
Fixes #422889
Closes #350063
Feedback is welcome about the versioning interface :)
This is very WIP as I have to go now, haven't even tested and may need to do changes for theconfigHooktoocc @jfraudeau
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.