classification
Title: add function to os module for getting path to default shell
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Chi Hsuan Yen, Roman.Evstifeev, WanderingLogic, akira, asvetlov, chris.jerdonek, christian.heimes, eric.araujo, eryksun, ezio.melotti, gregory.p.smith, lyapun, ned.deily, neologix, pitrou, r.david.murray, serhiy.storchaka, xdegaye
Priority: normal Keywords: patch

Created on 2012-10-29 01:19 by chris.jerdonek, last changed 2016-07-10 16:30 by xdegaye.

Files
File name Uploaded Description Edit
issue16353.diff lyapun, 2012-11-03 11:28 review
issue16353_added_doc_tag.diff lyapun, 2012-11-03 13:55 review
issue16353.diff lyapun, 2012-11-05 18:48 review
issue16353.diff lyapun, 2012-11-05 19:30 review
os.get_shell_executable.patch akira, 2014-08-01 19:24 os.get_shell_executable() implementation, docs, tests. review
os.get_shell_executable-3.patch akira, 2014-11-06 20:19 exclude '' completely review
default_shell.patch xdegaye, 2016-05-23 10:17 review
Messages (47)
msg174093 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-29 01:19
This issue is to add a function to the os module for getting the path to the default shell (e.g. os.getdefaultshell()).

In issue 16255, it was reported that on Android, the path to the default shell is "/system/bin/sh" rather than "/bin/sh" which it is on other Unix systems.

Such a function in the os module would implement the necessary detection logic which could then be used, for example, by the subprocess module when invoking Popen() with shell=True (e.g. for issue 16255).
msg174569 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-02 20:32
Is it the "system default shell" or the user's default shell that we want here?

That is, shouldn't we look up `pwd.getpwuid(os.getuid()).pw_shell` ?
(but only when os.getuid() == os.geteuid()?)
msg174572 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-02 20:47
The system default shell is what I had in mind when filing this issue (i.e. what Popen() uses by default or, in the case of Android, what Popen() should use).
msg174574 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-02 20:53
> The system default shell is what I had in mind when filing this issue
> (i.e. what Popen() uses by default or, in the case of Android, what
> Popen() should use).

Well, the question then becomes whether Popen() shouldn't use the user's
default shell instead? :)
It probably doesn't make much of a difference in most cases, though.
Also, there's the problem that some special users may have a void shell
entry (or /bin/false or /sbin/nologin), to prevent a human from logging
in as them.
msg174576 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-02 21:00
I guess to return sh if supported, cmd.exe for Windows.
The reason is: other shells can have different calling agreements (I mean rules to process command line).
subprocess intended to use by library writers, so we need solid well known basis.

There are another option — provide two functions: first for shell in terms of subprocess requirements and second will return actual user shell.
I like it.
msg174578 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2012-11-02 21:07
On 02/11/2012 21:00, Andrew Svetlov wrote:
> I guess to return sh if supported, cmd.exe for Windows.

FWIW the canonical approach on Windows is to return whatever %COMSPEC% 
points to.
msg174580 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-02 21:17
> Well, the question then becomes whether Popen() shouldn't use the user's default shell instead? :)

That's a good question, too. :)  I was thinking just in terms of supporting the status quo.  Maybe two functions would be useful? (as suggested also by Andrew)

I do think the question of changing Popen()'s behavior should be decided independently of this issue though.  In other words, even if we change Popen() in the future, I think exposing the system default shell would still be worthwhile.
msg174582 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-11-02 21:25
> That is, shouldn't we look up `pwd.getpwuid(os.getuid()).pw_shell` ?
> (but only when os.getuid() == os.geteuid()?)

No, you can't use the users shell from the pwd module.  That can be
any crazy program.  Not a functional /bin/sh for use in making
commands when shell=True in subprocess which is where this feature
request came from.

*system* default, not the user's.  subprocess MUST use /bin/sh or
equivalent, ignoring any per-user setting.
msg174583 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-11-02 21:28
The "system default shell" (which should always be a /bin/sh workalike, I think) should always be the default.  Any other shell should be something that has to be specified explicitly.  At least, that's the way most posix programs work, I think.  As Chris said, different shells can use different syntax, so if you want to write portable code you want to be able to expect /bin/sh by default.

