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
Introduce concept of NixOS support states #23590
Conversation
Each NixOS branch will have a .version-state.nix file, which will have an attribute "state" that is either set to "supported", "deprecated" or "unsupported". If the state is not equal to "supported", a warning will be generated during the building of a system configuration. Optionally, it can include a "description" attribute that can be used to communicate extra information to the end user, such as how (s)he should upgrade to the newer release.
8936000
to
23e0339
Compare
internal = true; | ||
type = types.nullOr (types.enum ["supported" "deprecated" "unsupported"]); | ||
default = nixosState.state; | ||
description = "Support state of this version of NixOS. Both depreciated and unsupported are considered to be unsupported."; |
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 think deprecated
is supported, but perhaps you mean "both deprecated and unsupported will raise a warning."
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.
Yes indeed. Will update.
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.
depreciated → deprecated.
Unless this is because the older releases diminish in value over a period of time?
Mark the old stable as <literal>deprecated</literal> or <literal>unsupported</literal> by updating | ||
the <literal>state</literal> field in the <literal>.version-state.nix</literal> file to | ||
the desired state. It's recommended to also add a <literal>description</literal> field in that file | ||
to give the user instructions on how to actually upgrade. |
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.
When marking a release as deprecated, it should likely also include a note in the description about when it will go from deprecated to unsupported.
I think this seems pretty good. @globin can you review? |
@@ -112,6 +154,8 @@ in | |||
HOME_URL="https://nixos.org/" | |||
SUPPORT_URL="https://nixos.org/nixos/support.html" | |||
BUG_REPORT_URL="https://github.com/NixOS/nixpkgs/issues" | |||
IS_SUPPORTED=${toString config.system.nixosState.isSupported} | |||
SUPPORT_STATE="${toString config.system.nixosState.state}" |
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.
This isn't part of os-release
it seems.
They recommend prefixing new fields:
Operating system vendors may extend the file format and introduce new fields. It is highly recommended to prefix new fields with an OS specific name in order to avoid name clashes. Applications reading this file must ignore unknown fields. Example: "DEBIAN_BTS="debbugs://bugs.debian.org/""
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.
Also, toString true
-> "1". You probably want to use lib.boolToString true
-> "true" instead.
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 think these changes are superfluous until some tool starts needing them.
nixosState = import "${toString pkgs.path}/.version-state.nix"; | ||
revisionFile = "${toString pkgs.path}/.git-revision"; | ||
gitRepo = "${toString pkgs.path}/.git"; | ||
gitCommitId = lib.substring 0 7 (commitIdFromGitRepo gitRepo); |
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.
Nitpicking, but I think the whitespace changes here before =
aren't necessary.
description = | ||
if cfg.nixosState.description == null | ||
then "" | ||
else "\n${cfg.nixosState.description}"; |
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.
description = lib.optionalString (cfg.nixosState.description != null) "\n${cfg.nixosState.description}";
@@ -86,6 +116,18 @@ in | |||
|
|||
config = { | |||
|
|||
warnings = | |||
if !cfg.nixosState.isSupported |
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.
warnings = lib.optional (!cfg.nixosState.isSupported) ...
readOnly = true; | ||
internal = true; | ||
type = types.nullOr types.str; | ||
default = if (nixosState ? description) |
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.
Unnecessary parens
# See nixos/doc/manual/development/releases.xml, subsection "After the final release time" | ||
{ | ||
state = "supported"; | ||
} |
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.
Oh and this file should probably be in the nixos folder instead
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.
Should it be in the nixos
folder, if it also affects the Darwin channels and stable channels used outside NixOS? (Even though this PR doesn't add anything for those right now)
I agree with the above reviewers that this should probably be made into " |
Actually, #22096 (comment) in #22096 suggests the same. Reading that discussion I see nothing that prevents putting such a check near nix version check on top of |
I think the idea of storing the support level of a branch in a file named Thus, since the set of supported branches is dynamic, this information should be fetched at runtime, e.g. in
which tools like As an aside, |
If somebody is never running |
I imagine that nixos-rebuild might check the commit-date of the revision and warn if it's more than 30 days old (for example). That seems a fairly good approximation without needing to encode the state anywhere, covering your forgetting to use |
Side note: the mechanism used to propagate the “supported” / “not-supported” state requires ability to figure out dynamic information about a There is some discussion about this last use case in NixOS/rfcs#34. |
Closing because as indicated by @edolstra (#23590 (comment)) this is not the right approach |
Each NixOS branch will have a .version-state.nix file, which will have an attribute "state" that is either set to "supported", "deprecated" or "unsupported".
If the state is not equal to "supported", a warning will be generated during the building of a system configuration.
Optionally, it can include a "description" attribute that can be used to communicate extra information to the end user, such as how (s)he should upgrade to the newer release.
Motivation for this change
See #22096
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)The warnings / state part as discussed in #22096