Enable enforcement of pkgs/by-name for new packages (by updating the pinned version)#281835
Enable enforcement of pkgs/by-name for new packages (by updating the pinned version)#281835infinisil merged 2 commits intoNixOS:masterfrom
pkgs/by-name for new packages (by updating the pinned version)#281835Conversation
Especially needed because NIX_PATH is set to something custom in the nix-shell of nixpkgs-check-by-name
infinisil
requested review from
DavHau,
Ericson2314,
philiptaron,
roberth and
tomberek
infinisil
added
the
1.severity: significant
ofborg
bot
added
10.rebuild-darwin: 0
delroth
added
the
12.approvals: 1
SuperSandro2000
left a comment
SuperSandro2000
left a comment
There was a problem hiding this comment.
I cherry-picked the necessary commits from master and this PR and run it on top of #282015 on my nixos-unstable branch and it looks like it does what it should.
➜ ./run-local.sh nixos-unstable
Warning: Dirty tree, uncommitted changes won't be taken into account
Using HEAD commit 9c80e56ee93bc0448c2b3f2ac64937da80fe1eb4
Creating Git worktree for the HEAD commit in /run/user/1000/tmp.EAX8mafz39/merged.. Done
Fetching base branch nixos-unstable to compare against.. 842d9d80cfd4560648c785f8a4e6f3b096790e19
Creating Git worktree for the base branch in /run/user/1000/tmp.EAX8mafz39/base.. Done
Merging base branch into the HEAD commit in /run/user/1000/tmp.EAX8mafz39/merged.. 9c80e56ee93bc0448c2b3f2ac64937da80fe1eb4
Reading pinned nixpkgs-check-by-name revision from pinned-tool.json.. 842d9d80cfd4560648c785f8a4e6f3b096790e19
Creating Git worktree for the nixpkgs-check-by-name revision in /run/user/1000/tmp.EAX8mafz39/tool-nixpkgs.. Done
Building/fetching nixpkgs-check-by-name..
/nix/store/8habk3j25bs2a34zn5q5p17b9dl3fywg-nixpkgs-check-by-name
Running nixpkgs-check-by-name..
pkgs.nodejs-16_x: This top-level package was previously defined in pkgs/by-name/no/nodejs-16_x/package.nix, but is now manually defined as `callPackage ./pkgs/development/web/nodejs/v16.nix { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
pkgs.nodejs-slim-16_x: This top-level package was previously defined in pkgs/by-name/no/nodejs-slim-16_x/package.nix, but is now manually defined as `callPackage ./pkgs/development/web/nodejs/v16.nix { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
pkgs.nodejs-slim_16: This top-level package was previously defined in pkgs/by-name/no/nodejs-slim_16/package.nix, but is now manually defined as `callPackage ./pkgs/development/web/nodejs/v16.nix { ... }` (e.g. in `pkgs/top-level/all-packages.nix`). While the manual `callPackage` is still needed, it's not necessary to move the package files.
pkgs.nodejs_16: This top-level package was previously defined in pkgs/by-name/no/nodejs_16/package.nix, but is now manually defined as `callPackage ./pkgs/development/web/nodejs/v16.nix { }` (e.g. in `pkgs/top-level/all-packages.nix`). Please move the package back and remove the manual `callPackage`.
Validation failed, see above errors
Cleaning up.. Done
I reverted the nodejs_16 removal in that branch because I still need it.
This just raises on concern: Do we now need to update the rev everytime a new compiler/interpreter version is released?
delroth
added
12.approvals: 2
Not sure what you mean, those validation errors shouldn't occur if you removed nodejs. And I can't see anything relating to nodejs in that PR.
Also not entirely sure what you mean. The |
I am reverting a removal in my branch on top of that PR , so there is a new package added and the script fails like expected.
I confused that with the reference point the tool is comparing to was. I just have |
I see, makes sense!
Indeed that would be the new way to do it. It could be refactored to a
CI compares the PR head against the base branch of the PR, so And here's a hack if you really need to: PRs that only fail this new "should be moved to pkgs/by-name" check can still be merged, they won't break master. This can still be used in case we discover that |
|
Let's go! I'll stay online for the next couple hours to tend to any potential fallout |
|
Thanks for the explanations! |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
5 participants
Context
This is the final follow-up to #275539, which has much more context. At the time of that PR, the changes were supposed to be automatically used in the next channel update, but after discovering a problem (#281390), I quickly changed how this works in #281374: Now one needs to manually update the pinned version of the tooling before it ends up getting used in CI.
Description of changes
This is the first PR that updates the pinned tooling. This is done by running
update-pinned-tool.sh.Immediately following the merge of this PR, all CI runs against
masterwill start to enforcepkgs/by-namefor new packages.Note that CI will test the new tooling right inside this PR itself as well, so if CI is green, this should be good to merge.
Things done
pkgs/by-namefail:pkgs/by-nametweag/nixpkgs#82Add a 👍 reaction to pull requests you find important.