Skip to content

Comments

[RFC 140 2a] pkgs/by-name: Enforce for new packages#275539

Merged
infinisil merged 6 commits intoNixOS:masterfrom
tweag:by-name-enforce
Jan 15, 2024
Merged

[RFC 140 2a] pkgs/by-name: Enforce for new packages#275539
infinisil merged 6 commits intoNixOS:masterfrom
tweag:by-name-enforce

Conversation

@infinisil
Copy link
Member

@infinisil infinisil commented Dec 20, 2023 •
edited
Loading

Note

An open call for the final review and merge was held on 2024-01-15, see #275539 (comment)

Context

This is part of the effort to implement RFC 140, aka pkgs/by-name. While previous PRs already enabled users to define packages using pkgs/by-name, there's still thousands of packages still using all-packages.nix, and new ones added every day.

This PR is part of the RFC 140 migration plan, building on top of #272395 and #274591.

This work is sponsored by Antithesis and Tweag ✨

Changes

This PR prevents new packages using pkgs.callPackage from being defined not in pkgs/by-name. Instead the pkgs/by-name directory has to be used instead. This the A-side of Part 2 of the RFC.

This doesn't affect packages defined using alternate callPackage's like libsForQt5.callPackage or callPackages, which pkgs/by-name cannot handle (yet?).

Note

The effects of this PR will only be seen once the nixos-unstable channel updates, since its pre-built version of the tooling is used for CI.

Things done

  • Tests updated
    • Test to ensure that new packages using callPackage but not using pkgs/by-name throw an error
    • Test to ensure that a package moved out of pkgs/by-name throws an error
    • Test to ensure that new packages using an alternate callPackage doesn't give an error.
  • Tested on the entirety of Nixpkgs
    • Ensured okay performance (about 25 single-core seconds on my machine)
  • Tidy code
  • Clean history

Add a 👍 reaction to pull requests you find important.

@infinisil infinisil added this to the RFC 140 milestone Dec 20, 2023
@infinisil infinisil mentioned this pull request Dec 20, 2023
Open
@infinisil infinisil added the 1.severity: significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Dec 20, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Dec 20, 2023
@infinisil infinisil mentioned this pull request Dec 20, 2023
Merged
@donovanglover donovanglover mentioned this pull request Dec 21, 2023
Closed
13 tasks
@infinisil infinisil mentioned this pull request Dec 21, 2023
Merged
4 tasks
@infinisil infinisil force-pushed the by-name-enforce branch 5 times, most recently from 1c31376 to 41b1926 Compare January 5, 2024 00:50
@infinisil infinisil mentioned this pull request Jan 5, 2024
Merged
1 task
@infinisil
Copy link
Member Author

infinisil commented Jan 5, 2024

This PR is now theoretically ready, but since the changes got a bit big, I split off everything not directly related to this PR into #278805, which should be merged first.

Ericson2314
Ericson2314 reviewed Jan 8, 2024
View reviewed changes
pkgs/by-name/README.md Outdated
Comment on lines 6 to 8
Copy link
Member

@Ericson2314 Ericson2314 Jan 8, 2024

Choose a reason for hiding this comment

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

I think it is still better to lead with the positive part. Something like

Packages found in the name-based structure are automatically included, without needing to be added in a file like all-packages.nix. They must only occur in file if they require overriding the default value of an implicit attribute (see below).

Copy link
Member Author

@infinisil infinisil Jan 10, 2024

Choose a reason for hiding this comment

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

I changed the wording a little bit, but this is essentially applied now :)

@nixos-discourse
Copy link

nixos-discourse commented Jan 9, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-09-nixpkgs-architecture-team-meeting-47/38037/1

@infinisil infinisil requested a review from philiptaron January 9, 2024 22:02
@infinisil infinisil marked this pull request as ready for review January 9, 2024 22:03
philiptaron
philiptaron reviewed Jan 9, 2024
View reviewed changes
philiptaron
philiptaron approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I took a thorough look. The tests are quite clear. Nice work. I don't have any code related comments, just copy-editing. Might also run a RFC101 formatter over these Nix files just for giggles.

pkgs/test/nixpkgs-check-by-name/src/nixpkgs_problem.rs Outdated
Copy link
Contributor

@philiptaron philiptaron Jan 9, 2024

Choose a reason for hiding this comment

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

For my notes: pkgs/test/nixpkgs-check-by-name/tests/new-package-non-by-name/expected is the test of this.

