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

zfs: use meta.broken instead of throw when incompatible with kernel #109029

Merged
merged 1 commit into from Jan 21, 2021

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Jan 11, 2021

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
  • 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.

@Atemu Atemu added the 9.needs: port to stable A PR needs a backport to the stable release. label Jan 11, 2021
Copy link
Member

@NeQuissimus NeQuissimus left a 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

Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

@Atemu Atemu force-pushed the zfs-meta.broken-instead-of-abort branch from 38ac730 to e1d6849 Compare January 21, 2021 13:56
@Atemu Atemu changed the base branch from master to staging January 21, 2021 13:57
`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.
@Atemu Atemu force-pushed the zfs-meta.broken-instead-of-abort branch from e1d6849 to aa58df5 Compare January 21, 2021 13:58
@Atemu Atemu changed the base branch from staging to master January 21, 2021 13:58
@Atemu
Copy link
Member Author

Atemu commented Jan 21, 2021

Whoops, accidentally tried to merge staging into master; sorry for the ping.

Merge conflict fixed.

@marvin-mk2 marvin-mk2 bot requested a review from kevincox January 21, 2021 15:17
@Mic92 Mic92 merged commit 22ac0b9 into NixOS:master Jan 21, 2021
@Mic92
Copy link
Member

Mic92 commented Jan 21, 2021

safe for backporting.

Copy link
Contributor

@jonringer jonringer left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
buildKernel && (incompatibleKernelVersion != null) && versionAtLeast kernel.version incompatibleKernelVersion
buildKernel && (incompatibleKernelVersion != null) && kernel.kernelAtLeast incompatibleKernelVersion

Copy link
Member Author

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 ;)

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Contributor

@jonringer jonringer Jan 21, 2021

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>

@Atemu Atemu deleted the zfs-meta.broken-instead-of-abort branch January 21, 2021 18:31
@blitz blitz mentioned this pull request Jan 28, 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

7 participants