Only uninstall packages managed by this module

[Tomaszal]

First of all thanks for creating this module! Convergent mode feels like the right the approach of combining Flatpak and NixOS to me, so I'm happy this exists.

Before this PR, there was an issue of packages being overridden on activation of the module, meaning if packages were installed outside of this module, they would get uninstalled on the next activation, and only the packages declared in this module would be left. This felt like a design flaw to me so I decided to fix it, I hope you agree and that this was not an intentional design choice.

This PR introduces a state file that keeps track of the module's modifications, so that they can be managed separately from imperatively introduced modifications. This is largely inspired by the way home manager handles dconf settings. However, this module needs to work for both system and user installations, so it cannot rely on the home manager activation flow. To work around that, I put a symlink to the state file in the gcroots folder of the respective installation, which prevents it from being garbage collected. I couldn't find any conventions on how this should be handled, so it could probably be improved in the future. The only issue I can currently see with this approach is that the state file derivation would not be being garbage collected even if this module is removed. This would be easy to solve with home manager, but then it would need a separate implementation depending on the installation. Anyway, this shouldn't be too big of an issue, as it's just a text file that doesn't link to anything else that won't be garbage collected.

The state file is a JSON object so that more data can be added later. This is relevant as I would like to work on implementing overrides in this module in a similar fashion if this goes through. That way the state of the overrides could be stored in the same file.

[Tomaszal]

Just realized I also had to change the entry point for the home manager activation script to be after reloadSystemd (rather than after writeBoundary), as that's when the service gets registered with the updated state file paths. I also changed the name of the entry point to be more descriptive (flatpak-managed-install instead of start-service) while I was at it. Technically this was an issue even before this PR, so I can split it into a separate commit if you'd like. It probably went under radar as it works just fine if you don't add any DAG dependencies to the flatpak-managed-install entry point (executing correctly after systemd reload). However, when I added a script that depended on Flatpak (home.activation.flatpakPatches = lib.hm.dag.entryAfter ["flatpak-managed-install"] "..."), the entry point moved above systemd reload and the service broke, as it wasn't being updated before running.

[gmodena]

First of all thanks for creating this module! Convergent mode feels like the right the approach of combining Flatpak and NixOS to me, so I'm happy this exists.

Thanks for this patch @Tomaszal ! I need to review and do some integration tests, but I love what you did here.

Before this PR, there was an issue of packages being overridden on activation of the module, meaning if packages were installed outside of this module, they would get uninstalled on the next activation, and only the packages declared in this module would be left. This felt like a design flaw to me so I decided to fix it, I hope you agree and that this was not an intentional design choice.

FWIW: this was a (poorly documented) conscious design choice. The assumption would be that flatpaks would be installed and managed only via configuration files. So, at any point in time we can understand the state of a system by inspecting said config.

However, I do agree that his maximalist approach is not pragmatic. IMHO we should add a module level configuration strict: bool option that when set to true
defaults to current behavior (uninstall all the things at activation), and when false behaves like expressed by your change. By default, we should set strict=false.

What do you think @Tomaszal ? Is it something you'd like doing in this PR? Totally fine if not, I'm good to merge as-is and add the strict: bool option myself afterwards.

[gmodena]

Technically this was an issue even before this PR, so I can split it into a separate commit if you'd like. It probably went under radar as it works just fine if you don't add any DAG dependencies to the flatpak-managed-install entry point

Great catch! Fine by me to keep everything in this commit. I'll file a bug and link to the fix in this MR in the next release.

This would be easy to solve with home manager, but then it would need a separate implementation depending on the installation. Anyway, this shouldn't be too big of an issue, as it's just a text file that doesn't link to anything else that won't be garbage collected.

+1 to not worry about this issue for now. I'd rather keep a list of known issues, than juggling multiple implementations that express divergent behaviour.

[gmodena]

LGTM. Tested on linux and darwin.

Left a couple of nits in review and on the issue thread.

[gmodena]

Could you maybe add a comment here wrt the risk of stateFile not getting GCed?

Not a concern for now IMHO (see comments in thread), but I'd like to be reminded of this..

[Tomaszal]

Yeah makes sense, will add it

[Tomaszal]

Thanks for the review @gmodena!

FWIW: this was a (poorly documented) conscious design choice. The assumption would be that flatpaks would be installed and managed only via configuration files. So, at any point in time we can understand the state of a system by inspecting said config.

However, I do agree that his maximalist approach is not pragmatic. IMHO we should add a module level configuration strict: bool option that when set to true defaults to current behavior (uninstall all the things at activation), and when false behaves like expressed by your change. By default, we should set strict=false.

What do you think @Tomaszal ? Is it something you'd like doing in this PR? Totally fine if not, I'm good to merge as-is and add the strict: bool option myself afterwards.

I could definitely add that as a part of this PR, though there's probably a better name for it than strict, as to me that sounds like it would make the whole config deterministic (i.e. what declarative-flatpak does). Perhaps uninstallUnmanagedPackages is a clearer name?

[gmodena]

I could definitely add that as a part of this PR, though there's probably a better name for it than strict, as to me that sounds like it would make the whole config deterministic (i.e. what declarative-flatpak does). Perhaps uninstallUnmanagedPackages is a clearer name?

Ack. I can see how the semantic of strict could become confusing. uninstallUnmanagedPackages is a bit verbose, but clear. Can't think of another alternative, so the naming works for me,

[gmodena]

I could definitely add that as a part of this PR,

Hey @Tomaszal . I wanted to touch base and ask if you still have the bandwidth/interest to work on this aspect. No rush, but I'd love to have the work you did in this MR merged and released.

uninstallUnmanagedPackages could be added post merge in case, happy to pick that up.

[Tomaszal]

Hi @gmodena, sorry for disappearing, I've been a bit busy with life the past couple of weeks. I'm more free now though so I'll try to add the changes very soon. I'll let you know if it's problematic, but I think it (uninstallUnmanagedPackages) should be pretty easy to add as part of this PR

[Tomaszal]

@gmodena added the uninstallUnmanagedPackages feature and slightly modified the code so that #24 can be a little cleaner. The way uninstallUnmanagedPackages works now is basically if it's enabled all Flatpak managed packages are added to the old state, taking precedence over the actual old state. That logic is fully contained within the new handleUnmanagedPackagesCmd function and the rest is unchanged

[gmodena]

LGTM.

I think the way you handle old/new state is neat. Running some tests now, but if it all works as expected we can merge.