So at a minimum we want a function for the system default shell.  Anything else is a bonus :)
msg174586 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-02 21:34
BTW, according to PEP 11 (http://www.python.org/dev/peps/pep-0011/) 
Python 3.4 will remove code for "Windows systems where COMSPEC points to command.com". There are only winner: cmd.exe as well known shell good as default, command.com is gone.

cmd.exe can be assumed as default shell as well as sh for posix platforms.

About subprocess: I like to add something to point str (or pathlib path when accepted) as shell, but it's definitely another issue.
msg174607 - (view) Author: Taras Lyapun (lyapun) * Date: 2012-11-03 11:28
Diff with implementation, test and doc.
msg174619 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-03 13:41
I would to prefer *get_default_shell* as function name (it's a bit shorter).

Also please add ".. versionadded:: 3.4" tag to docs.
msg174621 - (view) Author: Taras Lyapun (lyapun) * Date: 2012-11-03 13:55
> I would to prefer *get_default_shell* as function name (it's a bit shorter).

Hm... I'm not sure, because someone can imagine that function returns some object or something like this... 'path' indicates that function returns something like string.

> Also please add ".. versionadded:: 3.4" tag to docs.

Oh, sorry, added.
msg174623 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-03 14:09
which('sh') isn't correct here. 'which' searches in all PATH environ parts. However the shell must be looked up in CS_PATH only.

From man sh(1posix):

> 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.

'getconf PATH' queries confstr(_CS_PATH).

I suggest that you modify the POSIX part to:

  path = os.confstr("CS_PATH")
  which('sh', path=path)
msg174624 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-03 14:13
+++ b/Lib/shutil.py	Sat Nov 03 13:32:05 2012 +0200
+
+def get_default_shell_path():

Why is the patch putting the function in the shutil module?  The function should go in the os module as the title and comments of this issue state.  shutil seems misplaced to me.

For example, the description of shutil in the docs is, "The shutil module offers a number of high-level operations on files and collections of files.  In particular, functions are provided which support file copying and removal."  In contrast, the description of the os module is, "This module provides a portable way of using operating system dependent functionality."  The default shell is operating system dependent functionality.
msg174628 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-03 14:51
I'm with Chris. The information should be stored in the os module. I suggest os.shell var or os.get_shell() function like this:

def get_shell():
    for path in confstr("CS_PATH").split(pathsep):
        sh = sep.join((path, "sh"))
        try:
            mode = stat(sh).st_mode
        except OSError:
            pass
        else:
            if st.S_ISREG(mode):
                return sh
    raise FileNotFound("sh")

According to all examples S_ISREG() is sufficient here.

On Windows the function should use the env var COMSPEC instead of hard coding "cmd.exe".
msg174629 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-03 15:04
Is it ok to import *which* functions from shutil in *os* module?
There is only reason to put function into shutil.

But I like Christian's sketch.

Also, what reason to get shell name from COMSPEC? What should we do if COMSPEC points to some another shell, not cmd.exe?
msg174631 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-11-03 15:12
I think it would not be ok to import shutil in os, so I'm glad there is an alternative.
msg174646 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-03 16:26
> Also, what reason to get shell name from COMSPEC? What should we do if COMSPEC points to some another shell, not cmd.exe?

FWIW, the subprocess module does this (with surrounding code linked after):

comspec = os.environ.get("COMSPEC", "cmd.exe")

http://hg.python.org/cpython/file/ed091424f230/Lib/subprocess.py#l1060
msg174652 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-03 17:07
The os module can't import shutil as it would create a circular import. Also shutil.which() does lots of stat calls and we don't want additional stat calls in a core module like os. My implementation requires just one stat() call.

COMSPEC can point to an alternative command implementation. That's the point of COMSPEC. Perhaps some people remember 4DOS and 4NT.
msg174689 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-11-03 20:21
Any interest in doing like os.get_terminal_size/shutil.get_terminal_size with the os function being basic (i.e. current patch) and the shutil version querying the env var SHELL in addition?
msg174690 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-11-03 20:27
Please use ``sh`` or maybe :command:`sh` (check the sphinx doc) instead of `sh`.
msg174885 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-05 09:32
Christian,
Is there ``os.confstr`` supported by MaxOS X?
Is there using of environ['PATH'] makes sense as good callback if former is not present?

About COMSPEC. From my point of view it's useful if we need default path.
Or if we have Win9x, where shell is command.com (not supported by 3.4)

I guess to hardcode ``cmd.exe`` for Windows for the same reason as we hardcode ``sh`` for Unix systems.

I agree to moving code to ``os`` if we no need to use ``shutil.which``.
The function name is still the question: getdefaultshell, getshell, get_shell etc.

Adding shutil.getdefaultshell function is interesting suggestion. It function intended to return user shell (from SHELL env var or COMSPEC for Windows) and not used in subprocess if shell=True (but can be passed as executable param if need).
msg174886 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-05 10:25
> Any interest in doing like os.get_terminal_size/shutil.get_terminal_size

If functions with two different behaviors are needed, I think the two functions should probably have different names (e.g. get_shell() and get_user_shell()).  Otherwise, it may create confusion as to which to call (or which is being called when reading code).

Moreover, it doesn't seem like the following guidance in the docs for which get_terminal_size() to use would hold in parallel for the shell variants: "shutil.get_terminal_size() is the high-level function which should normally be used, os.get_terminal_size is the low-level implementation."
msg174929 - (view) Author: Taras Lyapun (lyapun) * Date: 2012-11-05 18:48
Updated patch.
Moved function to os and used Christian Heimes implementation.
Updated doc, and test.
Also renamed function to get_shell.

Test passes on mac os and windows.
msg174930 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-05 19:16
I've tested confstr("CS_PATH") on Linux, Mac OS X, Solaris, HP-UX and BSD. It works and the path to `sh` is always included.

Taras:
You don't have to perform the platform inside the get_shell() function. I suggest that you define the function depending on the value of `name`.

if name == "posix":
    def get_shell():
        ...
elif name in {"nt", "ce"}:
    def get_shell():
        ...
msg174932 - (view) Author: Taras Lyapun (lyapun) * Date: 2012-11-05 19:30
Updated patch in regards to Christian Heimes remark.
msg174933 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-05 19:31
Is someone able to test the patch on Android (the impetus for this issue)?
msg174935 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-11-05 19:51
Some minor comments.

+.. function :: get_shell()
+
+  Return the path to default shell.

Three leading spaces needed.  Also, "Return the path to the default shell."

+  For unix system returns path to ``sh``, for windows returns path to ``cmd.exe``.

"On Unix systems returns the path to ``sh``, and on Windows returns the path to ``cmd.exe``."  I would also document and test FileNotFound (e.g. by temporarily changing os.confstr).

+        """Return the path to default shell for Unix."""

the default shell

+        """Return the path to default shell for Windows."""

the default shell

+class TestGetShell(unittest.TestCase):
+    def test_get_shell(self):
+        shell = os.get_shell()
+        param = "/c" if sys.platform == 'win32' else "-c"
+        s = subprocess.Popen([shell, param, "echo test"],
+                                stdout=subprocess.PIPE)

Alignment should be:

+        s = subprocess.Popen([shell, param, "echo test"],
+                             stdout=subprocess.PIPE)
msg174962 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-06 13:35
Meh, Python 2.6 from SL4A (scripting languages for Android) doesn't have os.confstr(). I just tried it in a Android 2.3.3 emulator. Python 2.6 on Linux has the function. I propose we fall back to PATH env and use /bin as last resort.

    try:
        cspath = confstr("CS_PATH")
    except (ValueError, NameError):
        # installation doesn't have confstr() or doesn't know about CS_PATH
        # fall back to PATH environ and use /bin as last resort
        cspath = environ.get("PATH", "/bin")
    for path in cspath.split(pathsep):
        ...
msg175002 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-06 19:32
Confirm it for SL4A Python 3.2
msg175003 - (view) Author: Taras Lyapun (lyapun) * Date: 2012-11-06 19:36
Andrew, do you mean that Christian Heimes last snippet working on Android?
msg175006 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-11-06 20:00
I mean the last available SL4A doesn't have os.confstr
Fallback should work, os.environ['PATH'] contains '/system/bin' where 'sh' is living.
msg203160 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-11-17 14:10
3.4 beta1 will be released next weekend. I'll try to submit a patch in the next couple of days.
msg224477 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-01 09:31
Should it be function? Why not use just a variable initialized at import time? The path to the default shell shouldn't change during the time of program execution.

if sys.platform == 'win32':
    default_shell = 'cmd.exe'
else:
    default_shell = '/bin/sh'
msg224508 - (view) Author: Eryk Sun (eryksun) * Date: 2014-08-01 15:57
> The path to the default shell shouldn't change during the time 
> of program execution.

On Windows the ComSpec environment variable could change during program execution.
msg224509 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-08-01 16:47
Taking into consideration msg174586 we can ignore the ComSpec environment 
variable and hardcode "cmd.exe" on Windows, as we ignore the SHELL environment 
variable on Posix systems.
msg224512 - (view) Author: Eryk Sun (eryksun) * Date: 2014-08-01 17:51
Defaulting to just "cmd.exe" can execute an arbitrary program named "cmd.exe" in the application directory or current directory. 

When CreateProcess needs to find the shell to execute a batch file (.bat or .cmd), it doesn't search for "cmd.exe" -- at least not in NT 6. If ComSpec isn't set, it defaults to "%SystemRoot%\system32\cmd.exe". If both ComSpec and SystemRoot are unset, executing a batch file via CreateProcess fails with ERROR_DIRECTORY. 

I think Python should use the same default path if it's ignoring ComSpec:

    default_shell = path.join(environ["SystemRoot"], "system32", "cmd.exe")

A KeyError shouldn't be a problem here. CPython won't even run if SystemRoot isn't set to the correct path:

    C:\>set SystemRoot=
    
    C:\>py -3
    Fatal Python error: Failed to initialize Windows random API (CryptoGen)
msg224514 - (view) Author: Akira Li (akira) * Date: 2014-08-01 19:11
> Should it be function? Why not use just a variable initialized at
> import time? The path to the default shell shouldn't change during
> the time of program execution.

> if sys.platform == 'win32':
>     default_shell = 'cmd.exe'
> else:
>    default_shell = '/bin/sh'

Andriod uses /system/bin/sh (and /sbin/sh if we believe some adb
source on the internet). See issue16255: "subrocess.Popen needs
/bin/sh but Android only has /system/bin/sh"

POSIX recommends [1] `getconf PATH` (os.confstr('CS_PATH')):

  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.

  For example, to determine the location of the standard sh utility:

    $ command -v sh

i.e. shell executable is shutil.which('sh', os.defpath) on POSIX.
os.defpath could be tweaked on Android.

To avoid dependency on shutil something like msg174628 could be
adopted:

  if sys.platform == "win32": # use sys.platform to accommodate java
      def get_shell_executable():
          """Return path to the command processor specified by %ComSpec%.

          Default: %SystemRoot%\system32\cmd.exe
          """
          return (environ.get('ComSpec') or
                  path.join(environ.get('SystemRoot', r'C:\Windows'),
                            r'system32\cmd.exe'))
  else: # POSIX
      def get_shell_executable():
          """Return path to the standard system shell executable.

          Default: '/bin/sh'
          """
          for dirpath in defpath.split(pathsep):
              sh = dirpath + sep + 'sh' #NOTE: interpret '' as '/'
              if access(sh, F_OK | X_OK) and path.isfile(sh):
                  #NOTE: allow symlinks e.g., /bin/sh on Ubuntu may be dash
                  return sh
          return '/bin/sh' #XXX either OS or python are broken if we got here

Windows part is based on msg224512. If r'C:\Windows' or '/bin/sh' code
is reached then it means that either OS or python are
broken/misconfigured.

system(), popen() are described on POSIX as [2]:

  # fork()
  execl(shell path, "sh", "-c", command, (char *)0);

subprocess module that replaces system, popen, spawn*p* calls may use
os.get_shell_executable() to implement shell=True.

os.defpath is [3]:

  The default search path used by exec*p* and spawn*p* if the
  environment doesn’t have a 'PATH' key. Also available via os.path.

In the absense of os.confstr('CS_PATH') on Android (msg175006),
os.defpath seems appropriate than os.environ['PATH'] to expand 'sh'
basename to the full path to be used by subprocess later.

os.get_shell() name might imply that a shell object is returned that
can run commands like for example os.popen() returns a file-like
object that is not true (the intent is to return a path -- a simple
string).

os.get_shell_executable() is similar to sys.executable that returns
path to python executable.

os.get_shell_executable() is not necessary for every python run
therefore it is better to keep it a function to avoid unnecessary stat
calls on startup. Though the result should not change during the
program run.

> On Windows the ComSpec environment variable could change during
> program execution.

Does it? The docs for os.environ say [4]:

  This mapping is captured the first time the os module is imported,
  typically during Python startup as part of processing
  site.py. Changes to the environment made after this time are not
  reflected in os.environ, except for changes made by modifying
  os.environ directly.

I've uploaded a patch with the described implementation (plus docs, tests).

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html
[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/popen.html
[3] https://docs.python.org/3.4/library/os.html#os.defpath
[4] https://docs.python.org/3.4/library/os.html#os.environ
msg230711 - (view) Author: Matt Frank (WanderingLogic) * Date: 2014-11-05 20:01
Unfortunately os.defpath seems to be hardcoded.  And hardcoded to the wrong value on every system I have looked at, including Linux.

Lib/posixpath.py sets defpath=':/bin:/usr/bin' which is _not_ what `getconf CS_PATH` returns on my Linux (the extra ':' at the beginning means "include the cwd"[1] which is almost certainly not what anyone wants.)  The hardcoded value '/bin:/usr/bin' would also be wrong for Solaris and AIX installations that are trying to be POSIX (I think they use /usr/xpg4/bin/sh).

Meanwhile Lib/macpath.py sets defpath=':', which is also almost certainly wrong.  (I don't have a Mac and have never programmed one, but StackOverflow[2] indicates that one should use `path_helper` (which in turn gets the default value by reading it from the files in /etc/paths.d))[3]  So it seems likely that this patch breaks Popen() on MacOS (unless, perhaps, a path of ':' means something special on Mac?).

And Lib/ntpath.py sets defpath='.;C:\\bin', which doesn't resemble a path that even works (as pointed out in now closed http://bugs.python.org/issue5717).  (The correct value may be '%SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;%SYSTEMROOT%\System32\WindowsPowerShell\v1.0\'.)

So I don't know where to go next.  I'm happy to cook up a different patch, but I have no idea what it should be.  Here are some possibilities:

1. Kick the can down the road: file a new bug against os.defpath and (somehow) fix Lib/*path.py so they do something more reasonable in more cases.  One of which might be to have posixpath.py try to call os.confstr() (and then, perhaps, special-code something in Modules/posixmodule.c in the case that HAVE_CONFSTR is not defined.)

2. Modify @akira's patch to call os.confstr('CS_PATH') instead of os.defpath, and then deal with the fact that very few systems actually implement confstr() (perhaps by special-coding something in Modules/posixmodule.c as described above.)

3. Modify this patch to fall back to `PATH` if `sh` can't be found on os.defpath (or os.confstr('CS_PATH') fails).



[1] `man bash` on Linux, search for the description of the PATH variable.  (e.g., http://man7.org/linux/man-pages/man1/bash.1.html)
[2] http://stackoverflow.com/questions/9832770/where-is-the-default-terminal-path-located-on-mac
[3] https://developer.apple.com/library/mac/documentation/Darwin/Reference/Manpages/man8/path_helper.8.html
[4] http://superuser.com/questions/124239/what-is-the-default-path-environment-variable-setting-on-fresh-install-of-window
msg230713 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-11-05 20:42
Matt, ignore Lib/macpath.py.  It is not used on OS X systems other than in the rare case that someone explicitly needs to parse obsolete Classic Mac OS (Mac OS 9 or earlier) style path names.  OS X uses Lib/posixpath.py.
msg230720 - (view) Author: Akira Li (akira) * Date: 2014-11-06 00:47
> Matt Frank added the comment:
>
> Unfortunately os.defpath seems to be hardcoded.  And hardcoded to the
> wrong value on every system I have looked at, including Linux.

os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
used on Linux) sysdeps/posix/spawni.c I don't know whether it is
possible to change CS_PATH without recompiling every statically linked
executable on a system, sysdeps/unix/confstr.h:

  #define CS_PATH "/bin:/usr/bin"

Though this issue is about the path to the standard shell sh/cmd.exe.
It is not about os.defpath.

The patch [1] has tests. Have you tried to run them?
The tests *pass* at least on one system ;)

[1] http://bugs.python.org/review/16353/#ps12569

To run the tests, download the patch into your python checkout:

  $ curl -O \
  http://bugs.python.org/file36196/os.get_shell_executable.patch

and apply it:

  $ patch -p1< os.get_shell_executable.patch

to run only test_os.py:

  $ ./python -mtest test_os

> Lib/posixpath.py sets defpath=':/bin:/usr/bin' which is _not_ what
> getconf CS_PATH` returns on my Linux (the extra ':' at the beginning
> means "include the cwd" which is almost certainly not what anyone
> wants.) 

os.get_shell_executable() does not use cwd. Neither the documentation
nor the code in the patch suggest that.

> The hardcoded value '/bin:/usr/bin' would also be wrong for
> Solaris and AIX installations that are trying to be POSIX (I think
> they use /usr/xpg4/bin/sh).

os.defpath is the default search path used by exec*p* and spawn*p*
i.e., if os.defpath is incorrect for these system then os.exec*p* and
os.spawn*p* functions could also be broken.

I expect that Windows, OS X, Linux work as is. If the tests fail on
Solaris, AIX then os.defpath should be tweaked on these systems if it
hasn't been already. Note: os.defpath is a very conservative value: it
should be set at python's installation time at the very
latest. Henceforth it should remain the same.

> And Lib/ntpath.py sets defpath='.;C:\\bin', which doesn't resemble a
> path that even works (as pointed out in now closed
> http://bugs.python.org/issue5717).  (The correct value may be
> %SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;%SYSTEMROOT%\System32\WindowsPowerShell\v1.0\'.)

Please, look at the patch. Neither the documentation nor the code
suggest that os.defpath is used on Windows.

> So I don't know where to go next.  I'm happy to cook up a different
> patch, but I have no idea what it should be.  Here are some
> possibilities:
>
> 1. Kick the can down the road: file a new bug against os.defpath and
> (somehow) fix Lib/*path.py so they do something more reasonable in
> more cases.  One of which might be to have posixpath.py try to call
> os.confstr() (and then, perhaps, special-code something in
> Modules/posixmodule.c in the case that HAVE_CONFSTR is not defined.)
>
> 2. Modify @akira's patch to call os.confstr('CS_PATH') instead of
> os.defpath, and then deal with the fact that very few systems actually
> implement confstr() (perhaps by special-coding something in
> Modules/posixmodule.c as described above.)

Note I'm the author of http://bugs.python.org/issue16353#msg224514
message that references the POSIX recommendation about `getconf PATH`
and *explicitly* mentions os.confstr('CS_PATH').

I don't remember exactly the motivation to use os.defpath instead of
os.confstr('CS_PATH').

A guess: the result is the same (a lone `:` is ignored in the patch) but
os.defpath is easier to modify for uncommon systems where os.confstr
might not be available.

I expect that the tests in the patch will pass on all stable buildbots
[2] successfully without any modifications (with os.defpath as is).

[2] http://buildbot.python.org/all/waterfall?category=3.x.stable

Other systems should probably configure os.defpath appropriately (it
should include the path to the standard shell).

> 3. Modify this patch to fall back to `PATH` if `sh` can't be found on
> os.defpath (or os.confstr('CS_PATH') fails).

The standard shell should not depend on PATH envvar. The tests show that
os.get_shell_executable() works even with an empty environment. The
motivation is the same as why `getconf PATH` exists in the first place.

The documentation for os.get_shell_executable() (from the patch):

   Return a path to the standard system shell executable -- ``sh``
   program on POSIX (default: ``'/bin/sh'``) or ``%ComSpec%`` command
   processor on Windows (default: ``%SystemRoot%\system32\cmd.exe``).

   Availability: Unix, Windows

The intent is that os.get_shell_executable() returns the same *shell
path* as used by system(3) on POSIX i.e., (ignoring signals,
etc) os.system(command) could be implemented:

  subprocess.call(command, shell=True, executable=os.get_shell_executable())

(presumably subprocess can use get_shell_executable() internally instead
of /bin/sh but it is another issue)
msg230754 - (view) Author: Matt Frank (WanderingLogic) * Date: 2014-11-06 16:54
In msg174930 Christian Heimes (christian.heimes) wrote:
> I've tested confstr("CS_PATH") on Linux, Mac OS X, Solaris, HP-UX
> and BSD. It works and the path to `sh` is always included.

In msg230713 Ned Deily(ned.deily) wrote:
> ignore Lib/macpath.py.
> [...]
> OS X uses Lib/posixpath.py.

These two messages have convinced me that the correct approach is to kick the can down the road.  I will file a new issue and patch to fix os.defpath (by initializing os.defpath by calling confstr("CS_PATH")).  Then I will file a new issue and patch that will define a reasonable os.confstr() for platforms where HAVE_CONFSTR is not defined.
msg230755 - (view) Author: Matt Frank (WanderingLogic) * Date: 2014-11-06 18:25
In msg230720 Akira Li (akira) wrote:
> os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
> used on Linux) sysdeps/posix/spawni.c I don't know whether it is
> possible to change CS_PATH without recompiling every statically linked
> executable on a system, sysdeps/unix/confstr.h:
>
>   #define CS_PATH "/bin:/usr/bin"
>
> Though this issue is about the path to the standard shell sh/cmd.exe.
> It is not about os.defpath.

I understand this issue is about the path to the standard shell.
My argument is that os.defpath is the wrong tool for finding that
path, and os.confstr('CS_PATH') is the correct tool, as you originally
suggested in msg224514.

> The patch [1] has tests. Have you tried to run them?
> The tests *pass* at least on one system ;)
>
> [...]
>
> os.get_shell_executable() does not use cwd. Neither the documentation
> nor the code in the patch suggest that.

I ran the tests (and the entire Python test suite) before I wrote my
first message.  But you didn't test what happens when there is a bogus
'sh' somewhere along the path.  You also have a bug in your path
searching loop that interprets an empty element on the path as "/".
Here's the current failure mode:

Go to root on your Posix system.  With "su" or "sudo" create an
executable file named sh.  (for example "cd /; sudo touch sh; sudo
chmod +x sh").  Then go back to your python interpreter (with the
patch applied) and run "os.get_shell_executable()".  The returned
result will be '/sh'.

The loop _should_ be treating empty path elements as current working
directory.  (In the glibc sysdeps/posix/spawni.c file you pointed to
look around line 281, by the comment that says "/* Two adjacent
colons, or a colon at the beginning or the end of 'PATH' means to
search the current directory.*/")

But if you fix the loop to iterate over the path correctly (including
'cwd') then you will find that putting an executable named 'sh' in the
current working directory will make os.get_shell_executable() return
the "sh" in the current working directory (because os.defpath says it
should).

Note also, that glibc's spawni() uses two different "default paths".
One is for if the user didn't specify their PATH variable (that's the
one where they use ":/bin:/usr/bin") and a completely different (built
in path is used in the spawn*p* version) if it turns out that the file
is a script that needs to run under the system sh.  (On line 290 they
call maybe_script_execute(), which calls script_execute(), which uses
_PATH_BSHELL.)

> os.defpath is the default search path used by exec*p* and spawn*p*
> i.e., if os.defpath is incorrect for these system then os.exec*p* and
> os.spawn*p* functions could also be broken.

I'll look into it.

> I don't remember exactly the motivation to use os.defpath instead of
> os.confstr('CS_PATH').

The motivation was in msg224514.  You wrote:
> In the absense of os.confstr('CS_PATH') on Android (msg175006),
> os.defpath seems appropriate than os.environ['PATH'] to expand 'sh'
> basename to the full path to be used by subprocess later.

But os.defpath doesn't work on Android either (because it is hardcoded
to ':/bin:/usr/bin').
msg230761 - (view) Author: Akira Li (akira) * Date: 2014-11-06 20:18
> Matt Frank added the comment:
>
> In msg230720 Akira Li (akira) wrote:
>> os.defpath is supposed to be ':'+CS_PATH, e.g., look at glibc (C library
>> used on Linux) sysdeps/posix/spawni.c I don't know whether it is
>> possible to change CS_PATH without recompiling every statically linked
>> executable on a system, sysdeps/unix/confstr.h:
>>
>>   #define CS_PATH "/bin:/usr/bin"
>>
>> Though this issue is about the path to the standard shell sh/cmd.exe.
>> It is not about os.defpath.
>
> I understand this issue is about the path to the standard shell.
> My argument is that os.defpath is the wrong tool for finding that
> path, and os.confstr('CS_PATH') is the correct tool, as you originally
> suggested in msg224514.
>

I'll repeat os.defpath definition that I've cited in the same message
msg224514:

  The default search path used by exec*p* and spawn*p* if the
  environment doesn’t have a 'PATH' key. Also available via os.path.

1. os.defpath should work (contain the path to the standard shell)
   everywhere where os.confstr("CS_PATH") works.

2. And it might be easier to customize e.g., on systems where there is
   no os.confstr or where changing CS_PATH involves recompiling crucial
   system processes.

If these two statements are not true then os.defpath could be dropped.

>> The patch [1] has tests. Have you tried to run them?
>> The tests *pass* at least on one system ;)
>>
>> [...]
>>
>> os.get_shell_executable() does not use cwd. Neither the documentation
>> nor the code in the patch suggest that.
>
> I ran the tests (and the entire Python test suite) before I wrote my
> first message.  But you didn't test what happens when there is a bogus
> 'sh' somewhere along the path.  You also have a bug in your path
> searching loop that interprets an empty element on the path as "/".
> Here's the current failure mode:
>
> Go to root on your Posix system.  With "su" or "sudo" create an
> executable file named sh.  (for example "cd /; sudo touch sh; sudo
> chmod +x sh").  Then go back to your python interpreter (with the
> patch applied) and run "os.get_shell_executable()".  The returned
> result will be '/sh'.
> But if you fix the loop to iterate over the path correctly (including
> 'cwd') then you will find that putting an executable named 'sh' in the
> current working directory will make os.get_shell_executable() return
> the "sh" in the current working directory (because os.defpath says it
> should).
>

Comment in the patch explicitly says that '' is interpreted as '/'. The
intent is to exclude the current working directory, thinking that /sh
executable can create only root. And root can do anything to the machine
anyway.

I agree. It is a misfeature. I've uploaded a new patch that excludes ''
completely. The previous code tried to be too cute.

> The loop _should_ be treating empty path elements as current working
> directory.  (In the glibc sysdeps/posix/spawni.c file you pointed to
> look around line 281, by the comment that says "/* Two adjacent
> colons, or a colon at the beginning or the end of 'PATH' means to
> search the current directory.*/")

yes. Empty path element stands for the current working directory e.g.,

   $ PATH= python
   $ PATH=: python
   $ PATH=:/ python
   $ PATH=/: python

run ./python on my system.

The current working directory is intentionally excluded (in the original
and in the current patches) otherwise os.get_exec_path(env={}) would be
used.

> Note also, that glibc's spawni() uses two different "default paths".
> One is for if the user didn't specify their PATH variable (that's the
> one where they use ":/bin:/usr/bin") and a completely different (built
> in path is used in the spawn*p* version) if it turns out that the file
> is a script that needs to run under the system sh.  (On line 290 they
> call maybe_script_execute(), which calls script_execute(), which uses
> _PATH_BSHELL.)

glibc hardcodes the path (like subprocess module does currently):

  #define	_PATH_BSHELL	"/bin/sh"

os.get_shell_executable() is an enhancement that allows
(Unix) systems to configure the path via os.defpath.

> But os.defpath doesn't work on Android either (because it is hardcoded
> to ':/bin:/usr/bin').

As I've mentioned in msg224514 before posting my original patch:

- "Andriod uses /system/bin/sh"
- "os.defpath could be tweaked on Android"

i.e., yes. Default os.defpath doesn't work there and it should be
configured on Android to include the path to the standard shell.
msg266135 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-05-23 10:17
Following Serhiy suggestion at msg224477, this tentative patch adds the is_android attribute to the posix module and the default_shell attribute to the os module. No documentation for the moment. No test case is needed I think, as the changes in issue 16255 will test any solution found here.

An alternative to posix.is_android is sys.platform set to 'linux-android' on Android. These are the locations in the standard library that do not test for "if sys.platform.startswith('linux')":

Lib/distutils/tests/test_dist.py|386 col 16| if sys.platform in ('linux', 'darwin'):
Lib/test/test_logging.py|527 col 12| if sys.platform in ('linux', 'darwin'):
Lib/test/test_resource.py|135 col 26| @unittest.skipUnless(sys.platform == 'linux', 'test requires Linux')
Lib/test/test_socket.py|898 col 16| or sys.platform in ('linux', 'darwin')):
Lib/test/test_socket.py|2256 col 23| @skipWithClientIf(sys.platform not in {"linux"},
Lib/test/test_socket.py|4508 col 22| @unittest.skipUnless(sys.platform == 'linux', 'Linux specific test')
Lib/test/test_sysconfig.py|388 col 26| @unittest.skipUnless(sys.platform == 'linux', 'Linux-specific test')
msg270099 - (view) Author: Xavier de Gaye (xdegaye) * (Python committer) Date: 2016-07-10 16:30
Issue 27472 adds test.support.unix_shell as the path to the default shell.
History
Date User Action Args
2016-07-10 16:30:34xdegayesetmessages: + msg270099
2016-06-12 11:21:31christian.heimessetassignee: christian.heimes ->
2016-05-23 10:17:44xdegayesetfiles: + default_shell.patch
nosy: + xdegaye
messages: + msg266135

2016-04-27 10:39:10Roman.Evstifeevsetnosy: + Roman.Evstifeev
2016-04-26 16:04:41zach.warelinkissue26865 dependencies
2015-11-25 15:13:26Chi Hsuan Yensetnosy: + Chi Hsuan Yen
2015-02-23 14:38:36haypolinkissue23496 dependencies
2014-11-06 20:19:44akirasetfiles: + os.get_shell_executable-3.patch
2014-11-06 20:18:31akirasetmessages: + msg230761
2014-11-06 18:25:14WanderingLogicsetmessages: + msg230755
2014-11-06 16:54:29WanderingLogicsetmessages: + msg230754
2014-11-06 00:47:47akirasetmessages: + msg230720
2014-11-05 20:42:42ned.deilysetnosy: + ned.deily
messages: + msg230713
2014-11-05 20:01:15WanderingLogicsetmessages: + msg230711
2014-11-05 17:55:56WanderingLogicsetnosy: + WanderingLogic
2014-08-01 19:24:31akirasetfiles: + os.get_shell_executable.patch
2014-08-01 19:24:04akirasetfiles: - os.get_shell_executable.patch
2014-08-01 19:22:06akirasetversions: + Python 3.5, - Python 3.4
2014-08-01 19:11:54akirasetfiles: + os.get_shell_executable.patch
nosy: + akira
messages: + msg224514

2014-08-01 17:51:34eryksunsetmessages: + msg224512
2014-08-01 16:47:49serhiy.storchakasetmessages: + msg224509
2014-08-01 15:57:07eryksunsetnosy: + eryksun
messages: + msg224508
2014-08-01 09:31:16serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg224477
2013-11-17 14:10:25christian.heimessetassignee: christian.heimes
messages: + msg203160
stage: patch review
2013-10-24 14:16:55tim.goldensetnosy: - tim.golden
2013-08-19 16:52:03asvetlovlinkissue16255 dependencies
2012-11-06 20:00:46asvetlovsetmessages: + msg175006
2012-11-06 19:36:04lyapunsetmessages: + msg175003
2012-11-06 19:32:43asvetlovsetmessages: + msg175002
2012-11-06 13:35:58christian.heimessetmessages: + msg174962
2012-11-05 19:51:44chris.jerdoneksetmessages: + msg174935
2012-11-05 19:31:33chris.jerdoneksetmessages: + msg174933
2012-11-05 19:30:03lyapunsetfiles: + issue16353.diff

messages: + msg174932
2012-11-05 19:16:37christian.heimessetmessages: + msg174930
2012-11-05 18:48:18lyapunsetfiles: + issue16353.diff

messages: + msg174929
2012-11-05 10:25:49chris.jerdoneksetmessages: + msg174886
2012-11-05 09:32:04asvetlovsetmessages: + msg174885
2012-11-03 20:27:38eric.araujosetmessages: + msg174690
2012-11-03 20:21:36eric.araujosetnosy: + eric.araujo
messages: + msg174689
2012-11-03 17:07:43christian.heimessetmessages: + msg174652
2012-11-03 16:26:55chris.jerdoneksetmessages: + msg174646
2012-11-03 15:12:37r.david.murraysetmessages: + msg174631
2012-11-03 15:04:58asvetlovsetmessages: + msg174629
2012-11-03 14:51:10christian.heimessetmessages: + msg174628
2012-11-03 14:13:20chris.jerdoneksetmessages: + msg174624
2012-11-03 14:09:53christian.heimessetnosy: + christian.heimes
messages: + msg174623
2012-11-03 13:55:15lyapunsetfiles: + issue16353_added_doc_tag.diff

messages: + msg174621
2012-11-03 13:41:24asvetlovsetmessages: + msg174619
2012-11-03 11:28:44lyapunsetfiles: + issue16353.diff

nosy: + lyapun
messages: + msg174607

keywords: + patch
2012-11-02 21:34:13asvetlovsetmessages: + msg174586
2012-11-02 21:28:35r.david.murraysetnosy: + r.david.murray
messages: + msg174583
2012-11-02 21:25:49gregory.p.smithsetmessages: + msg174582
2012-11-02 21:17:59chris.jerdoneksetmessages: + msg174580
2012-11-02 21:07:20tim.goldensetnosy: + tim.golden
messages: + msg174578
2012-11-02 21:00:35asvetlovsetmessages: + msg174576
2012-11-02 20:53:40pitrousetmessages: + msg174574
2012-11-02 20:47:30chris.jerdoneksetmessages: + msg174572
2012-11-02 20:32:11pitrousetnosy: + pitrou, neologix, gregory.p.smith
messages: + msg174569
2012-11-02 18:58:59ezio.melottisetnosy: + ezio.melotti
2012-10-29 06:10:10chris.jerdoneksetnosy: + asvetlov
2012-10-29 01:19:20chris.jerdonekcreate