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.

Sign up for GitHub

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

Jump to bottom

kernel: fix passthru.tests #247520

Merged
merged 2 commits into from
Aug 8, 2023
Merged

kernel: fix passthru.tests #247520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6229f0b
all-tests: exclude passthru attributes from test discovery
Atemu Aug 6, 2023
a0dcabb
kernel: fix passthru.tests
Atemu Aug 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nixos/tests/all-tests.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ let
if isAttrs val
then
if hasAttr "test" val then callTest val
else mapAttrs (n: s: discoverTests s) val
else mapAttrs (n: s: if n == "passthru" then s else discoverTests s) val
Copy link
Member

Choose a reason for hiding this comment

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

iirc this code is used by both release.nix and nixosTests, and it has to deal with all tests, which are still written in a number of styles.
So this is hard to review; can't really tell the implications without a lot of manual testing.

In my opinion release.nix should be rewritten to follow the more easily understood structure of nixosTests, but until that's done, we need to consider that both code paths used this ill-conceived semi-generic code.

All that aside, we should really try to avoid the proliferation of the term "passthru". Unfortunately it's the mechanism we have to use with mkDerivation, but it is by itself meaningless, and its presence in the package attribute set is an implementation detail. So if we're going to accept a fix for the problem it would be great for it not to use the passthru attribute.

As for rewriting release.nix, ideally we'd have something like #176557 which has a nice and general solution for this particular problem of overriding a test to use a specific package. But the actual implementation was a bit messy, which is why I think it should be rewritten. It would probably still resemble the documented example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this isn't a great solution. It's really more of a workaround and I don't like adding special cases like this but I don't really see another way without refactoring the entire tests infrastructure which I don't feel comfortable doing and even you doing it would take quite some time (see #176557).

By using "passthru" as the special attribute's name, I hope to make it somewhat clear that it's intended for passing through values rather than exposing tests. Do you have a better name for it?

All this should change is that anything organised under a passthru isn't discovered as a test. My hope is that no Nixpkgs contributor, knowing the meaning of passthru in mkDerivation, would organise an actual test under "passthru", only values for, well, passing through.

else if isFunction val
then
# Tests based on make-test-python.nix will return the second lambda
Expand Down
6 changes: 4 additions & 2 deletions nixos/tests/kernel-generic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ let
};

in mapAttrs (_: lP: testsForLinuxPackages lP) kernels // {
inherit testsForLinuxPackages;
passthru = {
inherit testsForLinuxPackages;

testsForKernel = kernel: testsForLinuxPackages (pkgs.linuxPackagesFor kernel);
testsForKernel = kernel: testsForLinuxPackages (pkgs.linuxPackagesFor kernel);
};
}
2 changes: 1 addition & 1 deletion pkgs/os-specific/linux/kernel/generic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ let
+ toString (lib.attrNames (if lib.isAttrs args then args else args {}))
) overridableKernel;
};
in [ (nixosTests.kernel-generic.testsForKernel overridableKernel) ] ++ kernelTests;
in [ (nixosTests.kernel-generic.passthru.testsForKernel overridableKernel) ] ++ kernelTests;
};

finalKernel = lib.extendDerivation true passthru kernel;
Expand Down