After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 736660 - wayland session does not process .bash_profile
wayland session does not process .bash_profile
Status: RESOLVED FIXED
Product: gnome-session
Classification: Core
Component: gnome-session
3.13.x
Other Linux
: Normal normal
: ---
Assigned To: Session Maintainers
Session Maintainers
: 738336 738665 747854 748491 (view as bug list)
Depends on:
Blocks: WaylandRelated
 
 
Reported: 2014-09-15 07:37 UTC by Tuomas Kuosmanen
Modified: 2017-02-08 08:46 UTC
See Also:
GNOME target: 3.22
GNOME version: ---


Attachments
gdm-wayland-session: keep connection to session bus alive (9.18 KB, patch)
2016-08-29 16:38 UTC, Ray Strode [halfline]
committed Details | Review
gdm-{wayland,x}-session: import environment from systemd manager (23.71 KB, patch)
2016-08-29 16:39 UTC, Ray Strode [halfline]
committed Details | Review
gnome-session: make sure wayland sessions get a login shell (1.46 KB, patch)
2017-01-05 15:33 UTC, Ray Strode [halfline]
none Details | Review
Revert "gnome-session: make sure wayland sessions get a login shell" (1.33 KB, patch)
2017-01-09 18:58 UTC, Ray Strode [halfline]
committed Details | Review
gnome-session: change how environment is imported (1.29 KB, patch)
2017-01-09 18:59 UTC, Ray Strode [halfline]
none Details | Review
gnome-session: make sure wayland sessions get a login shell (1.58 KB, patch)
2017-01-09 18:59 UTC, Ray Strode [halfline]
none Details | Review
gnome-session: update activation environment from C code, instead of shell script (10.77 KB, patch)
2017-01-13 15:52 UTC, Ray Strode [halfline]
committed Details | Review
gsm-util: export environment to systemd (11.78 KB, patch)
2017-01-13 15:52 UTC, Ray Strode [halfline]
committed Details | Review
gnome-session: make sure wayland sessions get a login shell (1.44 KB, patch)
2017-01-13 15:52 UTC, Ray Strode [halfline]
committed Details | Review

Description Tuomas Kuosmanen 2014-09-15 07:37:06 UTC
Fedora 21 prerelease with gnome-session-wayland-session-3.13.3-2.fc21.x86_64

I noticed the wayland session does not appear to process .bash_profile, since after logging in, ~/bin is not in $PATH
Comment 1 Ray Strode [halfline] 2014-09-16 13:08:50 UTC
indeed, in Fedora, X sessions run through /etc/X11/xinit/Xsession which runs at login time.  we don't run Xsession for wayland sessions.  answer might be to run gnome-session through a login shell if it hasn't already been run through it.
Comment 2 Ray Strode [halfline] 2014-11-17 19:11:02 UTC
*** Bug 738336 has been marked as a duplicate of this bug. ***
Comment 3 Ray Strode [halfline] 2015-04-16 14:00:04 UTC
*** Bug 747854 has been marked as a duplicate of this bug. ***
Comment 4 Ray Strode [halfline] 2015-08-13 15:33:34 UTC
*** Bug 748491 has been marked as a duplicate of this bug. ***
Comment 5 Ray Strode [halfline] 2015-08-13 16:06:54 UTC
so there was a little discussion about this today:

<halfline> owen: so bug number is https://bugzilla.gnome.org/show_bug.cgi?id=736660
<bugbot> Bug 736660: gnome-session, normal, Normal, ---, gnome-session-maint, NEW , wayland session does not process .bash_profile
<halfline> owen: X sessions are piped through /etc/X11/xinit/Xsession in fedora
<halfline> which does exec -l $SHELL -c ...
<halfline> to get login shell run
<grawity> hmm what is a login shell needed for?
<halfline> owen: we don't use Xsession in wayland
<halfline> i'd like to keep it that way, since i want to get away with running a bunch of shell scripts in the login path
<owen> grawity: well, the main thing is that it allows your bash_profile to set environment variables that apply to the entire session
<halfline> owen: also, other distros, don't run a login shell at all for X
<owen> grawity: It's actually entirely broken right now, in that gnome-terminal by default doesn't run a login shell for the terminals either
<owen> grawity: so ~/.local/bin doesn't end up in $PATH, etc.
<grawity> ah
<halfline> owen: so you should set PATH in .bashrc
<halfline> which gets sourced for all interactive shells
<grawity> well I'd say gnome-terminal is right here; it's confusing when $PATH is available in terminals but not in other programs
<halfline> hmm but .bashrc won't get run at login time either i guess
<owen> halfline: the idea that users are supposed to work all the time in an environment where ~/.profile isn't read - is weird, o?
<owen> no?
<Jasper> halfline, we want to run a login shell, since we rely on /etc/profile.d/ being run
<Jasper> halfline, Debian doesn't, which annoys me to no end.
<halfline> Jasper:  well /etc/profile.d is actually run for interactive shells as well
<halfline> not just login shells
<halfline> owen: it is weird on some level
<Jasper> halfline, are you sure? It isn't for me.
<grawity> is it? haven't seen bash do that
<Jasper> halfline, I thought the *profile's were only for login shells.
<Jasper> .profile, .bash_profile, /etc/profile are only run on login shells, I thought
<halfline> owen: what i mean is, if you accept that .bash_profile should be a feature of a modern gui then it needs to run at some point
<grawity> generally yes
<halfline> maybe it's a fedora patch
<halfline> but fedora's /etc/bashrc runs profile.d scripts
<halfline> owen: basically debian hasn't ever had it run
<owen> halfline: well, there's two things a) do we need a way to set environment variables for the session b) should gui terminals be running in a login shell - if so, how?   You are claiming that in "other distros" (Debian vs. Fedora?) gui terminals don't run in a login shell?
<halfline> that's right
<grawity> also ... even if the session runs under a login shell, dbus-daemon doesn't anymore, does it? so $PATH won't be available for it either, will it
<halfline> grawity: that's right
<halfline> owen: we actually have a feature in gdm for setting environment variables for the session now
<halfline> drop a file in /usr/share/gdm/env.d
<owen> halfline: oh?
<halfline> https://bugzilla.gnome.org/show_bug.cgi?id=751158
<bugbot> Bug 751158: gdm, normal, Normal, ---, gdm-maint, RESOLVED FIXED, Support /etc/gdm/env.d
<owen> Ah, not for a user session, but systemwide?
<halfline> well it's for the environment of user sessions, but it's system wide
<owen> halfline: yeah
<halfline> owen: maybe we should also source .config/environment or something like that
<halfline> would be a one line change to the pam configuration
<drago01> well gnome-terminal is odd anyway when run it directly it sets PATH as defined in .bashrc but if you open it from nautilus it doesn't
<grawity> pam_env already reads ~/.pam_environment, but its syntax is a bit icky... had troubles even using ${HOME}/bin
<halfline> drago01: that's very strange
<halfline> grawity: pam_env won't read ~/.pam_environment by default
<drago01> halfline: yeah never tried to debug it so no idea whats going on
<grawity> hmm I was sure it does
<halfline> you have to put user_readenv=1 on the pam command line
<owen> halfline: Somehow this needs to get standardized somewhat widely
<grawity> halfline: but that's on by default
<grawity> at least the manpage says so
<grawity> (so does my testing on Arch)
<halfline> q. By default this option is off as user supplied
<halfline>            environment variables in the PAM environment could affect behavior
<halfline>            of subsequent modules in the stack without the consent of the
<halfline>            system administrator.
<halfline> our man pages are different !
<grawity> hmm well Arch has http://sprunge.us/MbAf
<halfline> owen: i don't necessarily disagree
<halfline> owen: but in some ways, as long as everyone is using GDM it doesn't matter too much
<halfline> and we sort of require GDM to get a full gnome experience anyway
<halfline> (screen unlocking etc depend on gdm)
<halfline> so i think if we pick a good name like ~/.config/environment then we're probably square
<halfline> and other display managers can follow suit on their own time
<halfline> or i could start a bike shed discussion on xdg-list
<halfline> grawity: interesting, your man page and my man page are different !  wonder which distro patched it
<grawity> well the only two patches Arch has are for CVEs
<halfline> owen: i mean i'm not strictly opposed to making it run the login shell at login time
<halfline> it's just
<halfline> 1) with wayland we have an opportunity to get away from a lot of shell script baggage
<-- owen has quit (Ping timeout: 182 seconds)
<halfline> 2) and other distros haven't ever had that baggage so i'm inclined to stay away from introducing it again
<halfline> grr ping timeout
<krnowak> halfline: If you look at http://pkgs.fedoraproject.org/cgit/bash.git/tree/ you'll see barrelloads of patches there.
<grawity> http://pkgs.fedoraproject.org/cgit/pam.git/tree/pam-1.1.3-nouserenv.patch
<halfline> well this would be a Linux-PAM patch not a bash patch
<halfline> grawity: heh
* grawity suddenly wonders how one would change $XDG_CONFIG_HOME if ~/.config/environment was used
<halfline> grawity: of course upstream Linux-PAM is hosted on fedorahosted.org and the upstream and downstream maintainer are they same person
<halfline> weird that there's a distro patch
<grawity> there's a dozen of them, even
<halfline> funny
<baedert> Does anyone have some documentation at hand how to use one of the foundation chromebooks? The OS is missing/damaged and the chrome recovery tool thingy doesn't support Linux...
<halfline> grawity: i guess XDG_CONFIG_HOME would have to be changed from /etc/environment
Comment 6 Ray Strode [halfline] 2015-08-13 16:39:49 UTC
<grawity> ... so there could be {/etc,/usr/lib}/env.d/* for packages and other system stuff (e.g. perl needs to add its core/vendor/site_perl directories to $PATH), but also /etc/env.user.d/<login> and $XDG_CONFIG_HOME/environment for per-user stuff ...
<halfline> grawity: yea maybe something like that
<halfline> perhaps it should be in pam_systemd ...
<grawity> maybe
<grawity> I think some distros already use pam_env for the package environment?
<halfline> yea, pam_env would be okay too, and logical
<grawity> perhaps if it wasn't so difficult to achieve the equivalent of FOO="$HOME/bar" with it
<halfline> yea it's very strange
<halfline> it has two different file formats
<halfline> one that allows that kind of thing and one that doesn't
<grawity> and the one which does allow it appears to lack both $HOME and $PATH sometimes
<halfline> well those things are set by gdm after the auth stack has run
<halfline> and pam_env is run as part of the auth stack
<halfline> well in fedora anyway
<halfline> so putting "session optional pam_env.so user_readenv=1 user_envfile=.config/environment" in the very bottom of the pam file would fix that issue
<halfline> oh i guess it wouldn't though
<halfline> since envfiles are the file format that doesn't support substitution
<grawity> hmm its parser actually doesn't care
<grawity> so both formats work in ~/.pam_environment
<halfline> oh interesting, didn't know that
<grawity> it's a rather weird module
<halfline> yea so just making it a session module instead of (or in addition to) and auth module would fix the issue i guess
<halfline> but if we could get systemd to do it, then we could maybe cleave ourselves away from pam_env and it's oddities
<grawity> well, pam_systemd already sets DBUS_SESSION_BUS_ADDRESS, so the precedent is already there
<halfline> grawity: yea and systemd must already have a parser since EnvironmentFile is a thing...
<halfline> though i don't know if EnvironmentFile supports substitution variables
<grawity> afaik it doesn't
Comment 7 Ray Strode [halfline] 2015-08-13 18:20:43 UTC
<owen> halfline: it sounds like the reading of environment variables in pam is at the "wrong time"
<halfline> well pam_env is currently done first thing before all the other modules
<halfline> so it can set up the system environment for them
<halfline> there's nothing that would prevent us for adding it at the very bottom too, for user environment
<owen> halfline: it seems like the fedora patch is saying that for that reason having user environment variables set then is bad
<halfline> exactly
<halfline> which i totally agree with
<halfline> since pam modules run as root
<halfline> it's also wrong for a different reason... we don't know the username (necessarily) until auth completes
<halfline> since it's valid to have the final username be different than what the user typed originally
<halfline> and likewise the home directory won't necessarily be known
<halfline> or even available at all until authentication completes
<halfline> so i think the fedora default is right, but i wonder why it's not upstreamed
<owen> Looking at /etc/profile.d, it looks like any static envvar reading solution will also break ~/.i18n and xdg-app
<halfline> xdg-app already installs a .env file in /usr/lib64/gdm/env.d
<halfline> and we get .i18n stuff from accountsservice
<owen> does accountservice read ~/.i18n or is it separate from /var ?
<halfline> separate
<owen> halfline: with that you can ust set LANG right, not LC_NUMERIC or whatever?
<halfline> oh true
<owen> halfline: I guess if we had a ~/.config/environment we could document it as a replacement for ~/.i18n
<halfline> right
<halfline> i'm pretty sure .i18n isn't really standardized either
<halfline> smcv: do you know if it's used in debian ?
<smcv> halfline: I've never heard of ~/.i18n being used in Debian
<owen> halfline: wonder how control-center/Region & Language/Formats works
<halfline> shell script wrapper around gnome-settings-daemon
<halfline> owen: cat /usr/libexec/gnome-settings-daemon-localeexec
<owen> halfline: confusion ... that's to what? don't you have to relogin to affect apps?
<halfline> owen: well settings-daemon runs first and then i think it pushes it into the gnome-session environment
<halfline> hmm though, my git grep can't confirm that so far
<halfline> also with wayland settings-daemon doesn't run first
<owen> halfline: where's the candle lit under the tray of water with the frog in it that jumps onto the trapdoor?
<halfline> just before the level that lets the ball free to roll down the rail toward the the teeter totter
<halfline> *lever
<halfline> ah there it is
<halfline> it's in main.c
<halfline> was doing git grep in the plugins dir
<halfline> so yea that's broken in wayland
<halfline> so instead of ~/.config/environment it should be ~/.config/env.d i guess and control-center should drop a file there
<halfline> broken at both ends
<owen> I think a mail to xdg-list more along the lines of "I'm doing this" might be useful if you do switch things around
* owen needs to switch sessions
<halfline> yea the question is who should do it
<halfline> systemd or gdm
<halfline> or pam_env
Comment 8 Ray Strode [halfline] 2015-10-09 11:33:43 UTC
*** Bug 738665 has been marked as a duplicate of this bug. ***
Comment 9 Christian Stadelmann 2016-03-06 21:32:21 UTC
See also this downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1149905
Comment 10 Simon McVittie 2016-04-16 23:20:52 UTC
(In reply to Ray Strode [halfline] from comment #5)
> <Jasper> halfline, we want to run a login shell, since we rely on
> /etc/profile.d/ being run
> <Jasper> halfline, Debian doesn't, which annoys me to no end.

Conversely, we do source ~/.xsessionrc, which other distributions probably don't... and we have a directory of hooks (/etc/X11/Xsession.d) which is relied on by various packages.

I can see why you want to get away from this stuff, and Wayland is as good an opportunity as any to simplify the login sequence, but we probably do still need some way for packages to set up integration glue.
Comment 11 Christian Stadelmann 2016-04-17 12:17:52 UTC
A side note: For security reasons (maliciously crafted .bash_profile file) it would be better to not read any of these bash files which are able to execute arbitrary code. BUT: this would break stuff.
Comment 12 Simon McVittie 2016-04-17 12:40:36 UTC
(In reply to Christian Stadelmann from comment #11)
> A side note: For security reasons (maliciously crafted .bash_profile file)
> it would be better to not read any of these bash files which are able to
> execute arbitrary code.

Not really. If an attacker can write to files with the same level of sensitivity as ~/.bash_profile, they already have any number of ways to execute arbitrary code: the ones I can immediately think of are ~/.config/systemd/user, ~/.local/share/dbus/services and ~/.config/autostart.

The solution is to not let an attacker write to those files, by sandboxing potentially-exploitable apps using containerization (xdg-app) or LSMs (AppArmor, SELinux, etc.) or preferably both.
Comment 13 Simon McVittie 2016-05-09 14:51:48 UTC
Ray, I see you closed this. What's the intended resolution? Is there something that you now expect distributors to be doing?

Commits a8896cc, 39f146e, d14f6db suggest that a full solution is still pending?
Comment 14 Ray Strode [halfline] 2016-05-09 15:39:26 UTC
no i didn't close this intentionally. I just referenced this bug in a git message and git-bz closed it i guess.
Comment 15 Simon McVittie 2016-06-23 15:55:47 UTC
While at the GTK hackfest I spent some time talking to other developers about a potential solution for setting environment variables (but specifically not running other code) via a new or improved PAM module. There's a brief writeup of a possible plan on my blog, <http://smcv.pseudorandom.co.uk/2016/gtk-hackfest/> (look for "Environment variables"). I'm going to try to follow up on this soon with a writeup of the problem and a prototype solution.

(Of course, if you have better ideas, please say.)
Comment 16 sassmann 2016-06-28 09:55:42 UTC
Where am I supposed to put my "xrdb -load ~/.Xresources" from now on? And what about ssh-agent?
Comment 17 Ray Strode [halfline] 2016-06-28 12:27:20 UTC
you can run things at login time using desktop files in ~/.config/autostart or /etc/xdg/autostart.  So you could do your xrdb that way.  Although, I think there aren't many popular programs that rely on Xresources left.  Maybe you could configure emacs to run that command when emacs first starts.

We already start an ssh-agent, though, so you should be set on that front.
Comment 18 sassmann 2016-06-28 13:26:43 UTC
The most prominent example for Xresources is probably xterm, which is unlikely to go away in the near future. That makes Xresources a hard dependency for me.

~/.config/autostart should work in theory, although I was hoping for something more convenient. The problem with desktop files is that it doesn't recognize $HOME or ~ for specifying the home directory. Seems more like a band-aid to me.

Any other solutions?
Comment 19 Ray Strode [halfline] 2016-06-28 14:11:59 UTC
> The most prominent example for Xresources is probably xterm, which is
> unlikely to go away in the near future. That makes Xresources a hard
> dependency for me.
Indeed, okay. 

> ~/.config/autostart should work in theory, although I was hoping for
> something more convenient. The problem with desktop files is that it doesn't
> recognize $HOME or ~ for specifying the home directory.
either:

Exec=xrdb -load .Xresources

or

Exec=/bin/sh -c "xrdb -load ~/.Xresources"

should work, I think.

You could also put a service file in /lib/systemd/user/ but it may race with DISPLAY getting imported to the systemctl --user environment, so I don't think that's a satisfactory solution right now.  Another option is mutter could run xrdb -load ~/.Xresources automatically if the file exists.  Historically we had to stop calling xrdb at start up because it was really slowing down login (because it invoked gcc for preprocessing, not sure if it still does).  Anyway, if we only run it when ~/.Xresources exists then it will only affect those that "opt in" i guess.
Comment 20 sassmann 2016-06-28 14:30:16 UTC
I really like the opt-in idea and it shouldn't take too much time nowadays.
# time xrdb -load ~/.Xresources

real    0m0.003s
user    0m0.000s
sys     0m0.003s

Thanks Ray!
Comment 21 Mantas Mikulėnas (grawity) 2016-06-28 16:01:07 UTC
(In reply to Ray Strode [halfline] from comment #19)
> You could also put a service file in /lib/systemd/user/ but it may race with
> DISPLAY getting imported to the systemctl --user environment, so I don't
> think that's a satisfactory solution right now.  Another option is mutter
> could run xrdb -load ~/.Xresources automatically if the file exists. 
> Historically we had to stop calling xrdb at start up because it was really
> slowing down login (because it invoked gcc for preprocessing, not sure if it
> still does).  Anyway, if we only run it when ~/.Xresources exists then it
> will only affect those that "opt in" i guess.

The old /etc/gdm/Xsession used to run xrdb every time, didn't it? (It uses -nocpp to avoid the preprocessor slowdown.) I am not sure how I feel about this being hardcoded within the WM, though.

That said, there are other places that Xlib looks for resources in:

 * ~/.Xdefaults-$HOSTNAME is always read, even if xrdb has been used already (unlike the main ~/.Xdefaults file).
 * If $XAPPLRESDIR is set, xterm will also load $XAPPLRESDIR/XTerm.

So even if you don't like gnome-terminal for some reason, xrdb isn't really critical.
Comment 22 Ray Strode [halfline] 2016-06-30 12:46:54 UTC
sounds like a reasonable answer to me.  

Stefan, does mv ~/.Xresources ~/.Xefaults-$HOSTNAME adequately address your issue?
Comment 23 sassmann 2016-06-30 14:17:47 UTC
I've tested this and I don't think it's the solution we should go for and there's 2 reasons for that. As Mantas said, it lacks the preprocessor and thus for example color defines are not preprocessed. For that in my .Xresources all colors were defined as black.
The second and more important reason however is, that ~/.Xdefaults-$HOSTNAME is loaded every time any application using Xlib starts, and we don't want it to happen over and over again.
This behaviour implies that even if I do xrdb -load ~/.Xresources (with preprocessor now) colors are still black on next xterm start because the ~/.Xdefaults-$HOSTNAME (without preprocessor) will override the settings.
Comment 24 Mantas Mikulėnas (grawity) 2016-06-30 16:15:06 UTC
(In reply to sassmann from comment #23)
> I've tested this and I don't think it's the solution we should go for and
> there's 2 reasons for that. As Mantas said, it lacks the preprocessor and
> thus for example color defines are not preprocessed. For that in my
> .Xresources all colors were defined as black.

How often do you change the colors that preprocessing at load time becomes a hard requirement? As opposed to e.g. calling `cpp $(xrdb -symbols) < .Xresources.in > .Xdefaults-foo` from a Makefile.

> The second and more important reason however is, that ~/.Xdefaults-$HOSTNAME
> is loaded every time any application using Xlib starts, and we don't want it
> to happen over and over again.

Which is perhaps less of a problem nowadays than it was in 1980's when xrdb got introduced... I know plenty of programs who read .ini files or .conf files from $HOME every time they start (take ~/.gtkrc-2.0 for example), and I don't see how .Xdefaults is any different [or worse] from those.

(There's still $XAPPLRESDIR/XTerm which is constrained to a single app; same for URxvt.)

And to be honest, spawning xrdb _and cpp_ every login doesn't seem particularly fast to me either – especially during the first login, when the programs aren't cached in memory yet – and I don't want it to happen over and over again.

So, if you're concerned about load time, perhaps it's even better to do the preprocessing just once whenever the file is changed?
Comment 25 sassmann 2016-06-30 17:33:51 UTC
Hi Mantas,

I put out the colors as an example. I'm sure there will be other cases where this comes into play as well. Your cpp call most likely works and I'm not going to argue with you there but I wouldn't want to over-engineer this. Let's keep it simple as Ray suggested.

So let me ask you the other way around. Why not have mutter run xrdb *if* ~/.Xresources exists? That keeps behaviour consistent with what was done in an X session and doesn't break existing user setups and it's opt-in.

I've got to admit I haven't heard of $XAPPLRESDIR before and it's not defined on my fedora system by default. Not saying that's a bad thing, just that even with a web search I had to look twice to find information about it. For somebody who's unaware of the variable it can be a real obstacle.

Performance-wise I'd say, why should I have to parse and evaluate a config file every time an application starts when I can achieve the same thing with a single xrdb call at startup?
Comment 26 Ray Strode [halfline] 2016-08-05 14:42:32 UTC
I filed a changeset against systemd here:

https://github.com/systemd/systemd/pull/3904

that tries to solve the environment variable problem at the systemd level.
Comment 27 Hans de Goede 2016-08-08 15:10:31 UTC
Hi,

I came here because I'm having the same issue (~/.bash_profile not processed, $HOME/bin not in PATH), I was planning on also filing a related bug for /etc/X11/Xresources not being honored for X apps started under a gnome wayland session, but I see this is being discussed here too...  This feels like a somewhat orthogonal problem to me, shall I file a separate bug for this ?

Regards,

Hans
Comment 28 Ray Strode [halfline] 2016-08-08 16:28:46 UTC
yea a separate bug would be good for /etc/X11/Xresources ~/.Xresources, i think. maybe Xwayland should just load them itself at start up actually ...
Comment 29 Evan Klitzke 2016-08-21 05:41:24 UTC
(In reply to Ray Strode [halfline] from comment #17)
> you can run things at login time using desktop files in ~/.config/autostart
> or /etc/xdg/autostart.  So you could do your xrdb that way.  Although, I
> think there aren't many popular programs that rely on Xresources left. 
> Maybe you could configure emacs to run that command when emacs first starts.
> 
> We already start an ssh-agent, though, so you should be set on that front.

I would like the ability to run another ssh-agent. As far as I know the GNOME ssh keyring daemon still does not support removing keys after they've been loaded (i.e. ssh -D).

Currently I have logic to start a regular ssh-agent process and set the appropriate environment variables in my ~/.bash_profile, and there should still be a way to do this in the future.
Comment 30 Evan Klitzke 2016-08-23 18:04:28 UTC
As another point here regarding gnome-keyring, it still does not support ECDSA/ED25519 keys, which has been an issue in the GNOME bugzilla since 2011: https://bugzilla.gnome.org/show_bug.cgi?id=641082

I honestly do not see what is wrong with having gnome-session source .bash_profile. Since .bash_profile is scriptable there's no way of knowing all the ways people are using it. I think it's presumptuous to assume that all of the use cases for exporting environment variables can be shoe-horned in to some other system, e.g. xdg autostart files.

Furthermore by definition regular users are not relying on this functionality, since GNOME doesn't ship with a .bash_profile (and AFAIK distros aren't generally putting anything interesting too in the system one). The people who want gnome-session to run the code in .bash_profile are precisely the people who actually took the time to put useful code in that file; code that might be exporting environment variables in interesting and unforeseen ways.

The systemd pull request linked to in comment 26 (https://github.com/systemd/systemd/pull/3904) is interesting and useful functionality, but I don't see how it's helpful if the environment variables are dynamic (as in the case of an SSH agent that puts the agent socket at an unpredictable location). At the very least I think there needs to be a way for users to explicitly ask for a shell script to be run, even if that's disabled by default.
Comment 31 Mantas Mikulėnas (grawity) 2016-08-23 18:32:17 UTC
(In reply to Evan Klitzke from comment #30)
> As another point here regarding gnome-keyring, it still does not support
> ECDSA/ED25519 keys, which has been an issue in the GNOME bugzilla since
> 2011: https://bugzilla.gnome.org/show_bug.cgi?id=641082

You can disable gnome-keyring's ssh support, can't you?

> I honestly do not see what is wrong with having gnome-session source
> .bash_profile. Since .bash_profile is scriptable there's no way of knowing
> all the ways people are using it. I think it's presumptuous to assume that
> all of the use cases for exporting environment variables can be shoe-horned
> in to some other system, e.g. xdg autostart files.

Other than not everyone uses bash? (I realize it's mostly pedantry, but the old gdm only used ".profile" and ".xprofile".)

Well, okay, given that gnome-session is already a shellscript wrapper (just like /etc/gdm/Xsession used to be), I do wish myself that it would source something limited like ~/.environ (though certainly not a massive generic .profile script). But there /are/ ways of living without it.

> The systemd pull request linked to in comment 26
> (https://github.com/systemd/systemd/pull/3904) is interesting and useful
> functionality, but I don't see how it's helpful if the environment variables
> are dynamic (as in the case of an SSH agent that puts the agent socket at an
> unpredictable location). At the very least I think there needs to be a way
> for users to explicitly ask for a shell script to be run, even if that's
> disabled by default.

ssh-agent lets you specify a fixed socket path, so put it in $XDG_RUNTIME_DIR like many other daemons are now doing (e.g. "ExecStart=/usr/bin/ssh-agent -a %t/ssh-agent.socket" has worked fine for me for months). For that matter, gpg-agent just recently moved its sockets there as well.
Comment 32 Christian Stadelmann 2016-08-27 12:09:08 UTC
(In reply to Mantas Mikulėnas from comment #31)
> (In reply to Evan Klitzke from comment #30)
> > I honestly do not see what is wrong with having gnome-session source
> > .bash_profile. Since .bash_profile is scriptable there's no way of knowing
> > all the ways people are using it. I think it's presumptuous to assume that
> > all of the use cases for exporting environment variables can be shoe-horned
> > in to some other system, e.g. xdg autostart files.
> 
> Other than not everyone uses bash? (I realize it's mostly pedantry, but the
> old gdm only used ".profile" and ".xprofile".)

+1

There are people who don't run bash.
Furthermore, gdm or gnome-session isn't related to bash at all, so why should it process the .bash_profile file? This is just an old relict from times when people used to run ~/.xinitrc from startx, connecting two completely unrelated technologies.
Comment 33 Ray Strode [halfline] 2016-08-29 16:37:59 UTC
So one thing that came out of the discussion on the system pull request (from comment 26) is that GDM should handle importing the environment into the session via the unprivileged launcher binary (gdm-wayland-session).  Doing it from pam_systemd means doing it from a root running process, which has both logistical and security ramifications that make it less than a great idea.

I will attach a couple of patches for the import part.
Comment 34 Ray Strode [halfline] 2016-08-29 16:38:53 UTC
Created attachment 334386 [details] [review]
gdm-wayland-session: keep connection to session bus alive

This makes it behave more like gdm-x-session. Also, we're
going to need the connection in a minute.
Comment 35 Ray Strode [halfline] 2016-08-29 16:39:00 UTC
Created attachment 334387 [details] [review]
gdm-{wayland,x}-session: import environment from systemd manager

The user may have configured the user environment via
user.conf or other means.  This commit makes sure this session gets
those environment changes.
Comment 36 Ray Strode [halfline] 2016-08-31 19:54:44 UTC
Attachment 334386 [details] pushed as 15c84a4 - gdm-wayland-session: keep connection to session bus alive
Attachment 334387 [details] pushed as 448134d - gdm-{wayland,x}-session: import environment from systemd manager
Comment 37 John Longe 2016-10-12 11:35:38 UTC
This is just great. By making Wayland the default in gdm 3.22, this just broke everyone's setup. Even though this bug has been known for years.

Classic GNOME development. Functionality X is now no longer available because *bullshit reason*, in casu "Shell scripts are bad, we think. Please wait while your desktop is broken while we figure out the most abominous way to half-assedly reimplement this simple thing."

You've been discussing this for two years now while you could have just let people run their shell script.
Comment 38 Tassilo Horn 2016-10-12 16:09:15 UTC
I also just had a big "WTF, everything's broken!" moment after upgrading my distro's GNOME packages to 3.22, but at least I'm a lucky user who knew about this issue here and thus it didn't take too long for me to guess that maybe I'm running a Wayland session after logging in after the upgrade.

So I really second John: unless there's a clearly documented upgrade path for users, the switch to defaulting to wayland sessions shouldn't have happened.
Comment 39 Robert O'Callahan 2016-11-23 01:29:24 UTC
~/.profile is by far the easiest way to set up environment variables in a way that works cross-platform. https://github.com/systemd/systemd/pull/3904 isn't merged yet, may never be merged, and won't work when homedirs are shared across systems that don't use (the right version of) systemd.

Even if there were known-good alternatives to everything people use ~/.profile for, choosing to silently break everyone who's currently using ~/.profile (a lot of people I guess, given this was the recommended way to set up the shell, in all Unix variants, for an extremely long term) seems unreasonable to me.
Comment 40 Michael Catanzaro 2016-11-23 01:37:39 UTC
Ray, are you planning any further changes here? You've left the bug in ASSIGNED state.

My understanding is that we really really do not want to support these startup scripts anymore and should close this...?
Comment 41 Ray Strode [halfline] 2016-11-23 02:28:46 UTC
Not going to close this until we have a credible story...  Systemd stuff is blocked on util-linux stuff which is blocked on karel who is out for a couple of weeks.  What we do beyond that after it lands im not sure yet.
Comment 42 Thomas Kluyver 2016-11-23 14:59:27 UTC
I don't personally mind if I have to adopt some new system to specify environment variables. In fact, I'd quite like to have a more declarative specification. But I'm a bit frustrated that when I upgrade to Fedora 25 I will seemingly have *no way* to specify environment variables for my login session. :-(

I don't generally do system-level development, but is there anything I can help with in any relevant component to help resolve this?
Comment 43 Ray Strode [halfline] 2016-11-23 21:43:23 UTC
For now, your best options are to use /etc/environment or to log into gnome on Xorg and use .bashrc.  you could also use /etc/systemd/user.conf.d but the syntax is less nice.
Comment 44 Adam Williamson 2016-11-28 19:53:25 UTC
Documented as https://fedoraproject.org/wiki/Common_F25_bugs#gnome-login-shell for F25.
Comment 45 Nadav Har'El 2016-12-04 13:35:28 UTC
Wow, I was really shocked to learn of this problem: I upgraded to Fedora 25, and now all the environment variables I set in my ~/.zprofile (this problem is not limited to bash, obviously) are no longer applied to my terminal windows. So after almost 30 years (!) of being able to set environment variables in ~/.profile and similar and having them inherited, poof! The GNOME people decided that they know better. And sadly, they don't. I think The people who decided this are inexperienced, and probably didn't quite realize what they have done, or why. Or why the GUI people should not make decisions for how the shell should work.

Interestingly, what happened is not a new problem - it is a problem that first appeared over 20 years ago, and was solved as soon as it appeared. But it's just that apparently the people in charge today forgot how to solve it :-( 

Some historical context:

X windows started with people typing "xinit" *after* logging into a textual login prompt. Therefore xinit and all its children inherited the environment variables which were set by ~/.profile (and similar, depending on shell) when you logged in.

In the mid-1990s, xinit was phased out by the "xsession" - X Windows was started *before* you logged in, to produce a nice graphical login screen. So you never "logged into" the shell, and your environment variables were never read. Obviously this didn't fly, back then as today, so people's ~/.Xsession - and system default Xsession script - used to run with the user's $SHELL run as a login shell. Basically, the script that started the window manager, the terminal, and became the grandparent of all the user's processes, started by reading your ~/.profile, so all these user processes inherited all the user's variable.

In pre-Wayland GNOME, you can see for example in /etc/gdm/Xsession lines like:

            exec -l $SHELL -c gnome-session

This "exec -l $SHELL -c" is the piece of the magic you guys are missing!

It means run to run $SHELL (the user's chosen shell) with argv[0]="-$SHELL" (i.e., starting with a minus), which leads the shell to think it is a login shell and causes it read the ~/.profile. That's it! Now, gnome-session inherits these variables, and from it all child processes inherit that too.

Now, why didn't you guys do that too?
Comment 46 Nadav Har'El 2016-12-04 14:37:03 UTC
By the way, I found /usr/share/wayland-sessions/gnome.desktop which contains the "gnome-session" command.

Basically, the simplest solution to this bug would be to set this command to "$SHELL -c gnome-session", where $SHELL will need to be replaced by the user's login shell - the one configured in the /etc/passwd file through chsh, and can be retrieved with getpwuid(getuid())->pw_shell.
Comment 47 Nadav Har'El 2016-12-04 14:38:24 UTC
Sorry, I meant "$SHELL -l -c gnome-session" in the comment above. Note the very important "-l" option, which is supported by all modern shells (like bash and zsh) as an alternative to the old argv[0] monkey-business.
Comment 48 Nadav Har'El 2016-12-04 15:05:46 UTC
(In reply to Christian Stadelmann from comment #32)
> (In reply to Mantas Mikulėnas from comment #31)
> > (In reply to Evan Klitzke from comment #30)
> > > I honestly do not see what is wrong with having gnome-session source
> > > .bash_profile. Since .bash_profile is scriptable there's no way of knowing
> > > all the ways people are using it. I think it's presumptuous to assume that
> > > all of the use cases for exporting environment variables can be shoe-horned
> > > in to some other system, e.g. xdg autostart files.
> > 
> > Other than not everyone uses bash? (I realize it's mostly pedantry, but the
> > old gdm only used ".profile" and ".xprofile".)
> 
> +1
> 
> There are people who don't run bash.
> Furthermore, gdm or gnome-session isn't related to bash at all, so why
> should it process the .bash_profile file? This is just an old relict from
> times when people used to run ~/.xinitrc from startx, connecting two
> completely unrelated technologies.

Wow, a lot of misunderstandings here....

First of all, of course, gnome-session should not read specifically ".profile" or ".bash_profile". It should run the user's chosen shell - that might be bash, dash, zsh, or something I never heard of. As I explained above, all shells since 40 years ago (!) have a standard way of telling them that they are a login shell (by adding a minus in argv[0]) - and then they read whatever configuration files they want - be they ~/.profile, ~/.bash_profile, ~/.zprofile, or whatever.

Second, you are treating keeping 40-year old technology (the Unix shell) as an "old relic". But it's a "relic" that works very, very well - despite what you may think, people are still using the shell in workstation settings. They are also using it in remote non-graphical settings (e.g., ssh). And they need it to work correctly in all cases. And part of "working correctly" means that any invocation of the shell needs to be a child process of a "login shell" which read the ~/.profile or equivalent. This means either run every shell as a login shell (this is what ssh does), or run one login shell which is the parent of everything else (as I noted above, this is what we used to do in X Windows). But what will NOT work is what happens in Wayland today: There is no login-shell parent, and each shell (in a gnome-terminal) is run as a non-login shell - so nothing reads ~/.profile.

By the way, yes, we could imagine other ways to configure environment variables - like some system-wide configuration file. But it's not clear how these will apply to multi-user systems or to other login mechanisms (like ssh or non-graphical Linux login), and most importantly - it will break the setup of all the people who used Linux in the past and set environment variables in ~/.profile (or equivalent) where they were told they were supposed to set environment variables.

It is precisely because GNOME and the shell are independent technologies, that GNOME should NOT break the shell, and should NOT tell the shell users to change their ways (define environment variables elsewhere, or don't use them at all, or don't use the shell at all!).
Comment 49 Nadav Har'El 2016-12-04 15:13:28 UTC
Thanks to the insight of comment 31 (that gnome-session is already a script), I have a trivial fix for this bug!

Just replace in /usr/bin/gnome-session the line:

    exec /usr/libexec/gnome-session-binary "$@"

by

    exec -l $SHELL -c /usr/libexec/gnome-session-binary "$@"

That's all folk. That fixes this bug. And for any shell of the user's choosing, not just bash.
Comment 50 Jan Alexander Steffens (heftig) 2016-12-04 15:23:16 UTC
I disagree about importing the profile into the GUI session; that's very much shell stuff and reading it should be left to shells running in whatever terminal emulator you use. They don't even need to be POSIX-compliant, unlike the $SHELL you suggest.

What is a problem right now is that we have no way of injecting environment variables that the GUI apps actually need (such as PATH, XDG_DATA_DIRS and various hacks) that is as easily maintained with a package manager as /etc/profile.d/. IMO pam_env has to be extended to support /etc/environment.d/.
Comment 51 Nadav Har'El 2016-12-05 08:26:11 UTC
Jan, the thing you disagree with ("importing the profile into the GUI session") is what has been happening ever since X Windows was invented, 30 years ago, and even before that, in non-graphical Unix - and what many people have been depending on for years and GNOME now broke. One might argue that environment variables were never the best way to configure GUI programs, and I would agree: looking at the 30 variables I have set in my own ~/.zprofile, none of them are actually intended for GUI programs, and all of them are for command-line tools. Indeed GUI programs usually have their own configuration files and/or GUI setting menus.

But the thing is, since it's so easy to maintain the existing behavior (see my previous comment how) and have all the session's programs - including GUI programs, terminal emulators, shells and command-line tools - inherit the environment variables like they have been doing for decades - I don't understand why you're against it. What's the harm in doing that? Even if you personally don't consider it a very useful feature?

Be careful about your suggestion to replace the per-user ~/.profile (et al.) with a system-wide /etc/environment.d. You're forgetting that a lot of people are still using Linux as a multi-user system. Yes, we're no longer seeing 20 people logging into the same machine as in the heyday of UNIX. And yes, many Linux users today don't know anything about the shell and never even heard about environment variables. But still there are many people who share desktop machines (e.g., a family computer) and having separate user setups was always one of the strong sides of Linux, and we shouldn't throw this away.

Finally, I don't understand your point about a "non-POSIX" shell. The user's default shell (as chosen by chsh) doesn't need to be "POSIX". All it needs to do is to support the "-c" option to run another command. It may or may not decide to read a ~/.profile or equivalent. But you're probably right - as a failsafe, it's also easy to check that it works before doing the exec thing (e.g., test "$SHELL -c true" and see that it succeeds. perhaps something like:

$SHELL -c true && exec -l $SHELL -c exec /usr/libexec/gnome-session-binary "$@" || exec /usr/libexec/gnome-session-binary "$@"
Comment 52 Thomas Kluyver 2016-12-05 09:04:27 UTC
The proposed mechanisms for environment files does include a per-user directory for these files: ~/.config/environment.d . So it shouldn't remove the possibility of doing per-user configuration.

See this PR for more info:

https://github.com/systemd/systemd/pull/3904
Comment 53 Nadav Har'El 2016-12-05 09:17:31 UTC
Thomas, that's better than nothing, but that suggestion is not a solution to this bug because:
1. It continues to break people's existing setup (with ~/.profile et al.)
2. All the existing login mechanisms - not just GNOME but also getty, ssh, etc., will need to be modified to use these new configuration files. 
3. It prevents people from setting up environment variables using the full power of their shell - e.g., using "if", other environment variables, running programs to check things, and so on. I've been doing exactly this. A text file with "varname=value" is not always powerful enough, especially if you want the same environment file to be usable on several different machines.

I understand why you guys want to invent a new mechanism, and you can do that. But why not do that after not breaking the mechanism that already exists?
Comment 54 Carwyn Edwards 2016-12-06 14:06:01 UTC
I'll give you an of how this can affect a GUI app. Many development focused text editors and IDEs make use of development languages specific environment variables to find libraries and tools. Often these are in user environments not system wide.

The Atom editor plugins for Golang (like many others I'm guessing) makes use of the GOPATH env to locate pretty much everything written in Go. Under Xorg I can launch Atom from Gnome and it all works. Under wayland I've had to force gnome-shells to be login shells and launch atom from the shell.
Comment 55 James Patterson 2016-12-10 19:11:44 UTC
Please could you unbreak this, then we can discuss ways to do this differently afterwards?
Comment 56 Ray Strode [halfline] 2016-12-14 15:39:01 UTC
(In reply to James Patterson from comment #55)
> Please could you unbreak this, then we can discuss ways to do this
> differently afterwards?

Yea, I'm considering caving on this. It's taking a lot longer to get the new architecture through than I thought it would, and it's clearly causing immediate issues.  I think I'm going to bring back the login shell, and maybe lobby for /etc/profile.d scripts to get broken up into two directories one for interactive shells and one for non-interactive shells.

We can still work on the non-shell way of changing the environment, and migrate away from relying on bash piecemeal.
Comment 57 Igor Bukanov 2016-12-15 09:02:40 UTC
(In reply to Nadav Har'El from comment #49)
> Thanks to the insight of comment 31 (that gnome-session is already a
> script), I have a trivial fix for this bug!
> 
> Just replace in /usr/bin/gnome-session the line:
> 
>     exec /usr/libexec/gnome-session-binary "$@"
> 
> by
> 
>     exec -l $SHELL -c /usr/libexec/gnome-session-binary "$@"
> 
> That's all folk. That fixes this bug. And for any shell of the user's
> choosing, not just bash.

That only works if /bin/sh that runs gome-session script supports exec -l (dash does not) and if gnome-session was called without arguments so "$@" expands to nothing. Plus it keeps useless bash login process. To fix this I replaced the last exec command by:

exec "$SHELL" -l -c 'exec "$0" "$@"' /usr/libexec/gnome-session-binary "$@"
Comment 58 Igor Bukanov 2016-12-22 08:30:29 UTC
(In reply to Igor Bukanov from comment #57)
> exec "$SHELL" -l -c 'exec "$0" "$@"' /usr/libexec/gnome-session-binary "$@"

It turned out on Fedora 25 that does not work either. gnome-session is run apparently to show the login screen when $SHELL is /sbin/nologin causing GDM to exit. So the proper fix is to replace the last line in /usr/bin/gnome-session with:

test /sbin/nologin = "$SHELL" && exec /usr/libexec/gnome-session-binary "$@"
exec "$SHELL" -l -c 'exec "$0" "$@"' /usr/libexec/gnome-session-binary "$@"

Alternatively, if the shell for the user is /bin/bash then the simplest and fastest fix is to change the first line of /usr/bin/gnome-session from #!/bin/sh to:

#!/bin/bash -l
Comment 59 kgizdov 2016-12-22 14:48:12 UTC
I can confirm changing #!/bin/sh to #!/bin/zsh -l in the beginning of the scripts works for me on Arch Linux. Obviously, I am running ZSH as my user shell.
Comment 60 Evan Klitzke 2016-12-27 19:38:49 UTC
Here's another interesting use case for running shell logic at login time: http://askubuntu.com/a/228278

In this case the solution isn't trying to set an environment variable at login time, but instead relies on being able to run pactl (in this answer, by putting the logic in ~/.xprofile) to put a sound sample in the pulse audio server.
Comment 61 Olav Vitters 2016-12-27 20:41:57 UTC
(In reply to Evan Klitzke from comment #60)
> In this case the solution isn't trying to set an environment variable at
> login time, but instead relies on being able to run pactl (in this answer,
> by putting the logic in ~/.xprofile) to put a sound sample in the pulse
> audio server.

For running things at startup you don't need a shell. See desktop autostart specification and /etc/xdg/autostart. https://specifications.freedesktop.org/autostart-spec/autostart-spec-latest.html
Comment 62 Ray Strode [halfline] 2017-01-05 15:33:26 UTC
Created attachment 342968 [details] [review]
gnome-session: make sure wayland sessions get a login shell

Users expect their shell profiles to get sourced at startup, which
doesn't happen with wayland sessions.

This commit brings back that feature, by making the gnome-shell
wrapper script run a login shell.
Comment 63 Ray Strode [halfline] 2017-01-05 20:44:15 UTC
Comment on attachment 342968 [details] [review]
gnome-session: make sure wayland sessions get a login shell

https://git.gnome.org/browse/gnome-session/commit/?id=5e36cd63e71520f829bbd77c102b64179e08992a
Comment 64 Mantas Mikulėnas (grawity) 2017-01-06 06:56:22 UTC
(In reply to Ray Strode [halfline] from comment #62)
> Created attachment 342968 [details] [review] [review]
> gnome-session: make sure wayland sessions get a login shell
> 
> Users expect their shell profiles to get sourced at startup, which
> doesn't happen with wayland sessions.
> 
> This commit brings back that feature, by making the gnome-shell
> wrapper script run a login shell.

This is invalid shell syntax, though. Sh's '[' acts like a command, not a paren, so you need a \ before line breaks; otherwise it's only going to complain about three incomplete commands before falling back to the 'else'.

Additionally, `exec -l` is a bashism – it won't work on Debian with dash as /bin/sh, for example.

(I'm also not sure why only the dbus- command is run via login shell. Shouldn't gnome-session/-shell itself obtain the same environment variables as well?)

How about something like:

cmd="dbus-update-activation-environment --all --systemd > /dev/null 2>&1"
cmd="$cmd; exec @libexecdir@/gnome-session-binary $*"

if [ "$XDG_SESSION_TYPE" = "wayland" ] &&
   [ "$XDG_SESSION_CLASS" != "greeter" ] &&
   [ -n "$SHELL" ]; then
  exec $SHELL -l -c "$cmd" || eval "$cmd"
else
  eval "$cmd"
fi

(This assumes "$@" won't ever have spaces in args, which is /probably/ safe for gnome-session.)
Comment 65 Ray Strode [halfline] 2017-01-06 14:50:27 UTC
Hi,

Thanks for the review!

(In reply to Mantas Mikulėnas (grawity) from comment #64)
> This is invalid shell syntax, though. Sh's '[' acts like a command, not a
> paren, so you need a \ before line breaks; otherwise it's only going to
> complain about three incomplete commands before falling back to the 'else'.
With bash '[' is a builtin that does behave like a parenthesis.  But you're right the top of the shell script does say /bin/sh not /bin/bash, so this could cause problems on debian.

> Additionally, `exec -l` is a bashism – it won't work on Debian with dash as
> /bin/sh, for example.
Well it's actually not a bashism, exec -l sets the first character of argv[0] to - which is a widely used convention to mean "run as login shell".  It's  a little strange that dash doesn't support that convention, but, granted, it is weird.  Also, assuming a shell supports -l as an argument that means "login shell" seems about as arbitrary as assuming a shell supports the argv[0][0] == '-' convention.

On the other hand, we're already assuming the shell supports -c, so we already rely on the shell accepting certain command line arguments.  And making the change will fix dash, so changing it seems fine to me, I guess.  I don't think many people use dash as their interactive shell on their workstations though?

> (I'm also not sure why only the dbus- command is run via login shell.
> Shouldn't gnome-session/-shell itself obtain the same environment variables
> as well?)
yea, not sure what I was thinking there, this is clearly a problem.
Comment 66 Ray Strode [halfline] 2017-01-06 14:53:08 UTC
> but, granted, it is weird.

to be clear, I mean the convention, not dash. (though does anyone else find it amusing that a shell called "dash" doesn't support a shell convention involving a dash ?)
Comment 67 Mantas Mikulėnas (grawity) 2017-01-06 15:36:58 UTC
(In reply to Ray Strode [halfline] from comment #65)
> Hi,
> 
> Thanks for the review!
> 
> (In reply to Mantas Mikulėnas (grawity) from comment #64)
> > This is invalid shell syntax, though. Sh's '[' acts like a command, not a
> > paren, so you need a \ before line breaks; otherwise it's only going to
> > complain about three incomplete commands before falling back to the 'else'.
> With bash '[' is a builtin that does behave like a parenthesis.  But you're

No it doesn't; it's `[[` that does. (Even though `[` *is* a builtin in nearly all shells, its syntax is still that of a command.)

$ if [ foo = bar -a
... baz = qux ]; then
... echo Success
... fi
-bash: [: missing `]'
-bash: baz: command not found

> right the top of the shell script does say /bin/sh not /bin/bash, so this
> could cause problems on debian.
> 
> > Additionally, `exec -l` is a bashism – it won't work on Debian with dash as
> > /bin/sh, for example.
> Well it's actually not a bashism, exec -l sets the first character of
> argv[0] to - which is a widely used convention to mean "run as login shell".

The '-' convention isn't a bashism (indeed it has existed since the early days), meanwhile `exec -l` *is* bash-specific -- it's present in zsh but not dash nor mksh.

> It's  a little strange that dash doesn't support that convention, but,
> granted, it is weird.  Also, assuming a shell supports -l as an argument
> that means "login shell" seems about as arbitrary as assuming a shell
> supports the argv[0][0] == '-' convention.
> 
> On the other hand, we're already assuming the shell supports -c, so we
> already rely on the shell accepting certain command line arguments.  And
> making the change will fix dash, so changing it seems fine to me, I guess. 
> I don't think many people use dash as their interactive shell on their
> workstations though?

All interactive shells support -c, don't they? It's already required for such things as `ssh <host> <command>`, after all.

But as for -l, you're right that it's not as well supported as dash0; at least csh/tcsh support the latter but not the former.

So I'm not sure what to use instead. Maybe just depend on bash?
Comment 68 Simon McVittie 2017-01-06 15:37:55 UTC
(In reply to Ray Strode [halfline] from comment #65)
> With bash '[' is a builtin that does behave like a parenthesis.  But you're
> right the top of the shell script does say /bin/sh not /bin/bash, so this
> could cause problems on debian.

If you are going to rely on bash features, please make it a #!/bin/bash script.

(Debian Policy does define a few non-POSIX extensions that must also be supported in Debian systems' /bin/sh - <https://www.debian.org/doc/debian-policy/ch-files.html#s-scripts> - and I suspect that in practice all relevant Linux distributions use either bash or dash by default; but POSIX is the best formal specification we have for what can be relied on in a shell script.)

> Well it's actually not a bashism, exec -l sets the first character of
> argv[0] to - which is a widely used convention to mean "run as login shell".

The fact that the exec builtin takes option arguments at all is a bashism. POSIX only describes "exec [command [argument...]]", and that's all dash has.

> It's  a little strange that dash doesn't support that convention, but,
> granted, it is weird.  Also, assuming a shell supports -l as an argument
> that means "login shell" seems about as arbitrary as assuming a shell
> supports the argv[0][0] == '-' convention.

A shell that doesn't support running differently as a login shell likely just won't look at argv[0] at all, so it won't execute login-only scripts but will still work (graceful degradation). A shell that doesn't have a -l option will likely just fail, recognising it as an unsupported option.

As it happens, dash does support -l with the same meaning as bash, but AIUI it would be valid for a user to have an interactive shell that doesn't.

> On the other hand, we're already assuming the shell supports -c, so we
> already rely on the shell accepting certain command line arguments.

POSIX defines the meaning of sh -c, but does not define the meaning of sh -l.

(I don't think a user's interactive shell is guaranteed to be a POSIX shell, though - csh is non-POSIX and so cannot be /bin/sh, but some people still use it as their login shell.)
Comment 69 Ray Strode [halfline] 2017-01-06 16:22:03 UTC
hmm there's another problem:

╎rstrode@halflap〉⎛/home/rstrode⎞ ⌜11:20 AM⌟
╎❯ dbus-update-activation-environment --all --systemd
dbus-update-activation-environment: warning: error sending to systemd: org.freedesktop.DBus.Error.InvalidArgs: Invalid environment assignments

╎rstrode@halflap〉⎛/home/rstrode⎞ ⌜11:20 AM⌟
╎❯ systemctl --user import-environment
Failed to import environment: Invalid environment assignments
Comment 70 Ting-Wei Lan 2017-01-06 16:32:05 UTC
(In reply to Simon McVittie from comment #68)
> (In reply to Ray Strode [halfline] from comment #65)
> > With bash '[' is a builtin that does behave like a parenthesis.  But you're
> > right the top of the shell script does say /bin/sh not /bin/bash, so this
> > could cause problems on debian.
> 
> If you are going to rely on bash features, please make it a #!/bin/bash
> script.

Please don't use #!/bin/bash in bash scripts because some systems don't install bash in /bin. You can check the path of bash in configure script if you need it. I remember that both FreeBSD and OpenBSD use /usr/local/bin/bash.

> > Well it's actually not a bashism, exec -l sets the first character of
> > argv[0] to - which is a widely used convention to mean "run as login shell".
> 
> The fact that the exec builtin takes option arguments at all is a bashism.
> POSIX only describes "exec [command [argument...]]", and that's all dash has.
> 
> > It's  a little strange that dash doesn't support that convention, but,
> > granted, it is weird.  Also, assuming a shell supports -l as an argument
> > that means "login shell" seems about as arbitrary as assuming a shell
> > supports the argv[0][0] == '-' convention.
> 
> A shell that doesn't support running differently as a login shell likely
> just won't look at argv[0] at all, so it won't execute login-only scripts
> but will still work (graceful degradation). A shell that doesn't have a -l
> option will likely just fail, recognising it as an unsupported option.
> 
> As it happens, dash does support -l with the same meaning as bash, but AIUI
> it would be valid for a user to have an interactive shell that doesn't.

/bin/sh on FreeBSD supports neither 'sh -l' nor 'exec -l'.

$ sh -l 
Illegal option -l

$ exec -l
exec: -l: not found
Comment 71 Ray Strode [halfline] 2017-01-06 19:08:28 UTC
(In reply to Ting-Wei Lan from comment #70)
> (In reply to Simon McVittie from comment #68)
> > (In reply to Ray Strode [halfline] from comment #65)
> > > With bash '[' is a builtin that does behave like a parenthesis.  But you're
> > > right the top of the shell script does say /bin/sh not /bin/bash, so this
> > > could cause problems on debian.
> > 
> > If you are going to rely on bash features, please make it a #!/bin/bash
> > script.
> 
> Please don't use #!/bin/bash in bash scripts because some systems don't
> install bash in /bin. You can check the path of bash in configure script if
> you need it. I remember that both FreeBSD and OpenBSD use
> /usr/local/bin/bash.

well the exec -l is in subshell anyway, so we could just write bash instead of using ( ) and rely on PATH...

> /bin/sh on FreeBSD supports neither 'sh -l' nor 'exec -l'.
I guess it doesn't really matter though. freebsd doesn't support wayland sessions and this only runs for wayland sessions.
Comment 72 Ray Strode [halfline] 2017-01-09 18:57:36 UTC
okay i'm going to attach a few patches:

1) a patch to revert attachment 342968 [details] [review]
2) a patch to change how we do dbus-update-activation-environment to address comment 69
3) a patch that tries to address the other comments brought up.  In particular, I try to remove all bashisms from the code.  I do keep the exec -l but run it explicitly under bash.

One thing I don't handle is spaces in arguments.  I did have some changes to make that work, but it was really messy (especially given the inability to rely on bash), and not needed.  This script is already getting messy enough, anyway.
Comment 73 Ray Strode [halfline] 2017-01-09 18:58:59 UTC
Created attachment 343185 [details] [review]
Revert "gnome-session: make sure wayland sessions get a login shell"

This reverts commit 5e36cd63e71520f829bbd77c102b64179e08992a.
Comment 74 Ray Strode [halfline] 2017-01-09 18:59:07 UTC
Created attachment 343187 [details] [review]
gnome-session: change how environment is imported

dbus-update-activation-environment --all doesn't work if any
environment variable has newlines in its values.

This commit adds the environment variables peicemeal so a few
bad apples don't ruin the bunch.
Comment 75 Ray Strode [halfline] 2017-01-09 18:59:16 UTC
Created attachment 343188 [details] [review]
gnome-session: make sure wayland sessions get a login shell

Users expect their shell profiles to get sourced at startup, which
doesn't happen with wayland sessions.

This commit brings back that feature, by making the gnome-session
wrapper script run a login shell.
Comment 76 Simon McVittie 2017-01-09 21:34:58 UTC
(In reply to Ray Strode [halfline] from comment #74)
> dbus-update-activation-environment --all doesn't work if any
> environment variable has newlines in its values.

Huh, it doesn't? I thought I'd been careful to avoid shell-style badness.

On closer inspection, this appears to be systemd putting arbitrary constraints on the names and values of environment variables:

As far as I'm aware, POSIX says environment variables' names are arbitrary bytestrings other than '\0' and '=', and their values are arbitrary bytestrings other than '\0'. For example, this works, and incidentally puts a carriage return and a line feed in an environment variable *name*:

zsh% env - $'\x02\x13\x10=\x01' env | xxd
00000000: 0213 103d 010a                           ...=..

The D-Bus APIs we're using restrict this to only allowing UTF-8 (this is the best we can do without using byte-arrays 'ay' instead of strings 's'). dbus-daemon doesn't apply any further restrictions.

Unfortunately, systemd also explicitly rejects environment variable names not matching /[A-Za-z_][A-Za-z0-9]+/, and environment variable values containing control characters (\x01-\x1f, \x7f) other than tab:

        /* bash allows tabs in environment variables, and so should
         * we */
        if (string_has_cc(e, "\t"))
                return false;

