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

update.onActivation Is very confusing #125

Open
thomasfinstad opened this issue Dec 20, 2024 · 10 comments
Open

update.onActivation Is very confusing #125

thomasfinstad opened this issue Dec 20, 2024 · 10 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@thomasfinstad
Copy link

First, thanks for the work you have done, I really like using flatpaks for desktop apps and you made that very easy.

I am having a hard time understanding the real intention of onActivation.

Whether to enable flatpak to upgrade applications during
{command}nixos system activation. The default is false
so that repeated invocations of {command}nixos-rebuild switch are idempotent.

implementation: appends --or-update to each flatpak install command.

I don't understand how the implementation achieves the stated goal.

How I understand it from the description this option, when set to false, the install / update will not be triggered by nixos-rebuild, but that is not what the implementation does at all. It also seems to be completely irrelevant if auto update is enabled.

updateApplications = cfg.update.onActivation || cfg.update.auto.enable;

I think the option should either change name to match the effective implementation, or change the implementation to match the name/description.

But if the option works and is described as intended and its just me being dumb then I think it would be very nice to have an option that can stop the service from restarting during a nixos-rebuild or perhaps change the service to be a forking type that would be helpful since running a rebuild can be excessively slow currently, as the service blocks until it has completed its work.

@gmodena
Copy link
Owner

gmodena commented Dec 21, 2024

Hey @thomasfinstad ,

thanks for reaching out.
Agree. onActivation, as currently implemented and documented, is a mess.

How I understand it from the description this option, when set to false, the install / update will not be triggered by nixos-rebuild, but that is not what the implementation does at all.

This should be the intended behavior, though in practice the current implementation tries (wrongly) to install packages anyway.

This is related to #110, which I plan to pick up next.

It also seems to be completely irrelevant if auto update is enabled.

That's currently expected behavior, though it may be poorly documented. At the time, it seemed like a good idea to have auto.update trigger an install upon activation, but that might be something worth reconsidering before hitting 1.0.0 (that should become a stable API).

What's your take on this?

@gmodena gmodena self-assigned this Dec 21, 2024
@gmodena gmodena added bug Something isn't working documentation Improvements or additions to documentation labels Dec 21, 2024
@thomasfinstad
Copy link
Author

thomasfinstad commented Dec 21, 2024

Thanks for getting back to me so quickly on this.

My thoughts on this would be fairly "categorized" since I like to group stuff.

update.auto.onCalendar updates the pkgs on a schedule, with it being disabled when null?

onActivation updates pkgs during a nixos-rebuild, independent of the onCalendar, perhaps renamed to "onRebuild"?However I think it would be less confusing if it was moved to be update.auto.onActivation, as that would indicate the enable/disable is relevant here too, and it is an automatic function so makes sense grouping wise.

Edit: onActivation should not be enabled if set to false in my opinion. And if you use null as a deactivation switch on onCalendar, then the enable/disable option will be redundant and can get cleaned up.

@thomasfinstad
Copy link
Author

another thought is maybe it would be worth while to flip the update.auto around to auto.update

This would allow a better grouping of all automatic tasks. This would be relevant to #96 which could then be auto.remove.unused right next other auto tasks.

auto = {
  update = {
    onActivation = false;
    onCalendar = "leapyearly";
  };
  remove = {
    unused = {
      onActivation = false;
      onCalendar = "leapyearly";
    };
    unmanaged = {
      onActivation = false;
      onCalendar = "leapyearly";
    };
  };
};

Probably have some spelling errors etc, but you get the point.

@gmodena
Copy link
Owner

gmodena commented Jan 3, 2025

onActivation should not be enabled if set to false in my opinion. And if you use null as a deactivation switch on onCalendar, then the enable/disable option will be redundant and can get cleaned up.

For context:
making onActivation conditional to auto updates is a shortcut to simplify systemd unit config. It avoids having to disambiguate when the install script is trigger by unit startup vs the timer.

another thought is maybe it would be worth while to flip the update.auto around to auto.update
This would allow a better grouping of all automatic tasks. This would be relevant to #96 which could then be auto.remove.unused right next other auto tasks.

I like your proposal to restructure attrs under auto.update.
When uninstallUnused was introduced, I should have refactored the unused and unmanaged options, but I avoided doing so to maintain API conventions. I'll explore refactoring further in the near future (likely after #110 and #82).

As for onActivation, I'm not yet sure if I want to change its behavior, but that decision is independent of auto.update. I do see your point though.

Let's keep this issue open to discuss and track API enhancements.

@thomasfinstad
Copy link
Author

thomasfinstad commented Jan 5, 2025

For onActivation the only really important thing, IMHO, is to be able to have it not try to update all flatpaks when doing a nixos-rebuild, but still have it auto update on a schedule in the background.

I think one solution to the API change being kind of "scary" in general is to simply keep a v0 branch, and a v1, v2, v3 etc etc. Then when you no longer wish to maintain a version branch just add a last commit that emits a warning that the branch is EOL and that the user should change their flake input version, but dont intentionally break the system by emitting an error. In my opinion that makes you a responsible dev, and a responsible user will update when notified and fix any incompatible settings. Doing this might allow you to iterate faster because you can try things out, and get a bit more feedback from other perspectives and still keep it safe for those that just pin the version branch.

@gmodena
Copy link
Owner

gmodena commented Jan 12, 2025

onActivation should not be enabled if set to false in my opinion. And if you use null as a deactivation switch on onCalendar, then the enable/disable option will be redundant and can get cleaned up.

For context: making onActivation conditional to auto updates is a shortcut to simplify systemd unit config. It avoids having to disambiguate when the install script is trigger by unit startup vs the timer.

Okay, I’ve come up with a solution that I’m relatively happy with. So far, I’ve only tested it in virtual machines, but it’s available in the refactor-updates branch (PR #136). I need to daily drive it before merging. Holler if you have a chance to try it.

Please note that this change doesn’t introduce any API modifications for now. It simply ensures that the schedule update configuration does not trigger an update upon activation. Side effects discussed in #110 still apply for now (a fix for that will follow shortly after #136).

@thomasfinstad
Copy link
Author

@gmodena I have given the branch a quick try and with my current config I don't see much difference. It seems stable, and still does a install / update of all flatpaks during nixos-rebuild.

Just for reference this is my cfg:

    enable = true;

    update = {
      onActivation = false;
      auto = {
        enable = true;
        onCalendar = "weekly";
      };
    };

    uninstallUnmanaged = false;

@gmodena
Copy link
Owner

gmodena commented Jan 26, 2025

@gmodena I have given the branch a quick try and with my current config I don't see much difference. It seems stable, and still does a install / update of all flatpaks during nixos-rebuild.

Just for reference this is my cfg:

enable = true;

update = {
  onActivation = false;
  auto = {
    enable = true;
    onCalendar = "weekly";
  };
};

uninstallUnmanaged = false;

Ack. Updates should not have been triggered on activation; could you double check what commit hash your flake.lock is pointing to? Short of caching arterfacts, I wonder if you caught a weird regression.

FWIW that branch - and additional work on this code path (#141) - is now in main.

@thomasfinstad
Copy link
Author

@gmodena sadly I am moving away from nixos at this time and cant really test right now, I could probably do it at a later time, but it will be a while until then. So if you can not replicate it I think you can consider it a quirk of my system / lock file. I wanna thank you for making my nix time a fair bit nicer with this flatpak feature. Keep up the good work :D

@gmodena
Copy link
Owner

gmodena commented Feb 1, 2025

@thomasfinstad no worries! thanks for the heads up & productive conversations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants