Skip to content

kernel/generic: add kernelTests automatically#121518

Merged
kevincox merged 5 commits intoNixOS:masterfrom
Atemu:automatic-kernelTests
Aug 23, 2021
Merged

kernel/generic: add kernelTests automatically#121518
kevincox merged 5 commits intoNixOS:masterfrom
Atemu:automatic-kernelTests

Conversation

@Atemu
Copy link
Copy Markdown
Member

@Atemu Atemu commented May 2, 2021 •
edited
Loading

Motivation for this change

Tests for different kernels had to be added manually and many didn't have that done. Now, they're simply generated automatically.

I didn't build all tests again but the couple I did run were the same drv as before.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: kernel The Linux kernel 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels May 2, 2021
@Atemu Atemu requested a review from NeQuissimus May 2, 2021 14:43
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels May 2, 2021
@Atemu Atemu force-pushed the automatic-kernelTests branch from bc0ef90 to 0cbbd27 Compare May 6, 2021 13:19
@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented May 6, 2021

Fixed a rebasing error.

@Atemu Atemu force-pushed the automatic-kernelTests branch from 0cbbd27 to 2c21fd1 Compare June 16, 2021 11:19
@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented Jun 16, 2021

Fixed merge conflict. Could you review this @NeQuissimus?

@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented Jun 22, 2021

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Jun 22, 2021

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2 marvin-mk2 bot requested a review from thiagokokada June 22, 2021 18:46
@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Jun 25, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

3 similar comments
@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Jun 28, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Jul 2, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Jul 5, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@Atemu Atemu force-pushed the automatic-kernelTests branch from 2c21fd1 to 865d0cf Compare July 5, 2021 14:34
Atemu
Atemu commented Jul 5, 2021
View reviewed changes
pkgs/os-specific/linux/kernel/generic.nix Outdated
Comment on lines 189 to 203
Copy link
Copy Markdown
Member Author

@Atemu Atemu Jul 5, 2021

Choose a reason for hiding this comment

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

For some strange reason, the kernel in here does not have .override or .overrideDerivation, only .overrideAttrs.

@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented Jul 5, 2021

Rebased.

@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented Jul 5, 2021

/status needs_reviewer

@marvin-mk2 marvin-mk2 bot requested a review from kevincox July 5, 2021 14:37
@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Jul 8, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Aug 5, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@Atemu Atemu force-pushed the automatic-kernelTests branch from 865d0cf to 6d3e7b6 Compare August 6, 2021 10:09
@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented Aug 6, 2021

Rebased and removed the manually declared tests from all kernels (missed some initially)

@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Aug 9, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@Atemu
Copy link
Copy Markdown
Member Author

Atemu commented Aug 9, 2021

/status needs_reviewer

@mweinelt mweinelt requested a review from TredwellGit August 9, 2021 12:33
@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Aug 12, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@timokau
Copy link
Copy Markdown
Member

timokau commented Aug 12, 2021

I think Marvin broke @timokau. It should've gone and looked for a new reviewer but only added awaiting_reviewer back.

Sorry for the late response. I think the bot might have re-requested someone with a pending review request. That doesn't show up in the GitHub UI if I recall correctly (timokau/marvin-mk2#86).

It seems like the bot is not really sustainable with me as the sole maintainer given my current priorities. Maybe it would be better to shut it down. It would be sad to abort this experiment without exploring it a bit further, but that may still be better than the current state. The bot is too spammy in its current configuration and may actually put off contributors or generate unnecessary stress. I think I will post a comment to timokau/marvin-mk2#34 soon-ish and ask for peoples opinions.

We should probably continue this discussion there, since it's a bit off topic here. Unfortunately I don't have the spare capacity to review this right now.

This was referenced Aug 13, 2021
Open
Draft
@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Aug 16, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

2 similar comments
@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Aug 19, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@marvin-mk2
Copy link
Copy Markdown

marvin-mk2 bot commented Aug 22, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@kevincox
Copy link
Copy Markdown
Contributor

kevincox commented Aug 23, 2021

@Atemu looks like some merge conflicts. If you resolve them I'll merge.

Atemu added 5 commits August 23, 2021 19:57
@Atemu
nixosTests.kernel-generic: simplify
2b52f2b
@Atemu
nixosTests.kernel-generic: expose test-making functions
457653d 
Changed the name to be clearer, 'makeKernelTest' could imply that it wants a
kernel pkg as its arg while it actually needs a set of linuxPackages.
@Atemu
nixosTests.kernel-generic: add myself as maintainer
a3f6ff6
@Atemu
kernel/generic: add kernelTests automatically
0a43b6d 
Unfortunately, there seems to be no way of referencing an overridable version of
the package you're in, so it had to be stubed to work at all.

This isn't important for our current, very basic kernel nixosTests but might
become important when we add more sophisticated ones.
@Atemu
kernels: remove manually declared kernelTests
d4afb73 
It is not necessary to declare these manually anymore
@Atemu Atemu force-pushed the automatic-kernelTests branch from 6d3e7b6 to d4afb73 Compare August 23, 2021 18:07
@kevincox kevincox merged commit c3df805 into NixOS:master Aug 23, 2021
@kevincox
Copy link
Copy Markdown
Contributor

kevincox commented Aug 23, 2021

Thanks! and sorry for the poorly managed review process.

@Atemu Atemu deleted the automatic-kernelTests branch August 23, 2021 18:24
infinisil added a commit to infinisil/nixpkgs that referenced this pull request Dec 7, 2021
@infinisil
kernel: Fix overriding
f0fec8e 
This was reported in NixOS#111504 but
further complicated in NixOS#121518

This should clean it up
@infinisil infinisil mentioned this pull request Dec 7, 2021
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NeQuissimus NeQuissimus NeQuissimus approved these changes

@thiagokokada thiagokokada Awaiting requested review from thiagokokada

@bennyandresen bennyandresen Awaiting requested review from bennyandresen

@kevincox kevincox Awaiting requested review from kevincox

@jonringer jonringer Awaiting requested review from jonringer

@TredwellGit TredwellGit Awaiting requested review from TredwellGit

Assignees

No one assigned

Labels

6.topic: kernel The Linux kernel 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Atemu @kevincox @timokau @NeQuissimus