Conversation
|
Initially attached as a patch vs. 1.22.0 (which is what Debian and Ubuntu are shipping), I'll rebase on main afterwards. |
6c2b21e to
77e837f
Compare
|
This also fixes original bug using my current reproduction steps, which is to enable a systemd user service [Unit]
Description=Dummy
After=gnome-session-initialized.target
[Service]
Slice=session.slice
ExecStart=/path/to/script/below
[Install]
WantedBy=default.targetwhere the script consists of #!/bin/bash
/usr/bin/busctl introspect --timeout=20 --auto-start=true --user org.freedesktop.portal.Desktop /org/freedesktop/portal/desktop
sleep 100000000000000, and then in the session open a terminal, then the about window and clicking the home page link. Both on main and with this patch applied, the script will fail to start xdg-desktop-portal, but clicking the links will work, and with the original fix reverted it will start xdg-desktop-portal too early meaning clicking links wont work. |
|
I've confirmed that on a MATE desktop on Debian testing, without this patch x-d-p won't start, and with this patch it does work. And, @jadahl already said:
so this doesn't regress #2028 for GNOME (and presumably KDE Plasma has similar behaviour). So I think it would be a good idea to apply this to 1.22.x. |
…9351) **Summary** - Fixes the portal services not starting if the DE never reaches graphical-session.target, such as Budgie and Xfce. See flatpak/xdg-desktop-portal#2027 Signed-off-by: Evan Maddock <maddock.evan@vivaldi.net> **Test Plan** Install the package in both Budgie and Xfce VMs, and see that the portals start as expected. **Checklist** - [x] Package was built and tested against unstable - [ ] This change could gainfully be listed in the weekly sync notes once merged <!-- Write an appropriate message in the Summary section, then add the "Topic: Sync Notes" label --> - [x] I agree to license this contribution and all my previous contributions under the licensing terms in [LICENSE.md](../blob/main/LICENSE.md) and have the power and authority to grant those licenses.
While this is not ideal, we need to support desktop environments that do not bind to graphical-session.target, at least as a temporary measure. We keep the After=graphical-session.target stanza, since 'After' will only affect the ordering of start operations if both are queued up to start, while it is a no-op if graphical-session.target is not pending start. This partially reverts commit 4d284de. Resolves: flatpak#1983 Bug-Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=2481764 Bug-Ubuntu: https://launchpad.net/bugs/2144855
244a8d6 to
690c9a9
Compare
10c88f4 to
5c14e8a
Compare
|
Added a critical warning if we still hit the race, and a comment in the systemd service file for hinting how to get reliable ordering. |
| if (!getenv ("WAYLAND_DISPLAY") && !getenv ("DISPLAY")) | ||
| { | ||
| g_critical ("Windowing system environment variables not set; " | ||
| "make sure your session integrates with graphical-session.target."); | ||
| } |
There was a problem hiding this comment.
This seems like a good addition, yes.
In practice X11 sessions on well-integrated distros often make sure that these environment variables are set during early session startup: for example, in Debian, /etc/X11/Xsession.d is responsible for this. Hopefully in those sessions, the bad sequence won't be hit in practice.
Is there anywhere that documents how a desktop environment or window manager maintainer can/should integrate with graphical-session.target?
There was a problem hiding this comment.
Is there anywhere that documents how a desktop environment or window manager maintainer can/should integrate with graphical-session.target?
Sway has these docs: https://github.com/swaywm/sway/wiki/Systemd-integration#managing-user-applications-with-systemd
Some bits are sway specific of course, but it should be easily abstractible
Graphical sessions that does not notify graphical-session.target being reached will risk having xdg-desktop-portal.service started after the appropriate environment variables have been set in the D-Bus activation environment, which will cause applications launched directly from xdg-desktop-portal to not have the winsys env vars set, thus potentially fail to launch.
See "xdp: Allow service's start even if graphical-session is not reached" for details.
5c14e8a to
f3cb870
Compare
| Requires=dbus.service | ||
| After=dbus.service |
There was a problem hiding this comment.
Why do we need the Requires= and After= dependencies on dbus.service? We don't actually have such a dependency do we? Note that systemd implicitly adds Requires= and After= dependencies on dbus.socket for Type=dbus service units.
The difference is subtle: systemd's implicit dependency says "we need the dbus socket to exist so we can connect to it and start communicating with dbus". Our explicit dependency here is stronger, and specifies that we need the broker to be already up and running before we can start. I don't think we have such a requirement, do we?
There was a problem hiding this comment.
Why do we need the Requires= and After= dependencies on dbus.service?
It's there for shutdown sequencing, not for startup sequencing: 96d779e.
There was a problem hiding this comment.
Ah, this is another issue that seems to be caused by desktops not doing graphical-session.target. IMO these desktops can probably tolerate some extra log messages?
There was a problem hiding this comment.
If there's any build configuration where graphical-session.target is not required (even if that means removing a drop-in or whatever), then the spurious warnings will occur in that configuration, even in desktop environments that do integrate with graphical-session.target.
There was a problem hiding this comment.
That doesn't sound accurate? systemd's requirement dependencies and ordering dependencies are orthogonal. Even if there's no Requisite=graphical-session.target, the unit is still PartOf=graphical-session.target and thus will be cleanly stopped whenever graphical-session.target stops (which happens when the graphical session ends, before dbus.service is stopped later, and thus there should be no warnings)
| [Unit] | ||
| Description=Portal service | ||
| PartOf=graphical-session.target | ||
| Requisite=graphical-session.target |
There was a problem hiding this comment.
It really isn't a good idea for a graphical environment to run without also holding up graphical-session.target. xdg-desktop-portal is a strictly graphical session service, i.e. it makes no sense to run it on a TTY session. However, without this dependency, systemd will believe that it's perfectly valid to dbus activate xdg-desktop-portal in non-graphical environments. Desktops that don't start graphical-session.target are indistinguishable to systemd from non-graphical environments.
I understand that some desktops simply don't interact with the service manager. Lots of things will be broken in such environments, so I don't find it to be a huge deal to add this to the list. However, I don't think it's fair to regress this on desktops that do things correctly. So I have an alternate proposal.
Rather than simply commenting out the dependency, we can move it into a drop-in file. Distros can then fix this on their end, via packaging, without patching anything. The drop-in file can be split out into its own subpackage. The main xdg-desktop-portal package can have a "recommends" dependency on the subpackage so that it gets pulled in by default, and desktop environments that fail to communicate with systemd can have a "conflicts" dependency on the subpackage so that it gets removed whenever that desktop is installed.
This way, if you're just running GNOME or KDE, then xdg-desktop-portal will have correct dependencies. But if you install MATE or some other non-conformant desktop then the package manager will remove the drop-in and xdg-desktop-portal will allow itself to run in non-graphical environments, including MATE (which, again, is indistinguishable from a TTY to systemd)
I'd also urge distros to not include the conflicts dependency on window manager packages, like Sway or Hyprland or whatever. If the end-user is assembling the desktop environment themselves, they really aught to hook it into systemd. This breaking would notify them of the fact that they are running an invalid configuration. They should fix it (ideally), or explicitly uninstall the subpackage and opt into all the buggy behavior their invalid configuration brings.
There was a problem hiding this comment.
(Over time, when desktop environments figure out how to integrate with systemd here, we would then remove the drop-in and go back to having a Requisite= dependency directly in the unit file. Window manager folks can then either override the unit file or fix their systems)
There was a problem hiding this comment.
It really isn't a good idea for a graphical environment to run without also holding up
graphical-session.target
If this is a new integration requirement of what we might call "the freedesktop.org platform", then someone will need to go round all the desktop environments (downstream in each distro, and/or upstream) reporting issues to ask them to integrate correctly, and a prerequisite for being able to do that would be having a clear picture of what integrating correctly looks like. The Sway documentation linked from #2027 (comment) is the closest thing I've seen so far.
I did some mass-bug-filing in Debian last time we had a new integration requirement, portals.conf (which you'll see is about 30% finished, 3 years later). I haven't yet done that for graphical-session.target, because I don't fully understand what is required, and I don't want to give desktop environments counterproductive advice based on a wrong understanding.
non-conformant
I'm having a hard time applying this framing to desktop environments if there's nothing to tell them what design they ought to be conforming to!
desktop environments that fail to communicate with systemd can have a "conflicts" dependency on the subpackage
That still requires someone making a change in each desktop environment, to add the Conflicts: it isn't enough to make pre-existing packages work.
There was a problem hiding this comment.
because I don't fully understand what is required, and I don't want to give desktop environments counterproductive advice based on a wrong understanding.
I'm having a hard time applying this framing to desktop environments if there's nothing to tell them what design they ought to be conforming to!
They need to make graphical-session-pre.target run as the graphical session starts so that services can run and upload environment variables into the activation environment (like the Wayland compositor can upload WAYLAND_DISPLAY), and then graphical-session.target should run once the desktop initialization is done (including everything in graphical-session-pre.target) and it becomes valid to start running graphical apps.
What that looks like is up to the desktop. There's no generic guidance here, just like there's no generic guidance for how a desktop starts up its own services. The Sway approach is probably an easy entry point to doing this.
desktop environments that fail to communicate with systemd can have a "conflicts" dependency on the subpackage
That still requires someone making a change in each desktop environment, to add the Conflicts: it isn't enough to make pre-existing packages work
Frankly, this is the responsibility of the distro
There was a problem hiding this comment.
Oh, and also desktops need to order graphical-session.target's shutdown in such a way that the target stops when the graphical session ends. This will cleanly bring down all graphical services that are PartOf=graphical-session.target, rather than letting them get killed by systemd when the user manager eventually exits. Or, worse, crashing when the dbus broker is killed by systemd during user manager shutdown.
|
What about a system where systemd is configured to start in |
Is this a problem with the change proposed in this PR, or is this a problem with the change that it's partially reverting? |
You're confusing the systemd system manager (pid1) and user manager |
I am talking about a full blown systemd-based distro but starting with |
I am running a bare Xorg + i3 wm setup with Xorg started manually (no display manager) and systemd being in |
|
Congrats, you're doing things wrong. This is exactly why I'm pushing back against this hack. |
Thank you, I will continue doing things wrong. Enjoy people complaining. |
|
I will repeat myself: you are confusing the different instances of systemd with each other. |
|
Yes, I've been confusing |
|
The failure case isn't "I boot into Instead, the failure case that is being worked around here is if your desktop environment doesn't tell In other words, the reason @bubbleguuum initially thought this didn't work was "no display manager", but the reason that it actually didn't work was that i3wm is not one of the environments that integrates with |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Thank you, this fixed the service and a number of other functionality in my KDE Plasma desktop on openSUSE Tumbleweed, launched by SDDM.
After debugging some more, it looks like the sequence to start KDE is correct. But I had a 5-year-old |
Patch by @aleasto, originally released to Ubuntu.
Allow service's start even if graphical-session is not reached.
While this is not ideal, we need to support desktop environments that do not
bind to graphical-session.target, at least as a temporary measure.
We keep the After=graphical-session.target stanza, since 'After' will only
affect the ordering of start operations if both are queued up to start, while
it is a no-op if graphical-session.target is not pending start.
This partially reverts commit 4d284de.
Bug: Regression: D-Bus service does not start in MATE (1.21.1) #1983
Bug-Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=2481764
Bug-Ubuntu: https://launchpad.net/bugs/2144855
cc @jadahl