zfs: rename zfsStable -> zfs_2_2; zfsUnstable -> zfs_unstable; remove enableUnstable option in favor of package #291951
Conversation
The `zfs` alias already has equivalent semantics. Instead, make this like zfs_2_1 so folks who want to pin a specific release series can do so easily and clearly to have more control over when more substantial updates occur. Rename all tests to match the pkg attr they are testing.
The previous names are already this.
This matches the naming of other zfs_* pkgs.
`zfs.enableUnstable` only has an effect if `zfs.enabled = true`, so only require `zfs.enabled` to be true here.
This just adds complexity and confusion. Once-upon-a-time, there was no `package` and only `enableUnstable`, but now it is just confusing to have both, as it would be possible to do e.g. `package = pkgs.zfs` and `enableUnstable = true`, but then `enableUnstable` does nothing.
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
8.has: clean-up
8.has: package (new)
delroth
added
the
12.approvals: 1
|
cc @infinisil on renames which appears as "new top-level" package, how should we proceed about that? |
delroth
added
12.approvals: 2
|
It triggered by-name again! Usually it forces the by-name migration. |
|
I suspect we're going to need to migrate zfs by hand, so maybe we can just ignore the check and merge? Happy to wait and see what infinisil says. |
pkgs/top-level/all-packages.nix
Outdated
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Package sets also have downsides: they are more awkward to overlay and adding aliases requires foregoing all the tooling builtin to
aliases.nixand manually dealing withallowAliases.
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.
|
I’ve followed the suggestion from @infinisil’s PR here, and have pushed as an additional commit on top. PR is now passing the pkgs/by-name check. |
delroth
removed
the
12.approvals: 2
|
@ofborg build zfs_2_2 zfs_2_2.passthru.tests zfs_unstable zfs_unstable.passthru.tests |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-23.11
git worktree add -d .worktree/backport-291951-to-release-23.11 origin/release-23.11
cd .worktree/backport-291951-to-release-23.11
git switch --create backport-291951-to-release-23.11
git cherry-pick -x dcff4f831836e249db5e808aeac6ca71d498cdc7 1c846675398059067b6ad147abc6a7877c4b8368 ce5b1e007e1eb96e5df296e33ed884b70dc19250 929fcf93358a833003435c0f74b9bd993f9546d0 2e36c49949f90f14a2ffcc002c8f411b725022d2 1f32eb724ddc9e27573266756c390a6bdcca4439 29a1b11f916e5994f7ad23039ae21945df99247c |
|
Do we really want to backport this? |
|
Unclear to me, but divergence between master and 23.11 will cause issues for further bumps IMHO. |
Description of changes
Please see individual commit messages for more details (also suggest reviewing per-commit, as the commits are reasonably atomic). Putting as one PR since most of the changes either build on each other semantically or would conflict, but can split up if some parts warrant more discussion.
Overall the goal here is to improve consistency in the naming and handling of different ZFS versions.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.