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

xdg-portal: update env vars, match nixos module #5158

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

Misterio77
Copy link
Contributor

@Misterio77 Misterio77 commented Mar 19, 2024

Description

Nixpkgs has recently(ish) made a few major changes to its xdg-desktop-portal package (NixOS/nixpkgs#268197), which silently breaks our module here:

  • The NIXOS_XDG_DESKTOP_PORTAL_CONFIG_DIR variable patch has been removed (in favor of putting portal configurations in /etc or XDG_CONFIG_HOME).
  • A new variable, NIX_XDG_DESKTOP_PORTAL_DIR, was introduced in a patch to avoid setting XDG_DESKTOP_PORTAL_DIR (which also affected portal configuration reading, not only portal definitions)

I updated our module to match the changes, but this breakage also made me revisit this module and look into some improvements.

Long story short, I think it's worth it to make it more similar to the NixOS one, as it will make behavior more predictable and consistent.

The main change is relying on the upstream linked systemd unit (instead of using systemd.user.services), and setting the environment variables globally instead of scoping it to the unit, as it's a very global thing anyway.

This might not be backwards compatible for people already facing #4922/#4972. Maybe I should also provide the unit to ~/.config?

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

Nixpkgs has recently made a few major changes to its
xdg-desktop-portal package, which silently breaks our module here:

- The NIXOS_XDG_DESKTOP_PORTAL_CONFIG_DIR variable patch has been
  removed (in favor of putting portal configurations in /etc or
  XDG_CONFIG_HOME).

- A new variable, NIX_XDG_DESKTOP_PORTAL_DIR, was introduced in a
  patch to avoid setting XDG_DESKTOP_PORTAL_DIR (which also affected
  portal configuration reading, not only portal definitions)

I updated our module to match the changes, but this breakage also made
me revisit this module and look into some improvements.

Long story short, I think it's worth it to make it more similar to the
NixOS one, as it will make behavior more predictable and consistent.
The main change is relying on the upstream linked systemd
unit (instead of using systemd.user.services), and setting the
environment variables globally instead of scoping it to the unit, as
it's a very global thing anyway.
@rycee rycee force-pushed the xdg-portal-update branch from 2aec824 to 1c2acec Compare March 20, 2024 22:43
@rycee rycee merged commit 1c2acec into nix-community:master Mar 20, 2024
3 checks passed
@rycee
Copy link
Member

rycee commented Mar 20, 2024

Thanks! Merged to master now 🙂

@andresilva
Copy link
Contributor

andresilva commented Mar 21, 2024

This broke portals for me.

❯ ls $NIX_XDG_DESKTOP_PORTAL_DIR
"/etc/profiles/per-user/andre/share/xdg-desktop-portal/portals": No such file or directory (os error 2)

Is this expected? The test passes though and it is expecting the correct output, so not really sure what's happening with my setup, my config is very much the same as the one in the test except I don't have any config (just a single entry configPackages).

Edit: for reasons that I am failing to understand /share/xdg-desktop-portal from the portals packages isn't getting linked into home.path, even though all the other files from the package are.

Edit2: environment.pathsToLink = [ "/share/xdg-desktop-portal" "/share/applications" ]; needs to be set. The tests pass because they're testing against home-manager-path directly.

@Misterio77
Copy link
Contributor Author

Hey @andresilva,

I can reproduce your issue when using the NixOS module and useUserPackages set to true, as in this case only specific stuff will get linked (as opposed to the entire home-manager-path being linked). For this, users will have to set environment.pathsToLink, as you noted.

I will open a follow up PR adding this info to the manual, similarly to programs.bash.enableCompletion.

@Misterio77
Copy link
Contributor Author

Alternatively, I wonder if the module could perhaps provide a sane pathsToLink default? Maybe conditionally?

Also, #2548 possibly fixes this, too.

Misterio77 added a commit to Misterio77/home-manager that referenced this pull request Mar 26, 2024
@Misterio77
Copy link
Contributor Author

Note added in #5184

@bobvanderlinden
Copy link

I ran into issues with this solution. Somehow /etc/profiles/per-user/bob.vanderlinden/share/xdg-desktop-portal/ didn't exist. Adding extraPortals to home.packages also did not work to get share/xdg-desktop-portal.

A solution that did work was to explicitly set NIX_XDG_DESKTOP_PORTAL_DIR to a symlinkJoin path of the extraPortals packages:

          home.sessionVariables.NIX_XDG_DESKTOP_PORTAL_DIR = lib.mkForce
            (let
              root = pkgs.symlinkJoin {
                name = "root";
                paths = config.xdg.portal.extraPortals;
              };
            in
              "${root}/share/xdg-desktop-portal/portals"
            );
          systemd.user.sessionVariables.NIX_XDG_DESKTOP_PORTAL_DIR = lib.mkForce config.home.sessionVariables.NIX_XDG_DESKTOP_PORTAL_DIR;

Would this be an viable solution to adopt in home-manager?

@Misterio77
Copy link
Contributor Author

Misterio77 commented Apr 8, 2024

Hey @bobvanderlinden,

I went with the profile approach as it is more consistent with the NixOS module, and relying on these nixpkgs-specific variables is a little risky, in my opinion (as this PR was written to fix breakage caused by NIXOS_XDG_DESKTOP_PORTAL_CONFIG_DIR being removed).

The issue you're facing, I believe, only happens if you're running NixOS and have useUserPackages = true, which links a lot less stuff. In that case, you should set pathsToLink like so:

environment.pathsToLink = [ "/share/xdg-desktop-portal" ];

See #5184

rycee pushed a commit to Misterio77/home-manager that referenced this pull request Apr 27, 2024
Specifically, add note about useUserPackages and pathsToLink. As
suggested in
<nix-community#5158 (comment)>.
rycee pushed a commit to Misterio77/home-manager that referenced this pull request Apr 27, 2024
Specifically, add note about useUserPackages and pathsToLink. As
suggested in
<nix-community#5158 (comment)>.
Noodlez1232 pushed a commit to Noodlez1232/home-manager that referenced this pull request Nov 21, 2024
Specifically, add note about useUserPackages and pathsToLink. As
suggested in
<nix-community#5158 (comment)>.
colonelpanic8 pushed a commit to colonelpanic8/home-manager that referenced this pull request Dec 30, 2024
Specifically, add note about useUserPackages and pathsToLink. As
suggested in
<nix-community#5158 (comment)>.
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.

4 participants