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

treewide: rewrite stateVersion docs (again), clean up some stateVersion usages (again) #263744

Merged
merged 8 commits into from
Oct 29, 2023

Conversation

K900
Copy link
Contributor

@K900 K900 commented Oct 27, 2023

Description of changes

This is as actionable (or really NON-actionable) as I can make it sound, I think.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 27, 2023
@K900 K900 requested review from domenkozar, maralorn and a team October 27, 2023 09:16
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Docs LGTM, also replacing the absolute paths with modulesPath. Not sure about the other details, so please wait for actual maintainers.

nixos/modules/misc/version.nix Outdated Show resolved Hide resolved
nixos/modules/misc/version.nix Outdated Show resolved Hide resolved
nixos/modules/misc/version.nix Outdated Show resolved Hide resolved
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 27, 2023
nixos/modules/installer/tools/tools.nix Outdated Show resolved Hide resolved
nixos/modules/misc/version.nix Show resolved Hide resolved
nixos/modules/misc/version.nix Outdated Show resolved Hide resolved
nixos/modules/installer/tools/tools.nix Show resolved Hide resolved
@K900
Copy link
Contributor Author

K900 commented Oct 27, 2023

r? @colemickens for Azure stuff, @mkg20001 for LXD stuff

@colemickens
Copy link
Member

To be honest, I haven't touched that stuff in years, and have less than no incentive to now. My last attempt was out-of-tree, which is probably what I'd recommend going forward. In fact, I'd recommend deprecating and planning to remove the Azure stuff from the tree, but I have no idea if folks are using any of it. Sorry I don't have a better answer.

Ideally we'd want to build _all_ the stateVersions, but this is probably still better?
We should really have the user set it, or at least have a warning.
Use modulesPath so we don't have to magically rewrite paths in activation script,
set stateVersion to the one this was built with (which should approximate "first install")
Just like we do for the other live images
Hopefully this way we don't confuse people into thinking you have to set it to 21.05.
@K900 K900 force-pushed the stateversion-docs branch 2 times, most recently from 3053b58 to 9338ea0 Compare October 27, 2023 09:58
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 27, 2023
conditional on the first version of NixOS you've installed (encoded in `stateVersion`), instead of
simply always using the latest one.

Note that this generally only affects applications that can't upgrade their data automatically -
Copy link
Member

Choose a reason for hiding this comment

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

Even if an application can upgrade automatically its data, often it can not downgrade the data, making rollbacks obsolete. I understood stateVersion also as a warning to make sure I create backups otherwise I would not be able to rollback.

Copy link
Contributor

Choose a reason for hiding this comment

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

The seems too tangential to include here, as it's not really about stateVersion, just another aspect of how nixos interacts with state. This description is already long enough that a fair chunk of people won't read it all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also not about backups really - automated upgrades happen in nixpkgs all the time without stateVersion bumps.

Copy link
Member

@mkg20001 mkg20001 left a comment

Choose a reason for hiding this comment

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

👍 for lxd

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 28, 2023
Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

I commented on the comment. Changes there then potentially need to be mirrored to the option docs.

