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

linux-testing: default to latest if it is newer #130872

Closed
wants to merge 1 commit into from

Conversation

ikervagyok
Copy link
Contributor

Motivation for this change

Testing users probably prefer newer to unstable versions.

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/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Comment on lines 21271 to 21274
linuxPackages_testing =
if lib.versionAtLeast linux_latest.version pkgs.linux_testing.version
then linuxPackages_latest
else linuxPackagesFor pkgs.linux_testing;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
linuxPackages_testing =
if lib.versionAtLeast linux_latest.version pkgs.linux_testing.version
then linuxPackages_latest
else linuxPackagesFor pkgs.linux_testing;
linuxPackages_testing =
if linux_latest.kernelOlder linux_testing.version
then linuxPackages_latest
else linuxPackagesFor linux_testing;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Atemu Nice suggestion, didn't know about that function! But I think it should be linux_latest.kernelAtLeast or the arguments need to be flipped.

nix-repl> if linux_latest.kernelAtLeast linux_testing.version then linux_latest.version else linux_testing.version
"5.13.3"

nix-repl> if linux_latest.kernelOlder linux_testing.version then linux_latest.version else linux_testing.version
"5.13-rc6"

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, didn't actually test.

@ikervagyok
Copy link
Contributor Author

@Atemu is this the solution you wanted?

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Yeah, please squash those commits though.

@ikervagyok
Copy link
Contributor Author

Sorry, now it's squashed!

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Um, what about linux_testing though?

It'd probably be better to implement the switch there instead.

@ikervagyok
Copy link
Contributor Author

ikervagyok commented Jul 21, 2021

I thought about it, but I'd like it to still be accessible, but not the default. If that is a wrong assumption, I'll switch it around.

EDIT: I could revert and add a new linux_latest_testing and a linuxPackages_latest_testing. Maybe that would be the better solution. Of course I'm open to better naming ideas.

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 130872 at 1ed3a6ef run on aarch64-linux 1

10 packages failed to build:
1 package skipped due to time constraints:
  • linuxPackages_testing.openafs (linuxPackages_testing.openafs_1_8)
43 packages built successfully:
  • linuxPackages_testing.acpi_call
  • linuxPackages_testing.akvcam
  • linuxPackages_testing.apfs
  • linuxPackages_testing.batman_adv
  • linuxPackages_testing.bcc
  • linuxPackages_testing.can-isotp
  • linuxPackages_testing.cpupower
  • linuxPackages_testing.cryptodev
  • linuxPackages_testing.ddcci-driver
  • linuxPackages_testing.digimend
  • linuxPackages_testing.dpdk-kmods
  • linuxPackages_testing.ena
  • linuxPackages_testing.fwts-efi-runtime
  • linuxPackages_testing.gcadapter-oc-kmod
  • linuxPackages_testing.hid-nintendo
  • linuxPackages_testing.hyperv-daemons
  • linuxPackages_testing.jool
  • linuxPackages_testing.kernel
  • linuxPackages_testing.lttng-modules
  • linuxPackages_testing.mba6x_bl
  • linuxPackages_testing.mbp2018-bridge-drv
  • linuxPackages_testing.netatop
  • linuxPackages_testing.oci-seccomp-bpf-hook
  • linuxPackages_testing.openafs_1_9
  • linuxPackages_testing.openrazer
  • linuxPackages_testing.perf
  • linuxPackages_testing.r8125
  • linuxPackages_testing.r8168
  • linuxPackages_testing.rtl8812au
  • linuxPackages_testing.rtl8821cu
  • linuxPackages_testing.rtl88x2bu
  • linuxPackages_testing.rtw88 (linuxPackages_testing.rtlwifi_new)
  • linuxPackages_testing.rtw89
  • linuxPackages_testing.stdenv
  • linuxPackages_testing.systemtap
  • linuxPackages_testing.tmon
  • linuxPackages_testing.usbip
  • linuxPackages_testing.v4l2loopback
  • linuxPackages_testing.veikk-linux-driver
  • linuxPackages_testing.vhba
  • linuxPackages_testing.xmm7360-pci
  • linuxPackages_testing.xpadneo
  • linuxPackages_testing.zfs (linuxPackages_testing.zfsStable ,linuxPackages_testing.zfsUnstable)

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

No, just put it that on _latest aswell. I can't imagine a true use-case where you'd want an RC instead of the released version.

@ikervagyok
Copy link
Contributor Author

No, just put it that on _latest aswell. I can't imagine a true use-case where you'd want an RC instead of the released version.

@Atemu Maybe I'm too tired, but I don't understand, what I should do. Could you rephrase it?

@Atemu
Copy link
Member

Atemu commented Jul 22, 2021

Put the logic in linux_testing instead of linuxPackages.

@ikervagyok ikervagyok force-pushed the kernel-testing branch 2 times, most recently from c1a0ceb to 77044b0 Compare July 26, 2021 11:09
@ikervagyok
Copy link
Contributor Author

@Atemu wdyt now?
The case split for the version is in linux_testing.
The one in linuxPackages_testing is just to keep the recurseIntoAttrs in the same state as before.

Comment on lines 20986 to 20995
let linux_testing =
callPackage ../os-specific/linux/kernel/linux-testing.nix {
kernelPatches = [
kernelPatches.bridge_stp_helper
kernelPatches.request_key_helper
];
};
in if linux_latest.kernelAtLeast linux_testing.version
then linux_latest
else linux_testing;
Copy link
Member

Choose a reason for hiding this comment

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

Creating a new linux_testing in the let statement makes things ambigous. TBH, just do away with the let and inline it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it linux_testing_ for the let binding, so it's easier to read and not ambiguous.

Copy link
Member

Choose a reason for hiding this comment

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

To be frank, that's even worse.

Just inline it please.

Copy link
Member

Choose a reason for hiding this comment

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

Looking into it again, I just realised that what I said is stupid: You need to reference _testing in the version check and therefore can't inline it.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@xaverdh
Copy link
Contributor

xaverdh commented Oct 11, 2021

I think #98865 is a better approach. It turns out we don't update linux_testing regularly anyway, so ppl are probably better off choosing the version themselves.
If #98865 is merged, we should probably think about either removing the linux_testing attribute, or automatically updating it.
edit: well automatic updating won't work anyway, because some manual work is usually required around kernel configuration

Atemu added a commit to Atemu/nixpkgs that referenced this pull request Nov 13, 2021
@ikervagyok
Copy link
Contributor Author

closing in favor of #144979

@ikervagyok ikervagyok closed this Nov 14, 2021
alyssais pushed a commit that referenced this pull request Nov 22, 2021
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.

None yet

4 participants