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
subrocess.Popen needs /bin/sh but Android only has /system/bin/sh #60459
Comments
The subprocess.Popen function uses /bin/sh in Unix environments. Android is detected as a Unix environemnt, but has moved that executable to /system/bin/sh. This can be worked around by adding a parameter "executable='/system/bin/sh'" to the call, but it is impractical to do this for every call to Popen in every library and codebase. For subprocess.Popen to work on Android, Popen needs to be able to detect if /bin/sh is not there and try /system/bin/sh instead. |
Android really should not be breaking the standards that way. We do want to support Android, but have you submitted a bug report to them? |
...Applications should note that the standard PATH to the shell cannot be assumed to be either /bin/sh or /usr/bin/sh, and should be determined by interrogation of the PATH returned by getconf PATH , ensuring that the returned pathname is an absolute pathname and not a shell built-in.... |
@RPetrov thanks for finding that - I was having trouble hunting down what the standard is. I think you're quoting from http://pubs.opengroup.org/onlinepubs/009695399/utilities/sh.html |
We should do that, then, if /bin/sh doesn't exist. |
I wouldn't blame android for this; I doubt Android claims to support whatever standard you are holding it to. It seems simple enough for us to make the default configurable (a public module level constant that anyone can override in their code after importing the module to change the default) or to otherwise query the system for where the default shell is at import time. |
Well, posix; but I was wrong about what posix required, as Roumen pointed out. |
Is there some document describing procedure for building/installing/running_tests at Android device? I have Samsung Galaxy Tab and would to work on the issue. |
This seems good to do in any case (as opposed to having the string be "hard-coded"), and we can test it independently of Android. There is a recently added test in the 3.x branches that can be modified: test method "test_executable_replaces_shell" which checks that the executable argument replaces the default shell for non-Windows. I would suggest implementing Gregory's two suggested measures as two separate patches. |
I'm using the android-python27 project, but you can also reproduce it under SL4A: Start SL4A >>> from subprocess import Popen
>>> p = Popen("ls", shell=True)
*Fails* OSError: [Error 2] No such file or directory
>>> p = Popen("ls" shell=True, executable="/system/bin/sh")
*Works* |
It occurs to me that logic for detecting the shell might make sense for being part of the os module, e.g. os.getdefaultshell(). |
Chris, agree with you. |
I created bpo-16353 for adding a function to the os module for getting the path to the default shell. The current bpo-16255 could be addressed in 3.4 by calling such a function from the subprocess module. For earlier versions (since enhancements are not allowed), the current issue could perhaps be addressed by backporting the logic in such a function directly into the subprocess module (i.e. without adding a new function to the os module). |
Why not simply use $SHELL? |
$SHELL is up to the user and not guaranteed to be anything remotely /bin/sh
|
Assuming bpo-16353 is fixed using http://bugs.python.org/file36196/os.get_shell_executable.patch the appropriate way to find the path to the default shell is by calling os.get_shell_executable(). This is the 1-liner patch that uses os.get_shell_executable() in Popen() instead of the hard-coded string "/bin/sh". |
It seems that Android is the only known platform that deviates from |
The list of locations where '/bin/sh' is hard coded in the standard library: Lib/distutils/tests/test_build_scripts.py|68 col 31| ("#!/bin/sh\n"
Lib/distutils/tests/test_install_scripts.py|57 col 38| write_script("shell.sh", ("#!/bin/sh\n"
Lib/distutils/tests/test_spawn.py|34 col 37| self.write_file(exe, '#!/bin/sh\nexit 1')
Lib/distutils/tests/test_spawn.py|45 col 37| self.write_file(exe, '#!/bin/sh\nexit 0')
Lib/subprocess.py|611 col 24| >>> check_output(["/bin/sh", "-c",
Lib/subprocess.py|1450 col 26| args = ["/bin/sh", "-c"] + args
Lib/test/test__osx_support.py|46 col 24| f.write("#!/bin/sh\n/bin/echo OK\n")
Lib/test/test__osx_support.py|58 col 24| f.write("#!/bin/sh\n/bin/echo ExpectedOutput\n")
Lib/test/test__osx_support.py|149 col 28| f.write("#!/bin/sh\n/bin/echo " + c_output)
Lib/test/test__osx_support.py|205 col 24| f.write("#!/bin/sh\nexit 255")
Lib/test/test_os.py|673 col 42| @unittest.skipUnless(os.path.exists('/bin/sh'), 'requires /bin/sh')
Lib/test/test_os.py|677 col 24| with os.popen("/bin/sh -c 'echo $HELLO'") as popen:
Lib/test/test_os.py|681 col 42| @unittest.skipUnless(os.path.exists('/bin/sh'), 'requires /bin/sh')
Lib/test/test_os.py|684 col 14| "/bin/sh -c 'echo \"line1\nline2\nline3\"'") as popen:
Lib/test/test_subprocess.py|1563 col 31| fobj.write("#!/bin/sh\n")
Lib/test/test_subprocess.py|1611 col 31| fobj.write("#!/bin/sh\n")
Lib/test/test_subprocess.py|1629 col 15| sh = '/bin/sh' |
I've also opened a feature request here: |
I have entered bpo-27472. |
The attached patch fixes the issue. There is at least already an existing test case: test_subprocess.MiscTests.test_getoutput calls subprocess.getstatusoutput() that runs a Popen instance with shell=True. |
Apparently sysconfig.get_config_var() is already called in site.py anyway, In the stdlib it seems to be the first use outside site.py though, and But perhaps I'm being irrational here. Victor, what do you think? |
New patch that does not call sysconfig.get_config_var() on platforms that have '/bin/sh'. |
New patch using sys.getandroidapilevel() that is only defined for Android. |
For some reason the following notifications have not all been received (yet): But the buildbots have processed the build request. |
New changeset 96a9992d1003 by Xavier de Gaye in branch 'default': |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: