nixos/version: validate system.stateVersion #351158
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
wegank
added
the
12.approvals: 1
nixos/modules/misc/version.nix
Outdated
| parts = lib.versions.splitVersion version; | ||
| isVersion = lib.length parts == 2 && lib.all (p: lib.stringLength p == 2) parts; | ||
| majorVersion = lib.toIntBase10 (lib.elemAt parts 0); | ||
| minorVersion = lib.elemAt parts 1; | ||
|
|
||
| versionPatterns = [ | ||
| # only 13.10 | ||
| { fromMajor = 13; minor = [ "10" ]; } | ||
| # 14.04 and 14.12 | ||
| { fromMajor = 14; minor = [ "04" "12" ]; } | ||
| # only 15.09 | ||
| { fromMajor = 15; minor = [ "09" ]; } | ||
| # 16.03 to 20.09 | ||
| { fromMajor = 16; minor = [ "03" "09" ]; } | ||
| # from 21.05 | ||
| { fromMajor = 21; minor = [ "05" "11" ]; } | ||
| ]; | ||
|
|
||
| # find the versioning pattern that applies by looking for the first | ||
| # major version newer than `majorVersion`, and picking the previous pattern | ||
| patternIndex = lib.lists.findFirstIndex | ||
| ({ fromMajor, ... }: fromMajor > majorVersion) | ||
| (lib.length versionPatterns) | ||
| versionPatterns; | ||
|
|
||
| validMinorVersions = | ||
| if patternIndex == 0 | ||
| then [] | ||
| else (lib.elemAt versionPatterns (patternIndex - 1)).minor; | ||
|
|
||
| correctMinorVersion = lib.elem minorVersion validMinorVersions; | ||
| notNewerThanNixpkgs = lib.versionAtLeast trivial.release version; |
There was a problem hiding this comment.
Why do we have to play this game instead of just listing all previous versions?
There was a problem hiding this comment.
The intention was to avoid adding yet another place that needs to be changed when a new release is cut (just when the release schedule changes). It was neater before I added pre-16 releases with less regularity.
There was a problem hiding this comment.
system.stateVersion was added in d166c85 and its first valid value was actually, uh… 15.07(!). So you can omit the earlier releases but have to account for past schedule drift. Looking at git log -p .version, you also need 21.03, 17.10 (though this only existed for about 20 minutes, so you can probably omit it), 15.08, and 15.06. (There were actually also 15.05, 15.04, 14.11 (looks like 14.12 never existed on master?), 14.10, and 14.02, but as I said those predate stateVersion.)
There was a problem hiding this comment.
As previously mentioned, there is a stateVersion even in the NixOS infrastructure using a value from before the option was introduced, so I can't omit it.
As for the .version log, that's a good point. I've been mostly going off of https://channels.nixos.org/ for the authoritative list of "real" versions. But that might be unfair to unstable users, if their config was generated while .version didn't refer to a channel version.
There are some very early versions with most likely zero instances in the wild, going back to 0.5 in 2004, in that history, but I'll include all of the ones following the YY.MM format (starting at 13.07), if it means we don't have to discuss which of them are "real enough". It'll still benefit users going forward, even if it might be overly-permissive for some (long) past releases. Please leave a thumbs-up or reply if you think we should just include all of them (YY.MM).
There was a problem hiding this comment.
Sorry for missing the previous discussion. Just going based on the .version log as you suggested seems good to me.
There was a problem hiding this comment.
You can't be expected to read all the previous iterations of this PR :)
It's not just .version, as channels.nixos.org contains some versions that have never been in master/.version, so I included it if it appeared in either.
tilpner
force-pushed
the
state-version-validation
branch
from
020c6d0 to
0f2e1cf
Compare
nixos/modules/misc/version.nix
Outdated
| # Refer to both https://channels.nixos.org/ and `git log --follow -p .version`. | ||
| # Some of the early version have never been released as a channel, but are included | ||
| # anyway for the benefit of nixos-unstable users who used these intermediate versions. | ||
| # Versions prior to the introduction of the `stateVersion` option have been observed | ||
| # in usage, so they can't be omitted. | ||
| versionPatterns = [ | ||
| { fromMajor = 13; minor = [ "07" "09" "10" ]; } | ||
| { fromMajor = 14; minor = [ "02" "04" "10" "11" "12" ]; } | ||
| { fromMajor = 15; minor = [ "04" "05" "06" "07" "08" "09" ]; } | ||
| { fromMajor = 16; minor = [ "03" "09" ]; } | ||
| { fromMajor = 17; minor = [ "03" "09" "10" ]; } | ||
| { fromMajor = 18; minor = [ "03" "09" ]; } | ||
| { fromMajor = 21; minor = [ "03" "05" "11" ]; } | ||
| { fromMajor = 22; minor = [ "05" "11" ]; } | ||
| ]; |
There was a problem hiding this comment.
This turned from a very sparse representation that made sense to me, to basically listing all versions from 2013 to 2018 exhaustively, and I'm less content with the patch for it. We could omit two lines if we ignore 17.10 (the 21min mistake), but then I'd have to add a comment explaining why, and it doesn't seem worth diverging from the "include everything" stance.
If anyone has a significantly more compact idea that doesn't need to be updated twice a year, I'm open for improvements.
wegank
removed
the
12.approvals: 1
jopejoe1
added
the
2.status: merge conflict
|
Even a simpler patch that only uses a regex to validate instead of a set of valid versions would be a big help. I just got finished diagnosing an issue with a noob's config that was caused by |
|
@rhendric I don't think the complexity of the validation has prevented this PR from being merged. Rather, it hasn't been rebased since 24.11, and has received no review attention since. If there still is interest, I don't mind rebasing it. |
|
I'm interested in some version of this! Absent strong feelings either way, with two PRs available I'm inclined to review and merge the simpler one first, but I could be convinced that this stronger validation is worth the relatively minor maintenance cost whenever we change our release schedule or some unforeseen edge case arises. |
tilpner
force-pushed
the
state-version-validation
branch
from
0f2e1cf to
174c28d
Compare
ofborg
bot
removed
the
2.status: merge conflict
|
I agree that any sort of validation would be better than the current state. This version will additionally reject never-released versions like "23.12", and catch impossible versions like "24.22". Those are less problematic versions than "unstable", but could still happen from misunderstandings or typos. As a consequence of being a "generic NixOS version" option type, the description can't contain suggestions specific to |
tilpner
force-pushed
the
state-version-validation
branch
from
174c28d to
17d3052
Compare
|
I'm not really a fan of what nixfmt does to this (turns 8 lines of versionPatterns into 66 lines), but short of committing a crime (packing version data into strings), I don't know how to keep it short. I'll try to accept it as an "ugly edge case". |
Continuation of #81135 and #317858.
With #340501 merged, hopefully this won't break eval again.
Description of changes
The location of the release type is open for discussion, it could go into lib.types instead.
Test with
Correctly invalid (with
lib.trivial.release == "24.11"):Correctly valid:
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.