pkgs/test/nixpkgs-check-by-name/src/eval.rs Outdated
Comment on lines +197 to +204
Copy link
Contributor

@philiptaron philiptaron Jan 9, 2024

Choose a reason for hiding this comment

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

Future ratchets might try to drive this number down, I might imagine.

Copy link
Member Author

@infinisil infinisil Jan 9, 2024

Choose a reason for hiding this comment

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

Oh yeah, I thought about implementing an eval-successful ratchet, but I think this should be a future PR. Then also it's also not pkgs/by-name-specific anymore and the tool could use a rename.

Copy link
Contributor

@philiptaron philiptaron Jan 10, 2024

Choose a reason for hiding this comment

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

One only needs to look at the list of things detected by NixpkgsProblems to realize we've backed ourselves into a damn fine nixpkgs linter framework. 🚀

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 9, 2024
@infinisil
tests.nixpkgs-check-by-name: Basic info for non-by-name attributes
69fc71a 
In a future commit this will be extended
@infinisil
tests.nixpkgs-check-by-name: Enforce pkgs/by-name for new packages
57afdc5
@infinisil
tests.nixpkgs-check-by-name: Sort the eval validation results
cbff028 
Not that important, but nice.
Also adds a nice test case show-casing the two current ratchet checks at
once.
@infinisil infinisil mentioned this pull request Jan 16, 2024
Merged
@infinisil
Copy link
Member Author

infinisil commented Jan 16, 2024 •
edited
Loading

Unfortunately this has a bug that needs to be addressed. Without doing anything, all PRs to master would start failing CI in like 1 day once nixos-unstable updates. I opened this PR to prevent this: #281374

Here's a follow up to fix the bug and improve CI to prevent this in the future: #281390

infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 16, 2024
@infinisil
tests.nixpkgs-check-by-name: Don't error for pkgs/by-name aliases
0f27917 
This was an oversight in NixOS#275539
not accounted for, which would've failed in CI
@infinisil infinisil mentioned this pull request Jan 16, 2024
Merged
@infinisil infinisil mentioned this pull request Jan 18, 2024
Merged
@infinisil
Copy link
Member Author

infinisil commented Jan 18, 2024

Now that nixos-unstable contains the latest fixes, here's the final follow-up PR: #281835 which starts enabling the enforcement!

@infinisil infinisil mentioned this pull request Jan 18, 2024
Closed
infinisil added a commit that referenced this pull request Jan 19, 2024
@infinisil
check-by-name: Update pinned tooling
bfe48c9 
Includes #275539
@infinisil infinisil mentioned this pull request Jan 19, 2024
Merged
13 tasks
@h7x4 h7x4 mentioned this pull request Jan 20, 2024
Merged
13 tasks
mkg20001 pushed a commit to mkg20001/nixpkgs that referenced this pull request Jan 21, 2024
@infinisil @mkg20001
check-by-name: Update pinned tooling
abb6cde 
Includes NixOS#275539
This was referenced Jan 21, 2024
Merged
Merged
peterhoeg pushed a commit to peterhoeg/nixpkgs that referenced this pull request Jan 22, 2024
@infinisil @peterhoeg
check-by-name: Update pinned tooling
20e9881 
Includes NixOS#275539
This was referenced Feb 1, 2024
Merged
Closed
@lelgenio lelgenio mentioned this pull request Feb 7, 2024
Merged
13 tasks
@infinisil infinisil mentioned this pull request Feb 7, 2024
Merged
1 task
@arikgrahl arikgrahl mentioned this pull request Feb 12, 2024
Merged
13 tasks
infinisil added a commit to NixOS/nixpkgs-vet that referenced this pull request Mar 18, 2024
@infinisil
tests.nixpkgs-check-by-name: Don't error for pkgs/by-name aliases
08f591a 
This was an oversight in NixOS/nixpkgs#275539
not accounted for, which would've failed in CI
infinisil added a commit to NixOS/nixpkgs-vet that referenced this pull request Mar 18, 2024
@infinisil
check-by-name: Update pinned tooling
e34551c 
Includes NixOS/nixpkgs#275539
@AndersonTorres AndersonTorres mentioned this pull request Aug 10, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philiptaron philiptaron philiptaron approved these changes

@roberth roberth roberth approved these changes

@Ericson2314 Ericson2314 Ericson2314 approved these changes

Assignees

No one assigned

Labels

1.severity: significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

RFC 140

Development

Successfully merging this pull request may close these issues.

6 participants

@infinisil @nixos-discourse @philiptaron @roberth @Ericson2314 @delroth