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

Add overrides management #24

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Add overrides management #24

merged 3 commits into from
Jan 31, 2024

Conversation

Tomaszal
Copy link
Contributor

This PR builds on top of PR #23, as it relies on a similar strategy to separately manage the declarative and imperative states. If/when that PR is merged I'll rebase this PR to only include the relevant commit.

This wasn't a trivial task and was mainly accomplished using jq, the logic of which has been separated into the modules/overrides.jq file. The installer simply provides the needed values to it by looping over previously and currently managed overrides, then reading their active (the actual Flatpak overrides files), old and new states (the files introduced in #23) from the respective files.

The actual logic to determine a value for an overrides entry is quite simple:

  • If the new value is a string, simply override the active/old value
  • If the value is an array, perform the following set arithmetic: active - old + new

@Tomaszal
Copy link
Contributor Author

Tomaszal commented Dec 13, 2023

Here's an example usage of this from my configs:

{
  services.flatpak.overrides = {
    global = {
      # Force Wayland by default
      Context.sockets = ["wayland" "!x11" "!fallback-x11"];

      Environment = {
        # Fix un-themed cursor in some Wayland apps
        XCURSOR_PATH = "/run/host/user-share/icons:/run/host/share/icons";

        # Force correct theme for some GTK apps
        GTK_THEME = "Adwaita:dark";
      };
    };

    "com.visualstudio.code".Context = {
      filesystems = [
        "xdg-config/git:ro" # Expose user Git config
        "/run/current-system/sw/bin:ro" # Expose NixOS managed software
      ];
      sockets = [
        "gpg-agent" # Expose GPG agent
        "pcsc" # Expose smart cards (i.e. YubiKey)
      ];
    };

    "org.onlyoffice.desktopeditors".Context.sockets = ["x11"]; # No Wayland support
  };
}

@gmodena gmodena self-requested a review December 16, 2023 09:07
@gmodena
Copy link
Owner

gmodena commented Dec 16, 2023

Thanks for another terrific piece of work @Tomaszal !

This PR builds on top of PR #23, as it relies on a similar strategy to separately manage the declarative and imperative states. If/when that PR is merged I'll rebase this PR to only include the relevant commit.

Ack.

@nikp123
Copy link

nikp123 commented Dec 21, 2023

Technical question: Is the overrides directory supposed to be created on its own?
Because I manually wiped /var/lib/flatpak before starting the module (to test how it'd handle a clean system).
The result was that flatpak-managed-install.service fails to start without the directory present.
Creating it fixes the problem.

@Tomaszal
Copy link
Contributor Author

Technical question: Is the overrides directory supposed to be created on its own? Because I manually wiped /var/lib/flatpak before starting the module (to test how it'd handle a clean system). The result was that flatpak-managed-install.service fails to start without the directory present. Creating it fixes the problem.

Good catch! Yes, it should create the directory if it doesn't exist, I'll fix that.

@Tomaszal
Copy link
Contributor Author

@gmodena in the spirit of #23 (comment), should another option be exposed that will do a similar thing with overrides? F.e. an option called removeUnmanagedOverrides. It would be simple to implement, as it could just delete the overrides folder and re-create it on activation, making sure only the overrides defined in the config are present. I personally don't really see why this would be needed, but it would be consistent with uninstallUnmanagedPackages.

@gmodena
Copy link
Owner

gmodena commented Dec 23, 2023

Hey @Tomaszal

I personally don't really see why this would be needed, but it would be consistent with uninstallUnmanagedPackages.

I like the idea but I share the quoted sentiment. I don't see an obvious use case for removeUnmanagedOverrides. I'd rather we leave the option out for now, and add it might the need arise. It's easier to add APIs than to deprecate them IMHO.

@Tomaszal
Copy link
Contributor Author

I've rebased this onto the latest changes in #23 which now include some small changes that were previously here. I also added an mkdir command that should fix the issue mentioned in #24 (comment)

Copy link
Owner

@gmodena gmodena left a comment

Choose a reason for hiding this comment

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

This branch has conflicts that must be resolved

@Tomaszal I merged #23 and now there is a conflict with this PR and main.

I resolved the merge conflict locally and was able to test your latest update. Everything seems to be working as expected.

Happy to merge once the merge this PR once the merge conflict is resolved in #24.

I refrained from pushing my local changes to avoid disrupting your PR and eventual ongoing work. Holler if you want me to go ahead with resolving and merging.

Thanks again for your contributions, and for taking the time to work through #23 and #24! Much appreciated.

I plan to cut a release in the upcoming days (after a little more battle testing).

@Tomaszal
Copy link
Contributor Author

@gmodena I've resolved the merge conflict with main, you should be able to merge now.

I realise now that there's probably a bug similar to the one you fixed in #28 with this, where on new installations OLD_STATE is empty. However, we can't fix it with the same simple check here, as if it's empty we still need to generate the overrides file with the overrides from the new state. I think the best fix for this (and for uninstalls too) is to initialize the OLD_STATE to a correct structure instead of simply {}. I can submit that fix, but I think it's better as a separate PR, since it affects both this and the uninstalls.

@gmodena
Copy link
Owner

gmodena commented Jan 31, 2024

Hey @Tomaszal

I think the best fix for this (and for uninstalls too) is to initialize the OLD_STATE to a correct structure instead of simply {}. I can submit that fix, but I think it's better as a separate PR, since it affects both this and the uninstalls.

Agree. +1 to do this in a dedicated PR.

@gmodena gmodena merged commit 30f6cb6 into gmodena:main Jan 31, 2024
1 check passed
@Tomaszal Tomaszal deleted the feature/overrides branch February 8, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants