Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

profile: Don't rely on bash syntax #2597

Closed
wants to merge 1 commit into from
Closed

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented Jan 15, 2019

In Debian, we reuse flatpak.sh in /etc/X11/Xsession.d (which is sourced
by /bin/sh, normally dash) so that these environment variables become
part of the X11 session environment. We might also have
non-bash-compatible shells that read profile.d (I'm not sure).

@smcv
Copy link
Collaborator Author

smcv commented Jan 15, 2019

shellcheck -s sh /etc/profile.d/flatpak.sh is happy with this, and it does seem to work:

% dash
$ unset XDG_DATA_DIRS
$ unset XDG_DATA_HOME
$ . /etc/profile.d/flatpak.sh
$ echo $XDG_DATA_DIRS
/home/smcv/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share:/usr/share

@smcv smcv mentioned this pull request Jan 15, 2019
In Debian, we reuse flatpak.sh in /etc/X11/Xsession.d (which is sourced
by /bin/sh, normally dash) so that these environment variables become
part of the X11 session environment. We might also have
non-bash-compatible shells that read profile.d (I'm not sure).

Fixes flatpak#2594

Signed-off-by: Simon McVittie <smcv@debian.org>
@matthiasclasen
Copy link
Collaborator

This does look like process substitution, still. So, does dash this support or not? Gotta love using a different shell just because

@mwleeds
Copy link
Collaborator

mwleeds commented Jan 15, 2019

Yes, I can confirm this works with dash and allows gdm to start

@matthiasclasen
Copy link
Collaborator

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 7189142 has been approved by matthiasclasen

@rh-atomic-bot
Copy link

⌛ Testing commit 7189142 with merge 2a25ecf...

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: matthiasclasen
Pushing 2a25ecf to master...

@chrisjbillington
Copy link

It sounds like bash does not support this either in this context:

/etc/gdm/Xsession uses #!/bin/sh /bin/sh is a sym link to /bin/bash. Calling bash as sh places it in posix compatibility mode which does not support process substitution.

@eli-schwartz
Copy link

eli-schwartz commented Jan 16, 2019

No need to spawn a new shell with ( ... ) as you can group commands using { ...; } as well. All you care about in this context is redirecting output for both commands to the same pipeline, and accepting all input into the read loop without deleting variables internal to the loop (see second bullet at http://mywiki.wooledge.org/BashFAQ/024#Workarounds)

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04_01

@eli-schwartz
Copy link

BTW why was the initial code ever merged in the first place, as it was reviewed as being faulty at the time: #2122 (comment)

@matthiasclasen
Copy link
Collaborator

It was merged because it worked for bash, and at the time nobody had a better solution. Merging it was successful in getting us one.

@Scimmia22
Copy link

So the strategy is to break things so that someone else will come by to fix them?

@lhanson
Copy link

lhanson commented Feb 25, 2019

So the strategy is to break things so that someone else will come by to fix them?

That's the impression I'm getting as well. If you write code that requires bash, make bash a requirement and add a proper shebang to actually invoke bash instead of pretending /bin/sh will work and shrugging when it doesn't work for huge numbers of users.

@smcv
Copy link
Collaborator Author

smcv commented Feb 26, 2019

If you write code that requires bash, make bash a requirement and add a proper shebang

That can't work here. Code that gets sourced by an existing shell, rather than run (in /etc/profile.d, /etc/Xsession.d and similar directories) does not have the opportunity to choose which shell will run it.

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

Successfully merging this pull request may close these issues.

None yet

8 participants