I think this is really a systemd bug, but if the systemd developers are happy with it the way it is, dbus-update-activation-environment and dbus-daemon seem a better place to do the filtering.
Comment 77 Simon McVittie 2017-01-09 21:47:53 UTC
(In reply to Simon McVittie from comment #76)
> For example, this works, and incidentally puts
> a carriage return and a line feed in an environment variable *name*:
> 
> zsh% env - $'\x02\x13\x10=\x01' env | xxd
> 00000000: 0213 103d 010a                           ...=..

which works even if I get the ASCII codes right for what I'd intended:

$ env - $'\x02\x0d\x0a=\x01' env | xxd
00000000: 020d 0a3d 010a                           ...=..
Comment 78 Simon McVittie 2017-01-09 22:13:49 UTC
> env | sed -n -e 's/\(^[^() =]*\)=\(.*\)/\1/p' | while read line; do dbus-update-activation-environment --systemd "$line"  > /dev/null 2>&1; done

Anything starting with "env |" is certainly not the answer - if an environment variable contains newlines, substrings of its value that happen to look like environment variables are going to end up in the environment passed to systemd.

I think this is quite a good illustration of why the long term path should be to reduce the number of shell scripts floating around - it's really hard to write shell scripts to be fully general.

We are going to need a helper executable that manipulates the environment from C: either a suitably modified dbus-update-activation-environment, or something provided by systemd, or systemd losing those restrictions.

https://github.com/systemd/systemd/issues/3729 is the relevant systemd bug.
Comment 79 Jan Alexander Steffens (heftig) 2017-01-10 01:18:12 UTC
Also, as noted in https://bugzilla.gnome.org/show_bug.cgi?id=769094, pushing env vars (or causing other side effects) when gnome-session --version or --help is executed is rather bad.
Comment 80 Michael Catanzaro 2017-01-10 02:14:49 UTC
(In reply to Simon McVittie from comment #78)
> I think this is quite a good illustration of why the long term path should
> be to reduce the number of shell scripts floating around - it's really hard
> to write shell scripts to be fully general.

Which brings us back to why this bug was left open for so long: the desire to reduce the number of shell scripts involved in session startup. We're going to be stuck with running the session in a shell forever if we do this now.

(In reply to Jan Alexander Steffens (heftig) from comment #79)
> Also, as noted in https://bugzilla.gnome.org/show_bug.cgi?id=769094, pushing
> env vars (or causing other side effects) when gnome-session --version or
> --help is executed is rather bad.

Yeah good point, that's probably one of my favorite bugs ever. Mention it at parties. At the very least the script ought to grow some straightforward check to avoid doing that.
Comment 81 Ray Strode [halfline] 2017-01-10 03:06:27 UTC
(In reply to Simon McVittie from comment #78)
> if an
> environment variable contains newlines, substrings of its value that happen
> to look like environment variables are going to end up in the environment
> passed to systemd.
No that's not true.  We're not passing environment *values*, we're passing just the keys.  The values are looked up in C code (dbus-update-activation-environment).  Of course, there is the possibility that a value will look like a key, but that's harmless, since we're trying to upload all variables anyway (worst cases are that we'd pass a key that doesn't exist in the environment or a key we already migrated. both case are harmless)
 
> the long term path should
> be to reduce the number of shell scripts floating around - it's really hard
> to write shell scripts to be fully general.
This I totally agree with.  The PID explosion from running dbus-update-activation-environment bugs me, too.  Probably I should just finish up https://git.gnome.org/browse/gnome-session/commit/?h=wip/halfline/env-to-systemd&id=415230016aa7e038a48994b5c0248de7569af23f

Of course we can't get rid of the shell script entirely, since its original purpose had nothing to do with systemd environment, but was about translating control-center locale settings to environment variables, and that has to be done before gnome-session is started since setenv() isn't thread safe. unless we moved that off to a separate C program too.
Comment 82 Ray Strode [halfline] 2017-01-10 03:26:27 UTC
Hi,
> Which brings us back to why this bug was left open for so long: the desire
> to reduce the number of shell scripts involved in session startup. We're
> going to be stuck with running the session in a shell forever if we do this
> now.
I think the point is we're sort of already stuck running the session in a shell indefinitely.  Originally we said "well, wayland gives us an opportunity to free ourselves from the login shell shackles" or whatever, and that was fine (enough) as long as wayland wasn't the default.  But now wayland is the default and so people are expecting backward compatibility.  It should be clear now, that there's no getting away from the shackles without breaking some configurations and alienating some people. Everyone is expecting the transition to be as smooth and seamless as possible. There are 77 users on the CC list of this bug, after all… 

Some of those 77 users would be fine, I guess, to use the new config format, but that hasn't landed in the mean time! So we broke the old way and didn't provide any good recourse.

I do want to land the new config format, and get users to start using it.  Then maybe, at some point, when the new way hits critical mass we can take the login shell back out by default (and provide a way to optionally turn it back on)
Comment 83 Mantas Mikulėnas (grawity) 2017-01-10 06:42:46 UTC
Ah, I thought my approach was already bad, but this is getting a bit crazy...

But now I'm wondering, does it really *need* to be the user's login shell? Back in X11, /etc/gdm/Xsession used to just directly source two 'profile' scripts and that was quite enough, if not even better. (After all, most users just want the ability to set environment shell-like.) So how about replacing this with:

#!/bin/sh
case $1 in
  -h|--help|--version)
    ;;
  *)
    if [ -f /etc/profile ]; then
      . /etc/profile
    fi
    if [ -f ~/.profile ]; then
      . ~/.profile
    fi
    <locale stuff>
    dbus-update-activation-environment --all > /dev/null 2>&1
    ;;
esac
exec /usr/lib/gnome-session/gnome-session-binary "$@"

(Side note: Do any shells still require the x$FOO workaround these days?)
Comment 84 Ray Strode [halfline] 2017-01-10 06:52:22 UTC
Thats goina fall over if the user has zsh (or whatever) extensions in .profile.  it also will miss .bash_profile.  Of course it also doesnt address the newline in PS1 (or whatever) issue.
Comment 85 Igor Bukanov 2017-01-10 06:53:33 UTC
Just sourcing /etc/profile and ~/.profile is a good way to address this. I have seen a few packages (epecialy in comersial settings) that just add their variables using /etc/profile.d/ scripts expecting those to be sourced no matter what the user shell is. I guess the original practice of Xsession made it work.
Comment 86 Mantas Mikulėnas (grawity) 2017-01-10 07:07:41 UTC
(In reply to Ray Strode [halfline] from comment #84)
> Thats goina fall over if the user has zsh (or whatever) extensions in
> .profile.  it also will miss .bash_profile.

That wasn't a problem earlier, so it shouldn't suddenly become a problem now.

After all, most users come here because they lost the functionality they had in X11, so I'm going to assume a) they already had a sh-safe ~/.profile with all necessary customizations, or b) they only needed the system-wide /etc/profile.

> Of course it also doesnt
> address the newline in PS1 (or whatever) issue.

If Lennart cannot be convinced otherwise, I think that should be filtered by dbus-update-env instead.
Comment 87 Ray Strode [halfline] 2017-01-10 07:31:28 UTC
The Xsession file used in Fedora (and probably other distros) doesnt just source those files. Granted the sample one shipped with gdm does. But some Xsession files do the whole exec -l dance.  Doing less will leave some users broken. Tcsh users were mentiones above.  They'd be broken.  Users that use .bash_profile instead of .profile too.

I think dbus-update-activation-environment should filter, but we should do it from gnome-session too so we can backport a fix eitjout forcing users to update dbus.  Ill just stop using dbus-update-activation-environment entirely ala the branch i mention in comment 81
Comment 88 Mantas Mikulėnas (grawity) 2017-01-10 07:53:42 UTC
> But some Xsession files do the whole exec -l dance.  Doing less will leave some users broken.

Wasn't the long-term plan to get rid of shell anyway, leaving all users broken until they port to the new config?

[In that case, I'd say this is a good chance for them to cleanly split env exports (in .profile) from non-env scripts like welcome messages and stuff (in .bash_profile)]

Either way, it seems that the main problem wasn't that this functionality suddenly needed some user intervention to get it working again – the problem was that this functionality had /disappeared outright/. If it returns even in a basic form, I think that'll satisfy most people.

> Tcsh users were mentiones above.  They'd be broken.

csh & tcsh don't accept -l and -c at the same time, so they'd be broken anyway...
Comment 89 Antonin 2017-01-10 08:48:33 UTC
> [In that case, I'd say this is a good chance for them to cleanly split env
> exports (in .profile) from non-env scripts like welcome messages and stuff
> (in .bash_profile)]

What about an "umask" command?
It won't be exported as environment value.
Comment 90 Mantas Mikulėnas (grawity) 2017-01-10 09:00:26 UTC
(In reply to Antonin from comment #89)
> > [In that case, I'd say this is a good chance for them to cleanly split env
> > exports (in .profile) from non-env scripts like welcome messages and stuff
> > (in .bash_profile)]
> 
> What about an "umask" command?
> It won't be exported as environment value.

Hmm, you could fit that under the broader sense of 'environment' and keep in .profile, but it's usually forgotten whenever env configuration is discussed, so it might be better to use pam_umask(8) for this instead. (That way it'll also work with cronjobs and stuff!)
Comment 91 Nadav Har'El 2017-01-10 09:02:10 UTC
Igor and Mantas, you are misunderstanding the pre-existing situation, as it existed for almost two decades (!) now on all systems I had access to. The default Xsession files in any reasonable Linux distribution or Unix setup *did* run a login shell, and did not try to run ~/.profile which only works for certain shells.

Indeed, as Ray pointed out, Fedora has this in /etc/gdm/Xsession:

     exec -l $SHELL -c gnome-session
     exec /bin/sh -c "exec -l $SHELL -c \"gnome-session\"" 

(I'm not sure why they have both, by the way - could it be that bash doesn't support exec -l but /bin/sh does?)

By the way, about the argument about some shells not supporting "exec -l": Well, we don't need all shells to support it - we use "exec -l" in the script whose shell we chose - not in the user's chosen shell. It is enough that the system has one shell which supports "exec -l" and we can use that. E.g., Fedora uses "#!/bin/bash" explicitly in the Xsession file. On a system that doesn't have a shell which supports "exec -l", I think the best approach will be to add a new external command which does it, i.e., let you run a command line while controlling argv[0].
Comment 92 Edgar Hoch 2017-01-10 09:41:11 UTC
(In reply to Mantas Mikulėnas (grawity) from comment #90)
> (In reply to Antonin from comment #89)
> > What about an "umask" command?
> > It won't be exported as environment value.
> 
> Hmm, you could fit that under the broader sense of 'environment' and keep in
> .profile, but it's usually forgotten whenever env configuration is
> discussed, so it might be better to use pam_umask(8) for this instead. (That
> way it'll also work with cronjobs and stuff!)

This does not work. A user can - if I know it right - not set his own umask as default for the running processes using pam_umask(8). But a user can set the default umask for all child processes by setting it in the init files for his login shell (which file this is depends on the login shell of the user).
Comment 93 Edgar Hoch 2017-01-10 09:48:43 UTC
(In reply to Nadav Har'El from comment #91)
> Indeed, as Ray pointed out, Fedora has this in /etc/gdm/Xsession:
> 
>      exec -l $SHELL -c gnome-session
>      exec /bin/sh -c "exec -l $SHELL -c \"gnome-session\"" 
> 
> (I'm not sure why they have both, by the way - could it be that bash doesn't
> support exec -l but /bin/sh does?)
> 
> By the way, about the argument about some shells not supporting "exec -l":
> Well, we don't need all shells to support it - we use "exec -l" in the
> script whose shell we chose - not in the user's chosen shell. It is enough
> that the system has one shell which supports "exec -l" and we can use that.
> E.g., Fedora uses "#!/bin/bash" explicitly in the Xsession file. On a system
> that doesn't have a shell which supports "exec -l", I think the best
> approach will be to add a new external command which does it, i.e., let you
> run a command line while controlling argv[0].

"#!/bin/bash" only defines the shell (the interpreter!) to use to run THIS shell script. It does not set or change the environment variable $SHELL. $SHELL is set by the system from the user definition of its login shell.

The script must call the login shell of the user, not any shell (e.g. bash), because the user write its initialisation parameters for kind processes (like environment variables, umask, limits, ...) in the specific configuration files of its login shell.
Comment 94 Simon McVittie 2017-01-10 10:01:10 UTC
(In reply to Ray Strode [halfline] from comment #87)
> I think dbus-update-activation-environment should filter

Happy to propose or review a patch when I'm back at the office next week, but to try to preserve my remaining sanity I'm avoiding work projects while not at work :-)

(In reply to Nadav Har'El from comment #91)
> The
> default Xsession files in any reasonable Linux distribution or Unix setup
> *did* run a login shell

Are Debian and Ubuntu unreasonable, then? They don't do anything with login shells for X11, and as far as I'm aware they never have.

(They do source ~/.xsessionrc and the contents of /etc/X11/Xsession.d, from a #!/bin/sh script; and if ~/.xsession exists, the user can put whatever they want there and select it to be exec'd instead of gnome-session or startkde or whatever.)
Comment 95 Ray Strode [halfline] 2017-01-10 16:51:07 UTC
Comment on attachment 343185 [details] [review]
Revert "gnome-session: make sure wayland sessions get a login shell"

Attachment 343185 [details] pushed as 1930c63 - Revert "gnome-session: make sure wayland sessions get a login shell"
Comment 96 Ray Strode [halfline] 2017-01-10 20:04:42 UTC
(In reply to Mantas Mikulėnas (grawity) from comment #88)
> csh & tcsh don't accept -l and -c at the same time, so they'd be broken
> anyway...

But we aren't passing -l to the shell… we're using exec -l

╎rstrode@halflap〉⎛/home/rstrode⎞ ⌜02:53 PM⌟
╎❯ echo 'echo login shell!' > ~/.login

╎rstrode@halflap〉⎛/home/rstrode⎞ ⌜02:53 PM⌟
╎❯ (exec -l tcsh -c 'echo passed in command!')
login shell!
passed in command!

I think my plan at this point is:

1) to ditch attachment 343187 [details] [review] and do the equivalent thing in gnome-session's C source (ala https://git.gnome.org/browse/gnome-session/commit/?h=wip/halfline/env-to-systemd&id=415230016aa7e038a48994b5c0248de7569af23f ) 

2) push attachment 343188 [details] [review] near term. 

3) At some point down the road, i'll write a small loader program that does the locale conversion and login shell invocation itself without bringing in sh/bash. 

4) longer term we'll phase out the login shell with some way to turn it back on for people who need it for backward compat.

Of course some of this is subject to change depending on how we move session services to systemd --user.
Comment 97 Igor Bukanov 2017-01-10 20:06:12 UTC
As alternative to running a login shell what about providing a utility that runs a helper process through a login shell to dump environment , umask, ulimit etc. settings that are inheritable across processes to a file in some format and then read that file when running the session? A user can run such utility either manually or it can be run automatically the first time the wayland session runs.
Comment 98 Ray Strode [halfline] 2017-01-13 15:45:28 UTC
Igor, well at this point we're just trying to mop up the regression.  a new config file format is what https://github.com/systemd/systemd/pull/3904 is about.
Comment 99 Ray Strode [halfline] 2017-01-13 15:52:05 UTC
Created attachment 343439 [details] [review]
gnome-session: update activation environment from C code, instead of shell script

dbus-update-activation-environment excepts certain environment
variables, that systemd won't.  We're going to want to eventually send
the environment to systemd, too, so we shouldn't make sure the same set
of variables get sent to both.

This commit takes the dbus-update-activation-environment call out of the
gnome-session shell script wrapper, and instead changes the C code to do
the export explicitly.
Comment 100 Ray Strode [halfline] 2017-01-13 15:52:13 UTC
Created attachment 343440 [details] [review]
gsm-util: export environment to systemd

If we get passed an environment variable, send it along to the
systemd --user session so things running in that context can pick it
up.
Comment 101 Ray Strode [halfline] 2017-01-13 15:52:22 UTC
Created attachment 343441 [details] [review]
gnome-session: make sure wayland sessions get a login shell

Users expect their shell profiles to get sourced at startup, which
doesn't happen with wayland sessions.

This commit brings back that feature, by making the gnome-session
wrapper script run a login shell.
Comment 102 Ray Strode [halfline] 2017-01-13 15:57:47 UTC
so i'm probably going to commit this and cut a release later today unless anyone can find any objective problems with them (typos, leaks, whatever). I realize attachment 343441 [details] [review] isn't going to win any beauty contests, but it's probably what I'm going to go with.
Comment 103 Ray Strode [halfline] 2017-01-16 14:27:30 UTC
Attachment 343439 [details] pushed as 52a3c15 - gnome-session: update activation environment from C code, instead of shell script
Attachment 343440 [details] pushed as 0af8ef8 - gsm-util: export environment to systemd
Attachment 343441 [details] pushed as 7e307f8 - gnome-session: make sure wayland sessions get a login shell
Comment 104 Adam Williamson 2017-01-16 17:51:18 UTC
Should we backport this to Fedora 25?
Comment 105 Ray Strode [halfline] 2017-01-16 17:57:15 UTC
yea I did shortly after I pushed here...

https://bodhi.fedoraproject.org/updates/FEDORA-2017-84b0233854
Comment 106 Adam Williamson 2017-01-16 18:01:11 UTC
ah, great! thanks.
Comment 107 Jeremy Bicha 2017-01-16 22:38:04 UTC
As long as you're pushing to Fedora, is there a reason you don't push this to at least the gnome-3-22 branch too? Thanks!
Comment 108 Yasushi SHOJI 2017-02-01 04:38:26 UTC
Comment on attachment 343441 [details] [review]
gnome-session: make sure wayland sessions get a login shell

what was the reason to use "exec bash -c ..." instead of "exec sh -c ..."?
Comment 109 Simon McVittie 2017-02-01 08:37:19 UTC
(In reply to Yasushi SHOJI from comment #108)
> what was the reason to use "exec bash -c ..." instead of "exec sh -c ..."?

The exec builtin in generic /bin/sh is not guaranteed to support the -l option, or any options, in a way that is compatible with bash. /bin/sh on Debian and its derivatives is dash, which cuts out unnecessary features for better speed and memory footprint (which was particularly important before those distributions defaulted to systemd as init, because the critical path for boot used to include a lot of #!/bin/sh scripts).

% dash
$ exec -l bash
dash: 1: exec: -l: not found
Comment 110 Jonas Hahnfeld 2017-02-07 20:09:24 UTC
The patch does not work on ArchLinux where bash is compiled without NON_INTERACTIVE_LOGIN_SHELLS. Can -i be added before '-c'?
Comment 111 Jan Alexander Steffens (heftig) 2017-02-07 20:13:07 UTC
(In reply to Jonas Hahnfeld from comment #110)
Please file a bug against Arch Linux's bash instead.
Comment 112 Jonas Hahnfeld 2017-02-08 06:48:47 UTC
(In reply to Jan Alexander Steffens (heftig) from comment #111)
> (In reply to Jonas Hahnfeld from comment #110)
> Please file a bug against Arch Linux's bash instead.

No, that's actually the default behaviour of bash since bash-2.04; Debian, Fedora and Ubuntu are patching the source code during build. From CHANGES of bash-2.04:
> e.  Restored the undocumented NON_INTERACTIVE_LOGIN_SHELLS #define to
>     config-top.h.  If this is defined, all login shells will read the
>     startup files, not just interactive and non-interactive started with
>     the `--login' option.

Two options here I think:
 - exec -l $SHELL -i -c '...' which is sh-compliant
 - exec $SHELL -l -c '...' which is not
Comment 113 Jan Alexander Steffens (heftig) 2017-02-08 07:32:23 UTC
I've also tried zsh, fish, dash and busybox ash. They all load the profile when tested via "env -i bash -c 'exec -l foo -c env'" (replace foo with shell), except for bash without NON_INTERACTIVE_LOGIN_SHELLS.
Comment 114 Edgar Hoch 2017-02-08 08:40:29 UTC
(In reply to Jonas Hahnfeld from comment #110)
> The patch does not work on ArchLinux where bash is compiled without
> NON_INTERACTIVE_LOGIN_SHELLS. Can -i be added before '-c'?

Option "-i" doesn't make sense because a shell that runs a script is NOT interactive by definition. The shell should not behave as it has a terminal to the user.
Comment 115 Jonas Hahnfeld 2017-02-08 08:46:19 UTC
(In reply to Jan Alexander Steffens (heftig) from comment #113)
> I've also tried zsh, fish, dash and busybox ash. They all load the profile
> when tested via "env -i bash -c 'exec -l foo -c env'" (replace foo with
> shell), except for bash without NON_INTERACTIVE_LOGIN_SHELLS.

So the interesting question is whether all of these also work with an additional '-i'.

(In reply to Edgar Hoch from comment #114)
> (In reply to Jonas Hahnfeld from comment #110)
> > The patch does not work on ArchLinux where bash is compiled without
> > NON_INTERACTIVE_LOGIN_SHELLS. Can -i be added before '-c'?
> 
> Option "-i" doesn't make sense because a shell that runs a script is NOT
> interactive by definition. The shell should not behave as it has a terminal
> to the user.

bash wants us to use 'bash -l' for that and not 'exec -l bash'. Should we add a special case for $SHELL == "bash"?