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

Fall back to /bin/bash for the default shell path. #239

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

mikepurvis
Copy link
Member

Fixes an issue when building in an environment without SHELL specified (such as inside debuild).

@NikolausDemmel
Copy link
Member

+1

I believe this is a reasonable default until a different choice can be motivated with a use case in #226.

@@ -59,6 +59,7 @@ def get_resultspace_environment(result_space_path, quiet=False, cached=True):
# Use fallback shell if using a non-standard shell
if shell_name not in ['bash', 'zsh']:
shell_name = 'bash'
shell_path = "/bin/bash"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, specifying the default not here, but above in shell_path = os.environ.get('SHELL', '/bin/bash') would allow to set SHELL to something custom not ending in /bash and still have it work (with the --norc flag as if it were bash). With this PR, the value in SHELL gets overwritten if it does not end in /bash or /zsh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what the other PR did initially with pjreed@4e730b3, but there were some concerns about sh being maybe a better default. As I said I would stick with bash until it is clear how sh should behave.

@mikepurvis
Copy link
Member Author

Right, makes sense— changed.

@pjreed @wjwwood @jack-oquin You guys good to see this go in?

@pjreed
Copy link

pjreed commented Dec 8, 2015

I'm fine with it.

@wjwwood
Copy link
Member

wjwwood commented Dec 9, 2015

I'm going to merge this and then add a check that /bin/bash exists and throw a meaningful error if it does not.

wjwwood added a commit that referenced this pull request Dec 9, 2015
Fall back to /bin/bash for the default shell path.
@wjwwood wjwwood merged commit f1ee783 into catkin:master Dec 9, 2015
@wjwwood
Copy link
Member

wjwwood commented Dec 9, 2015

#243

@mikepurvis mikepurvis deleted the fallback-shell-fix branch December 9, 2015 19:05
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.

4 participants