gmodena / nix-flatpak

Install flatpaks declaratively
Apache License 2.0
293 stars 10 forks source link

Implement state management for remotes declared by nix-flatpak #32

Closed ReedClanton closed 1 month ago

ReedClanton commented 8 months ago

Description

This repo's README.md states that Flathub is the default remote. It also explains how to add a remote. It does not explain that adding a remote will remove the default one.

This is an issue because:

  1. Adding a remote prevents the instillation of flatpaks that are found in the default remote but not the added one (with no errors or warning printed to the console).
  2. The common standard in Nix (so far as I've seen) is for assigning values to list attributes to append them (not overwrite). Thus services.flatpak.remotes being a list implies that any preexisting values will remain when new ones are added.

Solution(s)

  1. Add a note to the docs that explains the non-standard behavior. (easiest)
  2. Convert services.flatpak.remotes from a list to a set. (this just seems like an all around bad option, but I'm including it anyways)
  3. Always append new values to remotes list (my preferred method). This could be implemented in a few ways:

    • Keep default remote when new ones are added. This means it won't be possible to remove the default(s).
    • Remove the defaults. This would get around having to have a remote that a user may not use, but would require more up-front configuration.
gmodena commented 8 months ago

Hey @ReedClanton ,

Thanks for the detailed issue report!

It also explains how to add a remote. It does not explain that adding a remote will remove the default one.

This is currently expected behavior. I appreciate remote management is a bit confusing (and not well explained).

Thinking out loud:

Assuming this is related to #31, where you started with a fresh NixOs install, I think you possibly hit a corner case where you ran module activation with a non-default remote declared at generation 0 (e.g. before running activation with only the default remote set). In this case, flathub would have never been set.

Always append new values to remotes list (my preferred method).

Thinking out loud again.

remotes should implement the same kind state tracking as packages. At each activation nix-flatpak should diff previous/current remote declarations and update flatpak config accordingly.

Remove the defaults. This would get around having to have a remote that a user may not use, but would require more up-front configuration.

I think that having flathub as a fallback (= use it only if services.flatpak.remotes is empty) would still be my preferred choice, but at the very least it needs more explicit documentation.

As an alternative ways of managing default remote:

Both feel a bit clunky.

ReedClanton commented 8 months ago

Assuming this is related to #31, where you started with a fresh NixOs install, I think you possibly hit a corner case where you ran module activation with a non-default remote declared at generation 0 (e.g. before running activation with only the default remote set). In this case, flathub would have never been set.

That is what I did.

As an alternative ways of managing default remote:

* users could be given the option to select whether to keep it across activations (similarly to what is done for packages with `uninstallUnmanagedPackages`)

* I could document that `default` should be explicitly concatenated with the `services.flatpak.remotes` declaration.

I don't really understand either of these options entirely. I don't know what uninstallUnmanagedPackages does or how to use it. Is it a undocumented feature of nix-flatpak? As for the second option, it sounds like the most elegant way of doing it, but I still don't fully understand how it would work.

And to be clear, when I say I don't entirely understand either option, I don't mean that I disagree with them. Rather I'm just new to Nix, so some stuff goes over my head.

Both feel a bit clunky.

Agreed. I can't think of an option that doesn't feel at least a little clunky. Maybe add a boolean option that when set to true will retain the default remote? From what I understand of the second option you suggested it also seems relatively elegant.

ReedClanton commented 8 months ago

I was thinking about this while climbing, and I see three options that I like (although you may chose what every you want, obviously):

Option 1

Description

This would be what you suggested (quoted bellow).

  • I could document that default should be explicitly concatenated with the services.flatpak.remotes declaration.

Example

services.flatpak.remotes = [
  {
    name = "flathub-beta";
    location = "https://flathub.org/beta-repo/flathub-beta.flatpakrepo";
  }
] ++ config.services.flatpak.remotes;

Note: The above doesn't work, but I image you're suggestion would look something like it. Or maybe just explicitly include flathub in the list.

Option 2

Description

My currently preferred method would be to not have a default at all, as is done in the NixOS flatpak module I previously used. You've (@gmodena) expressed an opposition to this option, and I understand why.

Example

services.flatpak.remotes = [
  {
    name = "flathub-beta";
    location = "https://flathub.org/beta-repo/flathub-beta.flatpakrepo";
  }
  {
    name = "flathub";
    location = "https://dl.flathub.org/repo/flathub.flatpakrepo";
  }
];

Option 3

Description

Add an option to keep default remotes.

Example

services.flatpak = {
  # I would prefer this to default to true, but you've expressed a preference for the inverse,
  # and I think that's fine as well.
  keepDefaultRemotes = true;
  remotes = [
    {
      name = "flathub-beta";
      location = "https://flathub.org/beta-repo/flathub-beta.flatpakrepo";
    }
  ];
};

Note

Regardless of what change ends up being made, including no change, I strongly believe the docs should be updated to explicitly call out the behavior.

gmodena commented 8 months ago

I was thinking about this while climbing,

Excellent sport activity :+1:

Option 1 [...]

There is an idiomatic lib.mkOptionDefault function to augment an options' default value. It would work like this:

services.flatpak.remotes = lib.mkOptionDefault [{
   name = "flathub-beta";
   location = "https://flathub.org/beta-repo/flathub-beta.flatpakrepo";
}];

Option 2

What makes this your preferred option? Honest question, I really appreciate feedback about UX. I keep going back and forth on this. On the one hand I like having a default (reduces boilerplate), on the other hand I think explicit is better than implicit.

FWIW I'm currently leaning towards the status quo (with improved documentation).

Regardless of what change ends up being made, including no change, I strongly believe the docs should be updated to explicitly call out the behavior.

Added some notes wrt how remotes config works, and how to manage the default value.

ReedClanton commented 8 months ago

I'm going to do some research tomorrow (and maybe the next day) so I can really make up my mind what I believe the best option is, however, I want to bring something up before I forget.

I've mentioned before that the documentation should be updated at the least, but I also think that if a flatpak provided to services.flatpak.packages isn't available in any of the provided remotes, then the user should be made aware. My intuition says it should cause the build to fail, however, a warning would suffice. The fact that the user isn't informed is what tripped me up to the point that I thought this module was just broken (because nothing was getting installed).

ReedClanton commented 7 months ago

Approach

Before I started to answer this question, I wanted to determine what the most important factor(s) would be. Ultimately, I landed on consistency. In other words, the single most important factor when it comes to UX is consistency.

It's not hard to image how application close buttons being in different locations in each application would make life difficult. I figured the same thing would apply here. It would be better to place all application close buttons in the same place, rather than move it into a spot that might work better for one application.

Research

I started by looking through Home Manager options. No option that accepted only a list had any default. The only exceptions I found were multi-type options (ex. programs.neomutt.binds.*.map programs.rofi.theme).

Next I looked into how list type options are normally handled. Take environment.systemPackages as an example. In my configuration I set this value in many places. Each time, the values I set are added to the list. This is done without the need to explicitly call out that the values are being appended (i.e. no need to use some sort of += syntax).

Analysis

List type options, when provided by NixOS or Home Manager, universally have no default set. When a user sets a list type option, values are appended.

Applying Personal Experience

When I was in the process of switching over from declarative-flatpak to this module, I did the same thing I always do when adding something to my NixOS/Home Manager configuration. I looked at the options available, added each one I wanted to set, then ran my system build. My assumption that the default value would be appended to the values I added was the single largest sticking point.

Counter Points

Response to Counter Points

Conclusion

I believe this issue represents three distinct problems (listed in order of importance [according to me]):

  1. User isn't informed when an element from services.flatpak.packages can't be found.
  2. services.flatpak.remotes exhibits non-standard behavior.
  3. Documentation of the above is lacking.

I believe I did a poor job of writing this issue. It should be broken out into two issues. One that covers informing the use when flatpaks aren't installed, and another for addressing non-standard behavior/documentation.

Next Steps

A decision should be made regarding the path forward. @gmodena must make this diction. Regardless of what I think of it, I will accept it and help when/where I can.

If @gmodena agrees, either @gmodena or @ReedClanton should write a separate issue for checking if flatpaks can be found in remotes and warning the user when they can't be. Note that this functionality will be internet dependent. So no errors/warnings should be produced when not connected to the internet.

ReedClanton commented 7 months ago

I just realized that the solution I purposed above will need to account for what remote will be used when not provided. Personally, I always provide both appId and origin, but for those who don't, we could add a default option to remotes, as shown bellow, or no longer allow user's to not provide origin (though that would break people's config, and I have a feeling you wouldn't like the extra configuration required):

services.flatpak.remotes = [
  {
    default = true;
    name = "flathub";
    location = "someUrl";
  }
  {
    name = "flathub-beta";
    location = "someUrl";
  }
];

alternate implementation:

services.flatpak.remoteDefault = "flathub";

While I'd preffer the second implementation, if the first one is chosen, and multiple remotes are provided that have default set to true, we could:

Note: Also, choosing the second implementation would address the non-standard behavior of having a default remote because now services.flatpak.remoteDefault could just have a default value of flathub.

gmodena commented 7 months ago

Thanks for your analysis @ReedClanton . Super thoughtful and well put together. I will reply with appropriate depth as soon as I have some mental bandwidth to spare.

Lehmanator commented 4 months ago
  1. Convert services.flatpak.remotes from a list to a set. (this just seems like an all around bad option, but I'm including it anyways)

Why would this be a bad idea? It could provide a lot of flexibility.

User config defaults could look like:

services.flatpak.remotes = {

  # Default: flathub only
  flatpak = {
    enable = true;
    default = true; # Disable when other remotes set=true
    name = "flatpak";
    location = "https://flathub.org/repo/flathub.flatpakrepo" 
  };

  # Default: user-supplied remotes
  <name> = {
    enable = true;
    default = false; # Error if multiple true
    name = "<name>"; # Default to attrName, specify to override
    location = "<URL>";
  };

  # Default: other pre-defined remotes?
  #   Optionally simply user config, so URLs not needed
  <name> = {     
    enable = false;
    default = false; 
    name = "<name>";
    location = "<URL>";
  };

};

Benefits:

So a minimum config to enable with added remote would be:

services.flatpak = {
  enable = true;
  remotes.flathub-beta.location = "https://flathub.org/beta-repo/flathub-beta.flatpakrepo";

  # Or if default pre-defines some extra remote attrs
  remotes.flathub-beta.enable = true;
};
gmodena commented 4 months ago

Hey @Lehmanator ,

thanks for your input.

[...]

Benefits:

* Add remotes without unexpected behavior

* Add remotes without re-adding flathub

* Add remotes without automatically enabling them

* Add remotes with minimal user config

So a minimum config to enable with added remote would be:


services.flatpak = {
  enable = true;
  remotes.flathub-beta.location = "https://flathub.org/beta-repo/flathub-beta.flatpakrepo";

  # Or if default pre-defines some extra remote attrs
  remotes.flathub-beta.enable = true;
};
``
`
Within nix-flatpak, "default" basically means "the only one configured." Setting a default would not imply changes in the order of execution of `flatpak install`. I am afraid that adding a default keyword could be confusing.

I think we achieve the same benefit you listed by piggybacking on lib.mkOptionDefault (and its sibling functions) to handle semantics, without the additional overhead of having to implement bookkeeping.

In retrospect, I think it would have been better not to provide a default repo and let users manage their config explicitly. I might break this API in upcoming releases.

gmodena commented 1 month ago

I'm closing this issue since remote state management has been implemented. Might there be interested in API changes / follow ups, please open a thread in Discussions.