GNOME Bugzilla – Bug 788552
Ensure wayland -> xorg fallback to the corresponding session
Last modified: 2017-10-17 19:49:33 UTC
Created attachment 360955 [details] [review] proposed patch (bug initially reported for ubuntu 17.10 at https://launchpad.net/bugs/1718446) Ubuntu 17.10 has the following sessions by default: "ubuntu", "ubuntu-xorg", "gnome", "gnome-xorg" (and possibly unity if upgraded from a previous release). "ubuntu" and "gnome" are wayland sessions, and "ubuntu-xorg" and "gnome-xorg" are the corresponding X11 fallbacks, in case the system is not wayland-compatible (which can be emulated by uncommenting WaylandEnable=false in /etc/gdm3/custom.conf. If the session previously selected was "ubuntu" and the system cannot run wayland at the next boot, the expectation is that the fallback session selected by default will be "ubuntu-xorg". However with the current fallback selection logic, "gnome-xorg" is selected. I am not sure this can easily be upstreamed, but I am attaching a patch proposal targetted at ubuntu, and I would appreciate a review. Thanks!
Review of attachment 360955 [details] [review]: Thought I'd review as I saw it go by on Ubuntu IRC. I'm not an upstream reviewer though, so take this with a pinch of salt. First, please when you resubmit could you do it as a git formatted patch against gdm master? (see `git bz' if you haven't before, to make this easier in future) I'm not sure why you think this isn't suitable for upstream - it seems like it is to me. This approach assumes that "session_name-xorg" is a valid way to determine a fallback. I think that's established as a pattern now so this is OK in my mind, but I guess it could be up for debate whether we want to encode that in gdm or (say) have this information as a key in the .desktop files. ::: daemon/gdm-session.c @@ +554,3 @@ + } + } + g_free (name); This should be inside the conditional, or else you might free some random memory (name isn't initialised). @@ +950,3 @@ + g_free (self->priv->discarded_session); + self->priv->discarded_session = g_strdup (session_name); + gdm_session_defaults_changed (self); I think it'd be better to instead do all the logic (the above 'if') in here. We're not talking about the fallback (global default) session here, but a different type of fallback when the user's session isn't available but there is an equivalent. That is: get rid of `discarded_name' completely, check the strconcatted version here and set session_name and call gdm_dbus_greeter_emit_default_session_name_changed if that name is good.
so why don't you name both sessions the same name ? That's how fallback was originally designed.
(the -xorg is redundant, it's already in a directory called xsessions while wayland sessions go in a directory called wayland-sessions )
(In reply to Ray Strode [halfline] from comment #2) > so why don't you name both sessions the same name ? That's how fallback was > originally designed. That sounds good in theory, but in practice we want both xorg and wayland sessions to be presented to the user (in a wayland-capable scenario), and if both desktop files have the same basename then the wayland session is shadowing the xorg one, which is the reason why Ubuntu went for the "-xorg" suffix. Maybe a better approach would be to have unique identifiers for sessions be the full path, not just the basename?
(In reply to Ray Strode [halfline] from comment #2) > so why don't you name both sessions the same name ? That's how fallback was > originally designed. (In reply to Ray Strode [halfline] from comment #3) > (the -xorg is redundant, it's already in a directory called xsessions while > wayland sessions go in a directory called wayland-sessions ) The basename of the file is used as a key, see: https://git.gnome.org/browse/gdm/tree/daemon/gdm-session.c#n577 and https://git.gnome.org/browse/gdm/tree/libgdm/gdm-sessions.c#n167 So if you have two sessions with the same basename, one will shadow the other - I think the Wayland one will take priority. It's done in this way so that we can offer a "foo on Xorg" option in the greeter so people can explicitly choose that if they want it. (gnome-session does this as well as the Ubuntu sessions.) I guess you could in principle store the full path instead of the basename, but (1) existing configurations already reference the basename, and (2) it is documented to behave this way https://git.gnome.org/browse/gdm/tree/docs/C/index.docbook#n2052
but why not just ship both ubuntu-xorg.desktop and ubuntu.desktop in /usr/share/xsessions ?
(In reply to Ray Strode [halfline] from comment #6) > but why not just ship both ubuntu-xorg.desktop and ubuntu.desktop in > /usr/share/xsessions ? It needs to be in .../wayland-sessions/... so that it is started as a wayland session. https://git.gnome.org/browse/gdm/tree/daemon/gdm-session.c#n3036 and actually that's another place where the proposal to use the same basename would break things. We try to go from the basename back to the full path to work this out. https://git.gnome.org/browse/gdm/tree/daemon/gdm-session.c#n402 If we do the rename and have /usr/share/xsessions/gnome.desktop instead of gnome-xorg.desktop, we'll start the "gnome" session. Then when we go through this function to find the .desktop file I think we'll find /usr/share/wayland-sessions/gnome.desktop and start that one instead of the Xorg one that was selected previously.
I think we're miscommunicating, so let's reiterate for clarity. Your original proposal as I understand it: 1) If <"GNOME" /usr/share/wayland-sessions/gnome.desktop> doesn't work (because wayland is unavailable or is disabled) use <"GNOME on Xorg" /usr/share/xsessions/gnome-xorg.desktop> instead My counter proposal: 1) If <"GNOME" /usr/share/wayland-sessions/gnome.desktop> doesn't work (because wayland is unavailable or is disabled) use <"GNOME" /usr/share/xsessions/gnome.desktop> instead. 2) Provide <"GNOME on Xorg" /usr/share/xsessions/gnome-xorg.desktop> for users to pick X even when wayland is working and isn't disabled What's wrong with my counter proposal? Assuming that works, let's go with it, because it's what we've already been doing upstream for a couple of years.
(In reply to Ray Strode [halfline] from comment #8) > I think we're miscommunicating, so let's reiterate for clarity. Cheers. > Your original proposal as I understand it: > > 1) If <"GNOME" /usr/share/wayland-sessions/gnome.desktop> doesn't work > (because wayland is unavailable or is disabled) use <"GNOME on Xorg" > /usr/share/xsessions/gnome-xorg.desktop> instead Yes. > My counter proposal: > > 1) If <"GNOME" /usr/share/wayland-sessions/gnome.desktop> doesn't work > (because wayland is unavailable or is disabled) use <"GNOME" > /usr/share/xsessions/gnome.desktop> instead. > 2) Provide <"GNOME on Xorg" /usr/share/xsessions/gnome-xorg.desktop> for > users to pick X even when wayland is working and isn't disabled > > What's wrong with my counter proposal? Assuming that works, let's go with > it, because it's what we've already been doing upstream for a couple of > years. Oh right, I think we don't install /usr/share/xsessions/gnome.desktop in Ubuntu. With Wayland disabled, you see two options for the same session - "GNOME" and "GNOME on Xorg". Is that intentional? To me it implies that the first one is not "on Xorg", but it will be.
Confirming Iain's observation: with wayland enabled, Ray's proposal works fine, but with wayland disabled, I am seeing two entries for each X session (because there are two desktop files for effectively the same session in /usr/share/xsessions/, but with different names).
Created attachment 361243 [details] [review] proposed patch Updated proposed patch to be against gdm master, and applied Iain's suggestions.
Another possible approach would be to ignore "*-xorg.desktop" sessions when wayland is disabled. Possibly less intrusive?
hmm, I can see how the current approach does have a bit of a wart if wayland is disabled. I'm not a huge fan of encoding policy into the name of the desktop id though. I think ideally, what i'd like to see is 1) add a new key to the desktop file to, do as you suggest, maybe something like X-GDM-OnlyShowIn=wayland ? 2) change libgdm to get the list of sessions from the daemon instead of calculating the list on its own. That way we don't have to make the already hairy logic for figuring out what to show even hairier (optional extra credit) What do you think?
That sounds good to me. I'll give it a try.
If a user has {gnome,ubuntu}-xorg as their session and it goes away after applying this fix, what happens? I'm guessing they'll get the fallback session - can you think of a way we can do better than that?
They would probably get the fallback session indeed, and not even the correct one in all cases, because if ubuntu-xorg goes away gdm will probably pick the first valid session in its list, which would be gnome if present. See comment #12 for a less generic but also less intrusive solution (testing that now).
@Iain: we did discuss this on #ubuntu-desktop yesterday already and think a distro-patch for this short transition period is acceptable (17.10 only) (we rename -xorg selection to one without -xorg).
(In reply to Didier Roche from comment #17) > @Iain: we did discuss this on #ubuntu-desktop yesterday already and think a > distro-patch for this short transition period is acceptable (17.10 only) (we > rename -xorg selection to one without -xorg). I asked here in the hope that we could come up with an idea that works for all users, not just those who happen to use Ubuntu.
@Iain: I don't think that other users than ubuntu are in that situation: we were the one using -xorg prefix, and so, you comment on the -xorg transitioning only apply to ubuntu users, hence the relevant channel for discussion on this and not sidetracking the main topic here about the fallback mechanism itself.
I guess we could also go back to the proposal in comment 1 > This approach assumes that "session_name-xorg" is a valid way to determine a > fallback. I think that's established as a pattern now so this is OK in my > mind, but I guess it could be up for debate whether we want to encode that > in gdm or (say) have this information as a key in the .desktop files. So have like X-GDM-FallbackSession=ubuntu-xorg in ubuntu.desktop. Though I guess that might mean we'll have to read deskop files in /usr/share/wayland-sessions even when wayland is disabled ? maybe a little distro patch for the transition period is the easiest...not sure.
Iain, yea it's not a problem upstream, because gnome-xorg will properly fallback to gnome after the change... the only problematic case from comment 15 is ubuntu-xorg going to gnome instead of ubuntu-xorg going to ubuntu.
To clarify, I really think there are 2 issues: - an upstream one which is to handle multiple sessions with a controlled fallback. We want to fallback to the corresponding xorg session if wayland isn't supported. This is where X-GDM-FallbackSession= could be a good feature to have upstream. This would though need to take into account users who may want to select the corresponding Xorg session even if Wayland is available. - a second one, which is downstream only, which is, *if* we have to rename our <foo>-xorg session to <foo> to have that fallback mechanism in place, then a transient upgrade mechanism as a distro patch to rename the selected user session from <foo>-xorg to <foo>.
OK, a lot of cooks in this kitchen now so I'll probably step out after this comment. (In reply to Ray Strode [halfline] from comment #20) > I guess we could also go back to the proposal in comment 1 > > > This approach assumes that "session_name-xorg" is a valid way to determine a > > fallback. I think that's established as a pattern now so this is OK in my > > mind, but I guess it could be up for debate whether we want to encode that > > in gdm or (say) have this information as a key in the .desktop files. > > So have like X-GDM-FallbackSession=ubuntu-xorg in ubuntu.desktop. Though I > guess that might mean we'll have to read deskop files in > /usr/share/wayland-sessions even when wayland is disabled ? I'm thinking that when we throw out /usr/share/xsessions/ubuntu-xorg.desktop due to X-GDM-OnlyShowIn=wayland then we're already reading ubuntu-xorg.desktop, so we can do it this way around and put X-GDM-FallbackSession=ubuntu in ubuntu-xorg.desktop (same for gnome). Store that in a hash table or something (ubuntu-xorg → ubuntu) and then look at this when the session isn't found. (This part is similar to comment #11's patch but uses a lookup instead of the heuristic.) Does that sound sane? (In reply to Didier Roche from comment #19) > @Iain: I don't think that other users than ubuntu are in that situation: we > were the one using -xorg prefix, and so, you comment on the -xorg > transitioning only apply to ubuntu users, hence the relevant channel for > discussion on this and not sidetracking the main topic here about the > fallback mechanism itself. We aren't the only people doing this. It's upstream too. https://git.gnome.org/browse/gnome-session/tree/data/gnome-xorg.desktop.in (In reply to Didier Roche from comment #22) > To clarify, I really think there are 2 issues: > - an upstream one which is to handle multiple sessions with a controlled > fallback. We want to fallback to the corresponding xorg session if wayland > isn't supported. This is where X-GDM-FallbackSession= could be a good > feature to have upstream. This would though need to take into account users > who may want to select the corresponding Xorg session even if Wayland is > available. This bug is about - select a wayland session (/usr/share/wayland-sessions/foo.desktop) - disable wayland - get the right fallback (xorg) session (/usr/share/xsessions/foo.desktop) That part will already work with no code changes in GDM if Ubuntu provides /usr/share/xsessions/ubuntu.desktop (currently not there), but it makes a potentially confusing situation crop up where you have "Foo" and "Foo on Xorg" sessions visible. You want to try to hide "Foo on Xorg" in that case, which is what this bug is now trying to solve. > - a second one, which is downstream only, which is, *if* we have to rename > our <foo>-xorg session to <foo> to have that fallback mechanism in place, > then a transient upgrade mechanism as a distro patch to rename the selected > user session from <foo>-xorg to <foo>. I think we need to keep the -xorg.desktop files, so that the "on Xorg" option is still there. So I think any distro patch would end up implementing more or less what I outlined above, but with hardcoding instead of reading this from the desktop file. HTH.
Here is a shot at my proposal in comment #12. Since this hardcodes the "-xorg" suffix in code, I understand this is not the preferred approach, However this is a minor code change and as such much less risky, so potentially more suitable for a temporary distropatch in ubuntu, with final freeze for 17.10 tomorrow. In that regard comments are much appreciated! For this to work, the following layout is needed: $ ls -l /usr/share/xsessions/ total 12 lrwxrwxrwx 1 root root 18 oct. 11 17:38 gnome.desktop -> gnome-xorg.desktop -rw-r--r-- 1 root root 201 oct. 9 23:52 gnome-xorg.desktop lrwxrwxrwx 1 root root 19 oct. 11 17:33 ubuntu.desktop -> ubuntu-xorg.desktop -rw-r--r-- 1 root root 262 oct. 9 23:52 ubuntu-xorg.desktop -rw-r--r-- 1 root root 249 oct. 9 23:52 unity.desktop $ ls -l /usr/share/wayland-sessions/ total 8 -rw-r--r-- 1 root root 207 oct. 9 23:52 gnome.desktop -rw-r--r-- 1 root root 253 oct. 9 23:52 ubuntu.desktop Note that I found one interesting gotcha with this approach: if, with wayland enabled, I intentionally select ubuntu-xorg, then disable wayland, then reboot, in the login screen no session is selected in the dropdown (because gdm ignored ubuntu-xorg). However this will still launch the ubuntu-xorg session by default (which effectively is the same as the ubuntu session), so all is good (except for the fact that no session appears selected in the dropdown). This has the advantage of retaining the "ubuntu-xorg" session selected by the user if wayland is re-enabled later, though.
Created attachment 361336 [details] [review] proposed patch to ignore session IDs ending with "-xorg" under X11
Review of attachment 361336 [details] [review]: You know what, this is nice and self contained, let's just go with it. It does encode semantics into the -xorg naming scheme, which is a bit of a wart, but it fixes a user visible wart, so better to have a hidden wart than a user visible one. Feel free to push (assuming you've tested it). ::: libgdm/gdm-sessions.c @@ +178,3 @@ + if (is_x11 && g_str_has_suffix (filename, "-xorg.desktop")) { + char *base_name = g_strndup (filename, strlen (filename) - strlen ("-xorg.desktop")); + char *fallback_name = g_strconcat(base_name, ".desktop", NULL); missing space here. could probably consolidate both lines into g_strdup_printf
Created attachment 361757 [details] [review] proposed patch to ignore session IDs ending with "-xorg" under X11 Updated patch to add the missing space. I don't see how the two lines could possibly be consolidated into one without leaking memory.
> Feel free to push (assuming you've tested it). Yes, it's been pretty thoroughly tested. I don't think I have permissions to push though, can you do that on my behalf?
Alright, I can do that for you. Thanks all!
(In reply to Olivier Tilloy from comment #27) > I don't see how the two lines could possibly be consolidated into one > without leaking memory. for instance, char *fallback_path = g_strdup_printf ("%s/%.*s.desktop", dirname, strlen (filename) - strlen ("-xorg.desktop"), filename); (not saying it should get changed at this point, just answering for completeness)
D'oh, I wasn't aware of the %.*s syntax. I learnt something today, thanks!