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
zfs: use meta.broken instead of throw when incompatible with kernel #109029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I do wonder why meta.broken
was not used originally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff LGTM.
38ac730
to
e1d6849
Compare
`throw` aborts eval when the package is touched in inappropriate places. See NixOS#109001 for an adverse instance of that. ZFS now behaves like a regular broken package when it's, you know, broken. It also still prints the helpful incompatibility notice when fully evaluated.
e1d6849
to
aa58df5
Compare
Whoops, accidentally tried to merge staging into master; sorry for the ping. Merge conflict fixed. |
safe for backporting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm late
@@ -174,6 +168,13 @@ let | |||
license = licenses.cddl; | |||
platforms = platforms.linux; | |||
maintainers = with maintainers; [ hmenke jcumming jonringer wizeman fpletz globin mic92 ]; | |||
broken = if | |||
buildKernel && (incompatibleKernelVersion != null) && versionAtLeast kernel.version incompatibleKernelVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildKernel && (incompatibleKernelVersion != null) && versionAtLeast kernel.version incompatibleKernelVersion | |
buildKernel && (incompatibleKernelVersion != null) && kernel.kernelAtLeast incompatibleKernelVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to just push that.
Could you also do the backport to stable (staging?)? You'd probably be the one to merge my PR for that anyways ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although hold on, have we foreward-ported kernelAtLeast yet? You created that in staging IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should be there, I needed it to properly mark failing module builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[11:42:55] jon@jon-desktop /home/jon/projects/nixpkgs (release-20.09)
$ git pull
Already up to date.
[11:42:59] jon@jon-desktop /home/jon/projects/nixpkgs (release-20.09)
$ nix eval -f . linuxPackages.kernel.kernelAtLeast
<LAMBDA>
[11:43:17] jon@jon-desktop /home/jon/projects/nixpkgs (release-20.09)
$ git co master
Switched to branch 'master'
Your branch is behind 'origin/master' by 5 commits, and can be fast-forwarded.
(use "git pull" to update your local branch)
[11:43:46] jon@jon-desktop /home/jon/projects/nixpkgs (master)
$ nix eval -f . linuxPackages.kernel.kernelAtLeast
<LAMBDA>
Motivation for this change
throw aborts eval when the package is touched in inappropriate places, see
#109001 for an adverse instance of that.
It now behaves like a regular broken package when it's, you know, broken.
It also still prints the helpful incompatibility notice.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)