Skip to content
New issue

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

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

Already on GitHub? # 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.

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

Successfully merging this pull request may close these issues.

3 participants