Comment on lines 227 to 243
# This option defines the first version of NixOS you have installed on this particular machine,
# and is used to maintain compatibility with data not managed by NixOS configuration (e.g. databases)
# that was created by applications running on older NixOS versions.
#
# Most users should NEVER change this value after the initial install, for any reason,
# even if you've upgraded your system.
#
# This value does NOT affect the Nixpkgs version your packages and OS are pulled from,
# so changing it will NOT upgrade your system.
#
# This value being lower than the current NixOS release does NOT mean your system is
# out of date, out of support, or vulnerable.
#
# Do NOT change this value unless you have manually inspected all the changes it would make to your configuration,
# and migrated your data accordingly.
#
# For more information, see `man configuration.nix`.
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 this is good and clear. A few thoughts:

  • "This option defines the first version of NixOS you have installed on this particular machine" kinda blurs the difference between the current install and previous installs of nixos on the same machine. It’s hopefully not a confusion anyone runs into, but it seems like an unneccessary confusion. Maybe "This option is neccessary to remember the version of NixOS used during initial install on this particular machine."?

  • "with data not managed by NixOS configuration" this is confusing. The data is not generated from NixOS configuration, but file location, services, etc. are managed by NixOS configuration. We are looking for a beginner friendly term for "stateful data", right? How about simply "compatibility with application data (e.g. databases) that was created by older NixOS versions."

  • For my personal taste, this comment is too long. I would confine more of the warnings into the option documentation. That’s exactly why the comment previously said "a) just leave this, it’s fine and b) before changing read the docs". In the end I have no clue what works better for users. Just saying, that we maybe need a different solution before half of configuration.nix is an essay about stateVersion.^^

  • Why drop the the nixos.org link? I assume for many users it is the more natural interface to read options than the man-page which is arguably only semi-made for this.

I would love to see my first two points considered somehow, the second two are nice to have.

Copy link
Contributor Author

@K900 K900 Oct 28, 2023

Choose a reason for hiding this comment

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

  1. I get the problem, but I think "initial install" can be misread mostly the same way. I can't really think of a good way to phrase this to make it clear we mean "the first version of this install" without using "install" is a noun, which is confusing.

  2. Yes, that was the idea. I think I like your version, though "application data" is technically never created by NixOS, it's created by applications - but that's nitpicky and not relevant to the target audience for this comment.

  3. Originally it was intro, 3 points, see also. Now it's 4 points (with number 3 being added on suggestion from @tejing), and I think this is also my upper limit for how many points it should have. I'd like to condense it more, but I think more points that are punchier individually is better than less points that don't hit as hard.

  4. No particular reason, I think it just got lost in the sauce. I'll put it back.

Copy link
Member

Choose a reason for hiding this comment

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

If this were a book I would be very opposed to the one paragraph - one sentence policy. I think I would probably just pack the points 2 to 5 in one paragraph. That way we have three paragraphs "What is this, what to do, further reading". But that’s really just my intuition. You can leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually intentionally separated these into individual lines, because I want them to scan as individual points. For a book, you'd probably want to write the whole thing very differently. This is basically our equivalent to the "this is not a place of honor" messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied [2] and [4]. Still thinking of a good way to phrase [1].

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 29, 2023
@evils
Copy link
Member

evils commented Oct 29, 2023

how about something like this for the config comment?

# This option lets NixOS know which release established the stateful data [on the system]
# (such as versioned databases, files, and their paths), and lets future releases stay compatible [with it].
# It is strongly recommended to leave this unchanged, changing this may result in data loss.
# Read the documentation [link to description?] if you're considering changing this.

@K900
Copy link
Contributor Author

K900 commented Oct 29, 2023

Not scary enough, IMO.

@tejing1
Copy link
Contributor

tejing1 commented Oct 29, 2023

This option lets NixOS know which release established the stateful data on the system

This sentence is good. It's the most intuitive single-line answer to "what does this option do" I've seen yet.

I still think we should keep the punchy later lines, but the first paragragh could be changed to:

# This option lets NixOS know which release established the stateful data on the system
# so that release upgrades do not break compatibility with existing state.

@tejing1
Copy link
Contributor

tejing1 commented Oct 29, 2023

Did you mean to remove the nixos manual url in that last force-push?

Hopefully this version is clearer. Also tried to make it less technical.
Match the comment in nixos-generate-config and add some more details for the curious.
@K900
Copy link
Contributor Author

K900 commented Oct 29, 2023

No, fixed.

Copy link
Contributor

@tejing1 tejing1 left a comment

Choose a reason for hiding this comment

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

Yeah, I like where the wording is at now.

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Oct 29, 2023
@K900 K900 merged commit 2202414 into NixOS:master Oct 29, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants