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

Do not assume a path to the sh and env binaries #528

Merged
merged 2 commits into from Jul 17, 2017
Merged

Do not assume a path to the sh and env binaries #528

merged 2 commits into from Jul 17, 2017

Conversation

fornwall
Copy link
Contributor

This improves portability, specifically for non-rooted Android devices which lacks /bin and /usr folders.

This improves portability, specifically for non-rooted Android
devices which lacks /bin and /usr folders.
@tduehr
Copy link
Member

tduehr commented Sep 27, 2016

This also breaks installs where PATH is scrubbed before this is run. /usr/bin and /bin are standard locations for these binaries in most distributions and it's common practice to hardcode their location. Where are they in Android? A better route will be to detect they're not in the expected location and use a different path.

@fornwall
Copy link
Contributor Author

This also breaks installs where PATH is scrubbed before this is run.

Do you know of any such install (or have ever encountered one) with sh and env not in PATH, or is it more of a defensive plan?

I have been pushing in some changes in various projects that replaces /bin/sh => sh and /usr/bin/env => env (like in oh-my-zsh and gevent), so would like to know if I'm breaking something there!

Where are they in Android?

There are system binaries in /system/bin/, but the sh implementation varies over Androd devices and is not that reliable as a standard general sh, so in Termux an app-supplied sh is put in an app-private folder (a horrible path of /data/data/com.termux/files/usr/bin/sh).

A better route will be to detect they're not in the expected location and use a different path.

Would something like the below be ok to determine which sh to use? It would only fall back to using PATH if /bin/sh does not exist:

SHELL=/bin/sh
if [ ! -x /bin/sh ]; then SHELL=sh; fi

@tduehr
Copy link
Member

tduehr commented Sep 27, 2016

/usr/bin/env and /bin/sh are standard locations across Unix like systems. Any widely distributed, mildly cross-platform shell script will use the following shebang:

#!/usr/bin/env [ruby|python|bash|perl|etc]

The assumed shell in every unix-like system I've used is /bin/sh which should be at least compatible with Bourne Shell. A lot of software is unhappy if that is not the case. Usually resulting in a platform specific package and patch.

Fallback to relying on PATH is okay. but it should probably try /system/bin/sh before falling back to PATH.

@polyzium
Copy link

I tried it myself locally with Termux, it failed to build:

1 @localhost ~/ffi (git)-[master] % rake gem:install                                                                                                                                                    
rake aborted!
NoMethodError: undefined method `last_comment' for #<Rake::Application:0x00007f9c657ab8>
/data/data/com.termux/files/home/ffi/Rakefile:227:in `new'
/data/data/com.termux/files/home/ffi/Rakefile:227:in `block in <top (required)>'
/data/data/com.termux/files/home/ffi/Rakefile:226:in `<top (required)>'

@tduehr
Copy link
Member

tduehr commented Mar 20, 2017

Where are sh and env on that device? Would fallback to PATH have fixed it?

@fornwall
Copy link
Contributor Author

Where are sh and env on that device? Would fallback to PATH have fixed it?

They are under /data/data/com.termux/files/usr/bin/ which is in the PATH so using PATH should fix it.

Would something like this be fine (if the check for certain paths is still desired)?

SH=sh
ENV=env
if [ -x /bin/sh ]; then SH=/bin/sh; fi
if [ -x /usr/bin/env ]; then ENV=/usr/bin/env; fi

And then use $ENV and $SH to execute them?

@tduehr
Copy link
Member

tduehr commented Apr 20, 2017

Probably just use env vars to override their location. Not sure why I didn't think of that earlier.

Test the env var then test the standard path and finally fall back to PATH.

@fornwall
Copy link
Contributor Author

fornwall commented May 7, 2017

@tduehr Like this?

ENV=${ENV:-/usr/bin/env}
SH=${SH:-/bin/sh}

if [ ! -x "$SH" ]; then SH=sh; fi
if [ ! -x "$ENV" ]; then ENV=env; fi

@tduehr
Copy link
Member

tduehr commented May 8, 2017

Yup

@tduehr
Copy link
Member

tduehr commented Jul 17, 2017

Upon further research...

@tduehr tduehr merged commit a3e6f83 into ffi:master Jul 17, 2017
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

3 participants