This is an archive of the discontinued LLVM Phabricator instance.

When invoking Terminal, don't assume the default shell
ClosedPublic

Authored by beanz on Oct 18 2016, 1:53 PM.

Details

Summary

If a user has their shell set to a non-POSIX conferment shell the TestTerminal.py tests fail because the shell blurb constructed here may not work in their shell.

In my specific case fish-shell (The Friendly Interactive Shell - http://fishshell.com) does not support $?, it instead uses $status (because it is friendly).

This patch removes the assumption of your default shell by running the constructed bash command via "/bin/bash -c ...". This should be safer for users mutating their shell environment.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 75069.Oct 18 2016, 1:53 PM
beanz retitled this revision from to When invoking Terminal, don't assume the default shell.
beanz updated this object.
beanz added a reviewer: tfiala.
beanz added a subscriber: lldb-commits.
tfiala accepted this revision.Oct 18 2016, 3:00 PM
tfiala edited edge metadata.

Yep, looks right. We shouldn't be assuming the shell.

This revision is now accepted and ready to land.Oct 18 2016, 3:00 PM
joerg added a subscriber: joerg.Oct 18 2016, 4:08 PM

Hard-coding /bin/bash is not an improvement in my opinion. There are quite a few systems where this is plainly breaking things.

joerg added a comment.Oct 18 2016, 4:09 PM

OK, this is OSX specific, but I still think that hardcoding bash is a bad idea. Does it need anything not in /bin/sh?

This revision was automatically updated to reflect the committed changes.
beanz added a comment.Oct 18 2016, 5:06 PM

@joerg, it is OS X exclusive, and bash is the system default shell, so I think that makes sense.