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

zfs: rename zfsStable -> zfs_2_2; zfsUnstable -> zfs_unstable; remove enableUnstable option in favor of package #291951

Merged
merged 7 commits into from
Mar 1, 2024
Merged

zfs: rename zfsStable -> zfs_2_2; zfsUnstable -> zfs_unstable; remove enableUnstable option in favor of package #291951

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dcff4f8
zfs_2_2: Rename from zfsStable
amarshall Jan 27, 2024
1c84667
nixos/tests/zfs: Get test name from pkg under test
amarshall Jan 27, 2024
ce5b1e0
nixos/zfs: Fix typo in option doc
amarshall Feb 27, 2024
929fcf9
zfs_unstable: Rename from zfsUnstable
amarshall Feb 27, 2024
2e36c49
nixos/pam: Do not incorrectly use zfs.enableUnstable in assertion
amarshall Feb 27, 2024
1f32eb7
nixos/zfs: Remove enableUnstable in favor of setting package
amarshall Feb 27, 2024
29a1b11
zfs_*: Avoid failing pkgs/by-name migration check
amarshall Mar 1, 2024
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
4 changes: 2 additions & 2 deletions nixos/modules/security/pam.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1458,9 +1458,9 @@ in
'';
}
{
assertion = config.security.pam.zfs.enable -> (config.boot.zfs.enabled || config.boot.zfs.enableUnstable);
assertion = config.security.pam.zfs.enable -> config.boot.zfs.enabled;
message = ''
`security.pam.zfs.enable` requires enabling ZFS (`boot.zfs.enabled` or `boot.zfs.enableUnstable`).
`security.pam.zfs.enable` requires enabling ZFS (`boot.zfs.enabled`).
'';
}
{
Expand Down
20 changes: 4 additions & 16 deletions nixos/modules/tasks/filesystems/zfs.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ in

imports = [
(mkRemovedOptionModule [ "boot" "zfs" "enableLegacyCrypto" ] "The corresponding package was removed from nixpkgs.")
(mkRemovedOptionModule [ "boot" "zfs" "enableUnstable" ] "Instead set `boot.zfs.package = pkgs.zfs_unstable;`")
];

###### interface
Expand All @@ -219,9 +220,9 @@ in
boot.zfs = {
package = mkOption {
type = types.package;
default = if cfgZfs.enableUnstable then pkgs.zfsUnstable else pkgs.zfs;
defaultText = literalExpression "if zfsUnstable is enabled then pkgs.zfsUnstable else pkgs.zfs";
description = lib.mdDoc "Configured ZFS userland tools package, use `pkgs.zfsUnstable` if you want to track the latest staging ZFS branch.";
default = pkgs.zfs;
defaultText = literalExpression "pkgs.zfs";
description = lib.mdDoc "Configured ZFS userland tools package, use `pkgs.zfs_unstable` if you want to track the latest staging ZFS branch.";
};

modulePackage = mkOption {
Expand All @@ -239,19 +240,6 @@ in
description = lib.mdDoc "True if ZFS filesystem support is enabled";
};

enableUnstable = mkOption {
type = types.bool;
default = false;
description = lib.mdDoc ''
Use the unstable zfs package. This might be an option, if the latest
kernel is not yet supported by a published release of ZFS. Enabling
this option will install a development version of ZFS on Linux. The
version will have already passed an extensive test suite, but it is
more likely to hit an undiscovered bug compared to running a released
version of ZFS on Linux.
'';
};

allowHibernation = mkOption {
type = types.bool;
default = false;
Expand Down
18 changes: 9 additions & 9 deletions nixos/tests/zfs.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ with import ../lib/testing-python.nix { inherit system pkgs; };

let

makeZfsTest = name:
makeZfsTest =
{ kernelPackages
, enableSystemdStage1 ? false
, zfsPackage
, extraTest ? ""
}:
makeTest {
name = "zfs-" + name;
name = zfsPackage.kernelModuleAttribute;
meta = with pkgs.lib.maintainers; {
maintainers = [ elvishjerricco ];
};
Expand Down Expand Up @@ -192,23 +192,23 @@ let
in {

# maintainer: @raitobezarius
series_2_1 = makeZfsTest "2.1-series" {
series_2_1 = makeZfsTest {
zfsPackage = pkgs.zfs_2_1;
kernelPackages = pkgs.linuxPackages;
};

stable = makeZfsTest "stable" {
zfsPackage = pkgs.zfsStable;
series_2_2 = makeZfsTest {
zfsPackage = pkgs.zfs_2_2;
kernelPackages = pkgs.linuxPackages;
};

unstable = makeZfsTest "unstable" rec {
zfsPackage = pkgs.zfsUnstable;
unstable = makeZfsTest rec {
zfsPackage = pkgs.zfs_unstable;
kernelPackages = zfsPackage.latestCompatibleLinuxPackages;
};

unstableWithSystemdStage1 = makeZfsTest "unstable" rec {
zfsPackage = pkgs.zfsUnstable;
unstableWithSystemdStage1 = makeZfsTest rec {
zfsPackage = pkgs.zfs_unstable;
kernelPackages = zfsPackage.latestCompatibleLinuxPackages;
enableSystemdStage1 = true;
};
Expand Down
4 changes: 2 additions & 2 deletions pkgs/os-specific/linux/zfs/stable.nix → pkgs/os-specific/linux/zfs/2_2.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ in
callPackage ./generic.nix args {
# You have to ensure that in `pkgs/top-level/linux-kernels.nix`
# this attribute is the correct one for this package.
kernelModuleAttribute = "zfs";
kernelModuleAttribute = "zfs_2_2";
# check the release notes for compatible kernels
kernelCompatible = kernel.kernelOlder "6.8";

Expand All @@ -23,7 +23,7 @@ callPackage ./generic.nix args {

tests = [
nixosTests.zfs.installer
nixosTests.zfs.stable
nixosTests.zfs.series_2_2
];

hash = "sha256-Bzkow15OitUUQ+mTYhCXgTrQl+ao/B4feleHY/rSSjg=";
Expand Down
4 changes: 2 additions & 2 deletions pkgs/os-specific/linux/zfs/generic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ let

inherit maintainers;
mainProgram = "zfs";
# If your Linux kernel version is not yet supported by zfs, try zfsUnstable.
# On NixOS set the option boot.zfs.enableUnstable.
# If your Linux kernel version is not yet supported by zfs, try zfs_unstable.
# On NixOS set the option `boot.zfs.package = pkgs.zfs_unstable`.
broken = buildKernel && (kernelCompatible != null) && !kernelCompatible;
};
};
Expand Down
2 changes: 1 addition & 1 deletion pkgs/os-specific/linux/zfs/unstable.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ in
callPackage ./generic.nix args {
# You have to ensure that in `pkgs/top-level/linux-kernels.nix`
# this attribute is the correct one for this package.
kernelModuleAttribute = "zfsUnstable";
kernelModuleAttribute = "zfs_unstable";
# check the release notes for compatible kernels
kernelCompatible = kernel.kernelOlder "6.9";

Expand Down
2 changes: 2 additions & 0 deletions pkgs/top-level/aliases.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,8 @@ mapAliases ({
### Z ###

zabbix40 = throw "'zabbix40' has been removed as it has reached end of life"; # Added 2024-01-07
zfsStable = zfs; # Added 2024-02-26
zfsUnstable = zfs_unstable; # Added 2024-02-26
zinc = zincsearch; # Added 2023-05-28
zkg = throw "'zkg' has been replaced by 'zeek'";
zq = zed.overrideAttrs (old: { meta = old.meta // { mainProgram = "zq"; }; }); # Added 2023-02-06
Expand Down
6 changes: 3 additions & 3 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28729,13 +28729,13 @@ with pkgs;
zfs_2_1 = callPackage ../os-specific/linux/zfs/2_1.nix {
configFile = "user";
};
zfsStable = callPackage ../os-specific/linux/zfs/stable.nix {
zfs_2_2 = callPackage ../os-specific/linux/zfs/2_2.nix {
configFile = "user";
};
zfsUnstable = callPackage ../os-specific/linux/zfs/unstable.nix {
zfs_unstable = callPackage ../os-specific/linux/zfs/unstable.nix {
configFile = "user";
};
zfs = zfsStable;
zfs = zfs_2_2;
Copy link
Member

@infinisil infinisil Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the one annoying unanticipated edge case with RFC 140. I do like how it encourages refactorings though: In this case a pkgs.zfsVersions like described in #287918 (comment) would work great, I'd love to see that, it's my recommendation here.

However, merging this PR as is indeed won't break master (I have a PR to make this clearer), but we really shouldn't normalise merging with red checks (that's how we ended up with #289649).

Instead I'd like to improve the error messages and documentation to show how such refactorings can be done (and manually responding to pings as here until then).

Edit: Now opened #292214 to document this better

As a last resort, it's also possible to weaken the check for new packages to not trigger in such cases. This will be easy to do once the code for the automated migration is in place (working on it). I hope this won't be necessary with the improved error message/docs though.

Copy link
Member Author

@amarshall amarshall Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In this case a pkgs.zfsVersions

This is the opposite of what is described in [pkgs/README.md](https://github.com/NixOS/nixpkgs/blob/c6caed479a521fd96b847d287c3fbdc611ffdca3/pkgs/README.md#package-naming, that suggests multiple versions should be top-level (the example in the doc being json-c_0_9, never suggesting package sets. Package sets also have downsides: they are more awkward to overlay and adding aliases requires foregoing all the tooling builtin to aliases.nix and manually dealing with allowAliases.

we really shouldn't normalise merging with red checks

Agreed, and I appreciate the pause given here (and in another recent PR of mine that renamed—fortunately it was amenable to migration and so it was done (and in general I like pkgs/by-name).

One alternative to consider (nowhere near an opinion) is changing pkgs/by-name migration check to add a comment to the PR when merging is still “okay but discouraged”, rather than a failing check.

Copy link
Member

@infinisil infinisil Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Package sets also have downsides: they are more awkward to overlay and adding aliases requires foregoing all the tooling builtin to aliases.nix and manually dealing with allowAliases.

Oh yeah that's a good point. I just opened #292214 to have a better recommendation that can be applied more generally. Shouldn't be a problem with that.

One alternative that to consider (nowhere near an opinion) is changing pkgs/by-name migration check to add a comment to the PR when merging is still “okay but discouraged”, rather than a failing check.

That's a good idea, I'll keep it in mind and might use it if this becomes a bigger problem. For now I'm hoping the PR I just opened will give a good path forward, and once merged I intend to change the CI check to link to that documentation.


### DATA

Expand Down
6 changes: 3 additions & 3 deletions pkgs/top-level/linux-kernels.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,15 +565,15 @@ in {
configFile = "kernel";
inherit pkgs kernel;
};
zfsStable = callPackage ../os-specific/linux/zfs/stable.nix {
zfs_2_2 = callPackage ../os-specific/linux/zfs/2_2.nix {
configFile = "kernel";
inherit pkgs kernel;
};
zfsUnstable = callPackage ../os-specific/linux/zfs/unstable.nix {
zfs_unstable = callPackage ../os-specific/linux/zfs/unstable.nix {
configFile = "kernel";
inherit pkgs kernel;
};
zfs = zfsStable;
zfs = zfs_2_2;

can-isotp = callPackage ../os-specific/linux/can-isotp { };

Expand Down
Loading