Review of attachment 317253 [details] [review]:

::: src/core/main.c
@@ +352,3 @@
   gboolean is_wayland = FALSE;
 
+  session_type = g_strdup (g_getenv ("XDG_SESSION_TYPE"));

Does g_strdup even work on a NULL string?

It seems to work... with out the env var set, within my weston session (where the session type is NOT set from logind), it starts it nested.

I did it so that it could be something that wouldn't crash with free(), and so that I didn't have to put free() in an if statement

both g_strdup() and g_free() are NULL safe

That being said, I'm not in favor of adding an environment variable for this.

Review of attachment 317253 [details] [review]:

OK then. Please remove braces around the single-line if, otherwise OK.

(In reply to Matthias Clasen from comment #5)
> That being said, I'm not in favor of adding an environment variable for this.

I am.

Created attachment 317539 [details] [review]
Allow the XDG_SESSION_TYPE to be set with an environment variable

Uh, the braces are still in? Still missing a commit message, too.

Created attachment 317541 [details] [review]
Allow the XDG_SESSION_TYPE to be set with an environment variable

Allow the XDG_SESSION_TYPE to be set with an environment variable

Created attachment 317542 [details] [review]
Allow the XDG_SESSION_TYPE to be set with an environment variable

How does this overlap/interfere/conflict with the GNOME_SHELL_SESSION_MODE environment variable ?

(In reply to Matthias Clasen from comment #12)
> How does this overlap/interfere/conflict with the GNOME_SHELL_SESSION_MODE
> environment variable ?

Not at all.

There might be an issue if the session is created with logind and the variable is used to overwrite the session type with something else, but that's probably obscure enough to ignore.

So, the reason we're doing this is because when someone starts gnome-session from a TTY, the session type will be "tty", which means we always launch in X11 mode.

Couldn't we handle that by changing the check to session_type != "x11"?

There are also people who want to launch X11 sessions from a TTY.

Launching gnome-session from a TTY is not a supported use case.

Adding random environment variables to make an undefined set of ways to launch something that looks like a session work a little better is not making the overall code base better or more stable or supportable.

IMHO XDG_SESSION_TYPE is not that much of a random environment variable. it's supposed to be set by logind, (but in some cases it's not)

I also think this would be a good workaround for 
https://bugzilla.gnome.org/show_bug.cgi?id=758658
?

Is it decided that this patch won't be merged?

*** Bug 765053 has been marked as a duplicate of this bug. ***

The patches in 765053 have a slightly different reasoning. Apart from the desire to use XDG_SESSION_TYPE when the logind session type is neither "wayland" nor "x11", it has been expressed that wayland should become the default. The current code does not reflect that.

The first patch in 765053 does not include any reference to XDG_SESSION_TYPE and merely changes the fallback from x11 to wayland, mostly in line with Florian Müllner's comment 15.

(In reply to Jouke Witteveen from comment #22)
> The patches in 765053 have a slightly different reasoning. Apart from the
> desire to use XDG_SESSION_TYPE when the logind session type is neither
> "wayland" nor "x11", it has been expressed that wayland should become the
> default. The current code does not reflect that.

Well, wayland should become the default in the sense that GDM should open a logind session of type "wayland" by default. Running gnome-session from a logind session that is neither of type "wayland" nor "x11" is not really a supported use case, though there certainly are people who have it in their .xinitrc. If we are going to break that, I'd rather throw an "Unsupported session type" assertion than opening the same can of worms on wayland ...

The problem is that the logind session type is read-only. This means it is impossible to upgrade a text session to a graphical session. There is no technical reason for this limitation and it can be made to work properly given the right semantics.

The patch in this bug gives XDG_SESSION_TYPE precedence over the logind session type which, I think, is a bad idea with potential inconsistent behavior. Precisely because the logind session type is read-only, it should be trusted more than XDG_SESSION_TYPE.
Furthermore, if the XDG_SESSION_TYPE is of no use, the patch in this bug defaults to x11, which is not what we want.

The second patch from Bug 765053 implements well defined logic:
- the logind session type takes precedence over XDG_SESSION_TYPE
- the first of these two places where a usable value ("wayland" or "x11", currently) is found will be used
- the default/fallback is wayland

Support through .xinitrc is easily achieved using the line

  export XDG_SESSION_TYPE=x11

This can also be set in whatever sources the .xinitrc. In many cases it might even be possible to set the right logind session type for sessions using .xinitrc.

In conclusion, I think the second patch of Bug 765053 implements well-defined and maintainable behavior that re-enables use cases that were supported but got broken for no technical reason.

(In reply to Jouke Witteveen from comment #24)
> The problem is that the logind session type is read-only. This means it is
> impossible to upgrade a text session to a graphical session.

So? It's not possible to "downgrade" a graphical session to a text session, it's not possible to "upgrade" an x11 session to a wayland session, ...

Two system components - logind and mutter/gnome-shell - which assume differing session types has potential for subtle bugs that are hard to debug (because nobody will be able to reproduce the issue when the reporter forgets to mention that the session is launched via "startw").


> Support through .xinitrc is easily achieved using the line
> 
>   export XDG_SESSION_TYPE=x11

Well, I was partly referring to:

(In reply to Jouke Witteveen from comment #22)
> The first patch in 765053 does not include any reference to XDG_SESSION_TYPE
> and merely changes the fallback from x11 to wayland

But even if we start reading XDG_SESSION_TYPE, changing the default "graphical tty" fallback to wayland will break existing setups, so we'll still get the complaints.

(In reply to Florian Müllner from comment #25)
> Two system components - logind and mutter/gnome-shell - which assume
> differing session types has potential for subtle bugs that are hard to debug

One option around this would be asking the logind developers to support something like:

  loginctl start-session --type=wayland gnome-session
or
  loginctl start-session --type=x11 gnome-session

for starting a graphical session from a text one. There'll probably still be issues with screen locking, but at least logind and GNOME would have the same view of the session type ...

(In reply to Florian Müllner from comment #25)
> (In reply to Jouke Witteveen from comment #24)
> > The problem is that the logind session type is read-only. This means it is
> > impossible to upgrade a text session to a graphical session.
> 
> So? It's not possible to "downgrade" a graphical session to a text session,
> it's not possible to "upgrade" an x11 session to a wayland session, ...

I don't see why not. With any of the proposed patches in place, you can make a text session graphical and by stopping the graphical part you get back the text session. This also has worked for as long as startx has.

> > Two system components - logind and mutter/gnome-shell - which assume
> > differing session types has potential for subtle bugs that are hard to debug
> 
> One option around this would be asking the logind developers to support
> something like:
> 
>   loginctl start-session --type=wayland gnome-session
> or
>   loginctl start-session --type=x11 gnome-session
> 
> for starting a graphical session from a text one. There'll probably still be
> issues with screen locking, but at least logind and GNOME would have the
> same view of the session type ...

Possibly, starting a new session also makes it impossible to shutdown/reboot. I think the use case at hand is really about promoting text sessions to graphical ones.

For all I care the logind session type could be made writable. Of course, this is potentially dangerous, but as far as I can tell it is not _always_ dangerous.

> > Support through .xinitrc is easily achieved using the line
> > 
> >   export XDG_SESSION_TYPE=x11
> 
> Well, I was partly referring to:
> 
> (In reply to Jouke Witteveen from comment #22)
> > The first patch in 765053 does not include any reference to XDG_SESSION_TYPE
> > and merely changes the fallback from x11 to wayland
> 
> But even if we start reading XDG_SESSION_TYPE, changing the default
> "graphical tty" fallback to wayland will break existing setups, so we'll
> still get the complaints.

We should be getting complaints! The status-quo is ugly and at the very least we should indeed throw an "Unsupported session type" when the logind session type is not sane (and maybe XDG_SESSION_TYPE too). Every setup without a proper logind session type should be considered broken in the current state of affairs. The thing with supporting XDG_SESSION_TYPE is that it provides an easy way to fix these broken setups.

(In reply to Jouke Witteveen from comment #27)
> > So? It's not possible to "downgrade" a graphical session to a text session,
> > it's not possible to "upgrade" an x11 session to a wayland session, ...
> 
> I don't see why not. With any of the proposed patches in place, you can make
> a text session graphical and by stopping the graphical part you get back the
> text session. This also has worked for as long as startx has.

But you can not log into a graphical session and change it to become a text session. My point that "upgrading" the session type isn't something that generally works, but what works and what not is fairly random (or if you prefer, something best filed under "historical baggage") still stands.


> > But even if we start reading XDG_SESSION_TYPE, changing the default
> > "graphical tty" fallback to wayland will break existing setups, so we'll
> > still get the complaints.
> 
> We should be getting complaints! The status-quo is ugly and at the very
> least we should indeed throw an "Unsupported session type" when the logind
> session type is not sane (and maybe XDG_SESSION_TYPE too).

Now I'm confused. Don't you want to change the current ugly behavior of assuming "x11" if the session type is neither "x11" nor "wayland" to assuming "wayland" instead, which is just as ugly?

(In reply to Florian Müllner from comment #28)
> (In reply to Jouke Witteveen from comment #27)
> My point that "upgrading" the session type isn't something that
> generally works, but what works and what not is fairly random (or if you
> prefer, something best filed under "historical baggage") still stands.

Indeed, the fact that it had worked for the past decade(s) counts as historical baggage and might not be a reason to implement it today. I do think the use case (startx) should remain covered, also with wayland (`startw').

The easiest way I can currently think of to have a text login prompt followed by a graphical session is to set the session type to "wayland", already for the textual part of the session. This is of course a terrible hack.

> > > But even if we start reading XDG_SESSION_TYPE, changing the default
> > > "graphical tty" fallback to wayland will break existing setups, so we'll
> > > still get the complaints.
> > 
> > We should be getting complaints! The status-quo is ugly and at the very
> > least we should indeed throw an "Unsupported session type" when the logind
> > session type is not sane (and maybe XDG_SESSION_TYPE too).
> 
> Now I'm confused. Don't you want to change the current ugly behavior of
> assuming "x11" if the session type is neither "x11" nor "wayland" to
> assuming "wayland" instead, which is just as ugly?

After your Comment 23 I can think of only two ways forward:
1) Throw an error whenever the logind session type is not supported
2) Implement a clear logic involving XDG_SESSION_TYPE

For option 2 the question comes up what the default value should be (we could still choose to throw an error if $XDG_SESSION_TYPE is also not supported). Given the behavior of gdm, I think the proper default would be "wayland".

Indeed, in both these cases startx will potentially break, although with the second option there is an easy fix. Any startw case is already broken by the current code, even though it worked with 3.18.

The current code, following Ray Strode's 8ec0c99f, checks the session types of all active sessions of the user if the session type of the current session could not be determined. These sessions may even be on different seats than the current session. Why is this? I still feel the best is to:

1) check whether the current session is of supported type
2) otherwise, check whether XDG_SESSION_TYPE is supported
3) otherwise, throw an error (no fallback)

Any chance of a patch along these lines getting accepted?

Created attachment 326391 [details] [review]
Sensible session type logic

1) check whether the current session is of supported type
2) otherwise, check whether XDG_SESSION_TYPE is supported
3) otherwise, throw an error (no fallback)

(In reply to Jouke Witteveen from comment #30)
> The current code, following Ray Strode's 8ec0c99f, checks the session types
> of all active sessions of the user if the session type of the current
> session could not be determined. These sessions may even be on different
> seats than the current session. Why is this?
There's a concerted push to start things in the scope of systemd --user, not in the scope of a session. For instance, these days dbus-daemon is started outside of any given session (so it's "user bus" not a "session bus" now) and likewise anything dbus activated starts that way.  The plan going forward wrt to multi-seat (though it's not yet implemented) is to show a dialog like

   User '%s' is currently logged into another seat. [ Wait ] [ Force Log Out other session ]

so my thoughts are:

1) keeping startx working has some legacy value. 
2) i care a lot less about being able to run graphical wayland sessions on VTs that are registered as non-graphical. wayland is supposed to be our chance to break away from weird things like that.
3) Now that doesn't mean we shouldn't be able to trigger a new wayland session on a new VT to start from the command line.  Something like Florian proposes in comment 26 would be useful.  It could be in logind or gdm.  Of course, if GDM isn't running (at least in headless mode) then screen lock won't work.

Does that mean the proposed patch is out of the question? I do think it is an improvement over the current situation. It provides more flexibility, clearer logic and a more explicit attitude regarding startx. It will keep working, but users will be confronted with the fact that things are changing.

i don't think startx will continue to work with that patch ? Unless i'm missing something, the user will have to set XDG_SESSION_TYPE=x11 before running startx , right?

Yes, it's at the same time breaking a legacy feature and bringing it into the wayland world.

so, for the time being, we could let getenv ("DISPLAY") != NULL and logind session-type=="tty" imply X backend I guess and otherwise error out?

(In reply to Florian Müllner from comment #23)
> Running gnome-session from a
> logind session that is neither of type "wayland" nor "x11" is not really a
> supported use case, though there certainly are people who have it in their
> .xinitrc. If we are going to break that, I'd rather throw an "Unsupported
> session type" assertion than opening the same can of worms on wayland ...

The alternative, I think, is to accept the patch without the part that pulls in XDG_SESSION_TYPE. If startx is "not really a supported use case" then it should not be supported and throw an error.

By the way, what exactly is this 'can of worms' associated with supporting an XDG_SESSION_TYPE fallback also for Wayland?

It occurred to me that we can install a file with the line

    export XDG_SESSION_TYPE=x11

into /etc/X11/xinit/xinitrc.d/.
That way, startx will keep working for many/most setups.

Created attachment 329772 [details] [review]
Sensible session type logic

This patch has it all.

It has a clear logic for inferring the session type (same as comment #31) and sets it to x11 by default for xinit-based sessions.

All but the most broken legacy setups will remain working and modern (wayland) setups that are broken now get a proper error message.

Review of attachment 329772 [details] [review]:

::: data/Makefile.am
@@ +24,3 @@
 convert_DATA = mutter-schemas.convert
 
+xinitrcdir = $(sysconfdir)/X11/xinit/xinitrc.d

this dir is distro specific (won't work on debian for instance). to be honest, we've been trying to get away with loading shell scripts at login.

can you give comment 37 a whirl ?

Created attachment 329809 [details] [review]
Sensible session type logic

We should not even bother testing whether XDG_SESSION_TYPE is already set in the xinitrc script: If the variable is not equal to "x11" it is wrong for xinit anyway.

In reply to comment 43 (comment 44 was posted simultaneously):
That would leave no way to start a wayland session from the console, which is precisely what I am after for myself. Until we can upgrade sessions to graphical ones, or start new sessions from running sessions, there appears to be no way to run mutter on wayland without a desktop manager. At least with this patch there is.

Indeed, Debian does not use the xinitrc.d dir. I assume their packagers are familiar with the troubles it can give, since most of the other distros do use it. Mind you: without the xinitrc snippet you can still use it through xinit, you just have to make sure XDG_SESSION_TYPE=x11 is present somewhere else. This is a good idea anyway in such legacy setups.

Note that systemd too installs a script, 50-systemd-user.sh, in $(sysconfdir)/X11/xinit/xinitrc.d.

Created attachment 330432 [details] [review]
Well-defined session type logic

Similar to the previous patch, but instead of mandating xinit sessions to set XDG_SESSION_TYPE, fall back to the X backend when we find

getenv ("DISPLAY") != NULL (or getenv ("MUTTER_DISPLAY") != NULL) and logind session-type=="tty"

As suggested in comment #37.
This does away with the xinitrc script.

After three weeks, what is the verdict on the last patch? Does it satisfy everyone's needs?

Review of attachment 330432 [details] [review]:

yea good enough for me. do you have commit access?

Awesome!

Nope, no commit access on my side.