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

Introduce concept of NixOS support states #23590

Closed
wants to merge 1 commit into from

Conversation

moretea
Copy link
Contributor

@moretea moretea commented Mar 7, 2017

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
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

The warnings / state part as discussed in #22096

@mention-bot
Copy link

@moretea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @oxij, @obadz and @edolstra to be potential reviewers.

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.
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.";
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed. Will update.

Copy link
Member

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.
Copy link
Member

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.

@grahamc
Copy link
Member

grahamc commented Mar 8, 2017

I think this seems pretty good. @globin can you review?

@grahamc grahamc added this to the 17.09 milestone Jul 26, 2017
@samueldr samueldr modified the milestones: 17.09, 18.09 Aug 2, 2018
@@ -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}"
Copy link
Member

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/""

Copy link
Member

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.

Copy link
Member

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);
Copy link
Member

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.

@samueldr
Copy link
Member

samueldr commented Aug 2, 2018

Adopted into the 18.09 milestone since it was a leftover into the 17.09 milestone. I also like the concept.

I don't know who else than who @grahamc pinged, @globin, could review. Though, the changes don't seem too intrusive, and only consists of a warning.

description =
if cfg.nixosState.description == null
then ""
else "\n${cfg.nixosState.description}";
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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";
}
Copy link
Member

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

Copy link
Member

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)

@oxij
Copy link
Member

oxij commented Aug 6, 2018

I agree with the above reviewers that this should probably be made into "nixpkgs support states" instead, i.e. not NixOS-specific. I'd add a trace somewhere around the top-level ./default.nix.

@oxij
Copy link
Member

oxij commented Aug 6, 2018

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 nixpkgs evaluation like @edolstra suggested.

@edolstra
Copy link
Member

edolstra commented Aug 6, 2018

I think the idea of storing the support level of a branch in a file named .version-state.nix inside the repository is wrong, because it associates a specific support level with each Git revision rather than with the branch. Thus, for example, if you build NixOS from a pinned revision that has state = "supported", it will forever continue to show as "supported" even when the associated branch has long since become unsupported. Similarly, if you don't run nixos-rebuild --upgrade, the displayed support state of your system won't be updated.

Thus, since the set of supported branches is dynamic, this information should be fetched at runtime, e.g. in nixos-rebuild or from some systemd timer. We could have a file https://nixos.org/channels/channel-info.json that contains data like

{
  "nixos-17.09": { "supported": false, ... }
  "nixos-18.03": { "supported": true, ... }
}

which tools like nixos-rebuild can check against to see if the current branch is still supported.

As an aside, nixosState is a confusing name since it makes me think of the mutable state of the system (as in system.stateVersion).

@timokau
Copy link
Member

timokau commented Aug 26, 2018

If somebody is never running nix-channel --upgrade, it doesn't matter either way if the channel they are using is supported or not.

@vcunat
Copy link
Member

vcunat commented Sep 1, 2018

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 --upgrade. Note that commit-date of a cherry-pick commit is the date it was cherry-picked.

@samueldr samueldr modified the milestones: 18.09, 19.03 Oct 6, 2018
@Ekleog
Copy link
Member

Ekleog commented Dec 2, 2018

Side note: the mechanism used to propagate the “supported” / “not-supported” state requires ability to figure out dynamic information about a nixpkgs checkout. As such, I think this mechanism would be interesting also to be able to note “this checkout's version of apache is vulnerable to CVE-xxxx-xxxx”, etc.

There is some discussion about this last use case in NixOS/rfcs#34.

@infinisil
Copy link
Member

Closing because as indicated by @edolstra (#23590 (comment)) this is not the right approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants