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

nixos/xdg/xdg-open: init #187293

Closed
wants to merge 2 commits into from

Conversation

LunNova
Copy link
Member

@LunNova LunNova commented Aug 18, 2022

Description of changes

Potential fix for #160923 although is not currently enabled by default.

MERGED Until #181171 gets merged, this relies on enabling an overlay for all those packages to actually use the xdg-open implementation from the path.

Many other packages rely on it implicitly without adding it in a wrapper so will work already without the overlay.

Some other things like the steam FHS env also require the overlay currently. Could get them to pick it up from the PATH in a subsequent PR?

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/making-xdg-open-more-resilient/16777/8

# due to https://github.com/NixOS/nixpkgs/issues/160923
default = "xdg-open-with-portal";
};
# TODO:
Copy link
Member Author

Choose a reason for hiding this comment

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

  • FIXME: needs meta added

--timeout 5 \
"" "$targetFile" {}
fi
''
Copy link
Member Author

Choose a reason for hiding this comment

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

  • FIXME: needs meta added

{ config, pkgs, lib, ... }:
let
cfg = config.xdg.xdg-open;
implementations = {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: make this support custom impl packages, this approach isn't great

@FliegendeWurst
Copy link
Member

This solves #160923 for Trilium Notes (wrapped electron application) 👍

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

I don't think duplicating upstream code is a good idea.

We should rather patch the regular xdg-open to handle our FHSEnvs the same as other container runtimes (i.e. flatpak).
We should probably even propose that upstream. Perhaps not in specific to NixOS but for all containerised apps.

Alternatively, we could also just pretend our FHSEnv'd apps are flatpaks.

@LunNova
Copy link
Member Author

LunNova commented Sep 7, 2022

I don't think duplicating upstream code is a good idea.

It isn't really duplicating code? It's a small shell script that always uses the portal, and it's not just for FHS envs, it's also for running under simple wrapper scripts or any other context.

It doesn't use any xdg-open code and makes the portal responsible for things 100% of the time.

I could make a small repo for it under github.com/LunNova and package it from github instead of including it inline.

@Atemu
Copy link
Member

Atemu commented Sep 7, 2022

I don't see how we're not duplicating upstream code here? We've now got a shell script that is supposed to do the same as upstream xdg-open and even uses the same mechanism internally but we need to maintain it separately.

The location of the code is not the problem, the fact that we've got a separate implementation of xdg-open at all is.

I think we should instead have a minimal patch that makes xdg-open detect our fhsenvs and treat them the same as flatpak. That's very simply to do. Perhaps even propose that patch upstream so we don't have to maintain it downstream.

Building a new implementation of xdg-open and then globally(!) switching it out to fix a bug in fhsenvs is not sustainable in my eyes.

@LunNova
Copy link
Member Author

LunNova commented Sep 7, 2022

In general being able to replace the version of xdg-open on your system with something not from xdg-utils is useful. There's also handlr and xdg_open_ng, it's not unusual to want to swap your xdg-open implementation. Having a shell script called 'xdg-open' on path is the API that programs expect, and xdg-utils is not the only source of an implementation of it in the wild.

In my mind "xdg-open-with-portal" is a package that provides the xdg-open API and its implementation always uses dbus to ask the xdg portal to open an app. It's very simple, and doesn't contain most of the functionality of xdg-utils xdg-open, because that's now the portal's job. Since it assumes a portal exists it can be very minimal. It isn't derived from xdg-utils or a copy/paste job from upstream, it's a simple shell script I wrote for this use case. I think it fits into the same category as handlr or xdg_open_ng.

Separately, it'd be good to get xdg-utils to support some env var to tell it to always use the portal to open things, and to add support for opening files through the portal, but I don't think that should prevent adding a nixos module which controls which implementation of xdg-open you have on your PATH, or that it should prevent having a simple shell script impl of xdg-open that uses the portal.

@LunNova
Copy link
Member Author

LunNova commented Sep 7, 2022

Raised https://github.com/freedesktop/xdg-utils/pull/12 upstream to add an env var to tell xdg-open to always use the portal.

@LunNova
Copy link
Member Author

LunNova commented Oct 21, 2022

@Atemu how do you feel about including https://github.com/freedesktop/xdg-utils/pull/12 in nixpkgs in a separate PR before it gets merged?

xdg-utils seem very slow to respond to PRs.

@Atemu
Copy link
Member

Atemu commented Oct 21, 2022

I'd make it a patch behind a flag.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@bb010g
Copy link
Contributor

bb010g commented Apr 9, 2024

Raised freedesktop/xdg-utils/pull/12 upstream to add an env var to tell xdg-open to always use the portal.

This repository no longer exists, and I don't see a matching MR on https://gitlab.freedesktop.org/xdg/xdg-utils.

@LunNova
Copy link
Member Author

LunNova commented Apr 9, 2024

@bb010g it was this commit: LunNova/xdg-utils@123e8ce

@LunNova LunNova closed this Apr 9, 2024
@LunNova
Copy link
Member Author

LunNova commented Apr 9, 2024

Closed in favor of #197118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants