NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.38k stars 14.33k forks source link

system.autoUpgrade relies on deprecated functionality for flakes #349734

Open pwaller opened 1 month ago

pwaller commented 1 month ago

Describe the bug

nixos-upgrade-start[23857]: warning: '--update-input' is a deprecated alias for 'flake update' and will be removed in a future version.

Nix is in the process of removing the --update-input flag from nix build. It is currently marked as deprecated. When it is removed, auto updates configured according to the community wiki with:

system.autoUpgrade = {
  enable = true;
  flake = inputs.self.outPath;
  flags = [
    "--update-input"
    "nixpkgs"
    "-L" # print build logs
  ];
  dates = "02:00";
  randomizedDelaySec = "45min";
};

will fail, because the flags are passed to nix build, which will presumably reject these once they are removed.

Breaking automatic updates in this way is unfortunate, because if someone has a long term unattended system and automatic updates start to fail, this is a condition which won't recover without intervention. Future automatic updates won't be received, which could be a deeper problem if a user is relying on this to get new configurations to a fleet of machines.

What I think needs fixing:

Add a :+1: reaction to issues you find important.

iFreilicht commented 1 month ago

Agreed, this is a usability issue now and will become a major regression later on.

Interestingly, --update-input in this case isn't even directly parsed by nix, but by nixos-rebuild. It does just forward this to nix right now, but as it parses the flag itself, we have multiple options for fixing this:

  1. Run nix flake update <input> for every --update-input <input> flag nixos-rebuild receives
  2. Run nix flake update (to update all inputs) by default when --flake is passed
  3. Add a new flag --update-all-inputs that will run nix flake update first

I personally would vote for implementing options 1 and 2. I believe the default that most people would expect when adding a configuration like this:

system.autoUpgrade = {
  enable = true;
  flake = inputs.self.outPath;
};

is that it "just works", in other words the entire flake gets updated. Still allowing --update-input will make this change backwards-compatible and allow for more granularity in case people have inputs in their flake they really don't want to upgrade automatically.

In any case, the current lockFlags (stuff like --commit-lock-file) will have to be forwarded to nix flake update.

The main challenge I see is that we still need to support nix 2.18 (because the breaking changes prevent some users from upgrading), so we need to detect whether the user's nix version supports nix flake update and change the behavior only when it does.

miallo commented 1 month ago

Only marginally (but a bit) related: the man-page for nixos-rebuild still refers to the option --update-input: https://github.com/NixOS/nixpkgs/blob/deb5e0e4ebb067986f6da49aeb6265e395379b3b/pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.8#L31 https://github.com/NixOS/nixpkgs/blob/deb5e0e4ebb067986f6da49aeb6265e395379b3b/pkgs/os-specific/linux/nixos-rebuild/nixos-rebuild.8#L463

Zocker1999NET commented 1 month ago
  1. Run nix flake update (to update all inputs) by default when --flake is passed

@iFreilicht, I kind of agree and disagree with you on option 2. IMO the example config you gave can trigger this behavior, as I also think this is what most users expect. But e.g. my current workflow expects that machines do not update any flake inputs automatically at all. I emphasize this point because you suggested that users can add the --update-input options to select which inputs they want to update, without mentioning if it would be possible to avoid all inputs being updated.

Also (assuming I understood correctly for option 2 to be implemented into nixos-rebuild), I think option 2 might clash with the existing expectations of users using nixos-rebuild standalone (i.e. not as part of the autoUpgrade module), where a flake & its lock file now fully specifies certain input versions, which I think should only be updated when a user requests it manually, without needing users to specify a suppressing option each time.

Hence I would suggest to implement option 1 & 3 in nixos-rebuild and then change the systemUpgrade module to append the new --update-all-inputs option when a new module option (e.g. .updateFlakeInputs) is enabled. This new module option can then be enabled by default but can be suppressed by users/sysadmins when needed.

My current update workflow as additional context For context, my current update workflow (with `autoUpgrade`) is: - most machines (mostly VMs) are have `system.autoUpgrade` enabled - pointing to my Git remote repo (main branch) - updating their configuration each day in the night - allowing me to postpone configuration changes & nixpkgs updates to the night - I push changes the remote main branch when I want them to deploy on my machines eventually - I update (i.e. reference a newer nixpkgs version) regularly after - running a set of customized NixOS tests - testing specialties of my configurations (i.e. custom modules, which are not yet suitable to be upstreamed to nixpkgs) - building the configurations of most machines - updating critical machines manually (so I can check for failures right away)
iFreilicht commented 1 month ago

Those are good arguments, and I fully agree. I didn't consider that just upgrading all inputs automatically could be a breaking change. Your workflow seems reasonable and I'm sure you're not the only one who uses autoUpgrade like this.

Hence I would suggest to implement option 1 & 3 in nixos-rebuild and then change the systemUpgrade module to append the new --update-all-inputs option when a new module option (e.g. .updateFlakeInputs) is enabled.

Okay I like adding .updateFlakeInputs, how about it can be either "none", which is the default and the same behavior as now, an array of the inputs to update, or "all", which then triggers the --update-all-inputs flag?

This seems like it would tick all the boxes and be easy to use and discover.

Zocker1999NET commented 1 month ago

Okay I like adding .updateFlakeInputs, how about it can be either "none", which is the default and the same behavior as now, an array of the inputs to update, or "all", which then triggers the --update-all-inputs flag?

this solution sounds good to me :+1:

eldritch-cookie commented 1 month ago

hello just commenting here to mention that i also am affected by this issue

iFreilicht commented 1 month ago

@eldritch-cookie please don't do this, it's pinging everyone who follows the issue. Just react to the issue with šŸ‘, this helps maintainers prioritize.