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

Improve handling for SHELL environment variable: #421

Merged
merged 1 commit into from
Jan 7, 2017
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jan 6, 2017

  • Use boolean conditional for checking shell_path to catch empty strings.

  • Throw an error if shell_path does not exist.

This is a rebased version of #414

* Use boolean conditional for checking shell_path to catch empty strings.

* Throw an error if shell_path does not exist.
@wjwwood wjwwood merged commit db3965f into master Jan 7, 2017
@wjwwood wjwwood deleted the apetrone-master branch January 7, 2017 00:28
@mingless
Copy link

It might be irrelevant, but in the dotfiles I am using SHELL is being set to 'bash' rather than '/bin/bash' which resulted in this commit breaking my catkin_tools. Since the fix is simple and I am not the original creator of the script I am not reporting any particular issue, but rather asking if it is good to assume that SHELL must be '/bin/bash' and cannot be 'bash'.

@wjwwood
Copy link
Member Author

wjwwood commented Feb 15, 2017

I'm not sure if we can safely assume that the value in SHELL is absolute (i.e. we can run a subprocess with shell=Fase), but we currently do assume that. You can make an issue about this and someone can look into it as they have time.

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.

3 participants