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

tests.nixpkgs-check-by-name: Fix for aliases to packages in pkgs/by-name and better testing #281390

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jan 16, 2024

Context

In #275539 I made an oversight which shows itself when you run the tool on the current Nixpkgs:

❯ nix-build -A tests.nixpkgs-check-by-name
/nix/store/k7wwnxbrb2r05g5pw7hxw71gg1gls1dr-nixpkgs-check-by-name
❯ result/bin/nixpkgs-check-by-name --base . .
pkgs.NSPlist: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
[...]
pkgs.uclibc: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
pkgs.win-virtio: This attribute is defined using `_internalCallByNamePackageFile`, which is an internal function not intended for manual use.
Validation failed, see above errors

See #281374 to prevent the broken tooling from being used on master, that should be merged fairly soon before the nixos-unstable channel updates in like a day.

The problem

The problem is that there's some detection logic that makes sure the internal _internalCallByNamePackageFile attribute isn't used by packages.

If such a top-level attribute that's not in pkgs/by-name was detected to use this, an error is thrown. However, the detection logic can't distinguish between aliases and non-aliases.

So it gives errors even for aliases to packages defined in pkgs/by-name, e.g.

win-virtio = virtio-win; # Added 2023-10-17

https://github.com/NixOS/nixpkgs/blob/5df0c94c5ed9c4b9664d63047f2f3a7f93c0bb43/pkgs/top-level/all-packages.nix#L28854

The solution

It would be non-trivial to improve the detection logic, so instead this PR just changes the tool to not give an error in these cases.

Furthermore, a passthru.tests is added, addressing #256789 and preventing future such problems. It should be triggered automatically after ofborg finishes evaluation of a PR with a tests.nixpkgs-check-by-name: ... commit.


Add a 👍 reaction to pull requests you find important.

@infinisil
Copy link
Member Author

infinisil commented Jan 16, 2024

@ofborg build tests.nixpkgs-check-by-name tests.nixpkgs-check-by-name.tests

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/ci-will-soon-enforce-pkgs-by-name-for-new-packages/38098/2

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 16, 2024
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.

Sad but makes sense.

@delroth delroth added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 16, 2024
Copy link
Contributor

@quantenzitrone quantenzitrone left a comment

Choose a reason for hiding this comment

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

all changes are trivial enough for me to understand
so LGTM

@delroth delroth added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Jan 17, 2024
@infinisil infinisil merged commit a48d8ea into NixOS:master Jan 17, 2024
42 of 43 checks passed
@infinisil infinisil deleted the by-name-alias-fix branch January 17, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When updating nixpkgs-check-by-name, it should be run on Nixpkgs
6 participants