From 228a3c99bdb2d02771bead66a0beabafad3a90d3 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 17 Apr 2019 16:26:36 +0200 Subject: [PATCH] bpo-35755: shutil.which() uses os.confstr("CS_PATH") (GH-12858) shutil.which() and distutils.spawn.find_executable() now use os.confstr("CS_PATH") if available instead of os.defpath, if the PATH environment variable is not set. Don't use os.confstr("CS_PATH") nor os.defpath if the PATH environment variable is set to an empty string to mimick Unix 'which' command behavior. Changes: * find_executable() now starts by checking for the executable in the current working directly case. Add an explicit "if not path: return None". * Add tests for PATH='' (empty string), PATH=':' and for PATHEXT. --- Lib/distutils/spawn.py | 39 +++++++---- Lib/distutils/tests/test_spawn.py | 47 ++++++++++++- Lib/shutil.py | 13 +++- Lib/test/test_shutil.py | 68 +++++++++++++++++++ .../2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst | 6 ++ 5 files changed, 155 insertions(+), 18 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst diff --git a/Lib/distutils/spawn.py b/Lib/distutils/spawn.py index 53876880932..888327270e2 100644 --- a/Lib/distutils/spawn.py +++ b/Lib/distutils/spawn.py @@ -172,21 +172,32 @@ def find_executable(executable, path=None): A string listing directories separated by 'os.pathsep'; defaults to os.environ['PATH']. Returns the complete filename or None if not found. """ - if path is None: - path = os.environ.get('PATH', os.defpath) - - paths = path.split(os.pathsep) - base, ext = os.path.splitext(executable) - + _, ext = os.path.splitext(executable) if (sys.platform == 'win32') and (ext != '.exe'): executable = executable + '.exe' - if not os.path.isfile(executable): - for p in paths: - f = os.path.join(p, executable) - if os.path.isfile(f): - # the file exists, we have a shot at spawn working - return f - return None - else: + if os.path.isfile(executable): return executable + + if path is None: + path = os.environ.get('PATH', None) + if path is None: + try: + path = os.confstr("CS_PATH") + except (AttributeError, ValueError): + # os.confstr() or CS_PATH is not available + path = os.defpath + # bpo-35755: Don't use os.defpath if the PATH environment variable is + # set to an empty string to mimick Unix which command behavior + + # PATH='' doesn't match, whereas PATH=':' looks in the current directory + if not path: + return None + + paths = path.split(os.pathsep) + for p in paths: + f = os.path.join(p, executable) + if os.path.isfile(f): + # the file exists, we have a shot at spawn working + return f + return None diff --git a/Lib/distutils/tests/test_spawn.py b/Lib/distutils/tests/test_spawn.py index 0d455385d8a..f9ae69ef86b 100644 --- a/Lib/distutils/tests/test_spawn.py +++ b/Lib/distutils/tests/test_spawn.py @@ -87,11 +87,52 @@ class SpawnTestCase(support.TempdirManager, rv = find_executable(dont_exist_program , path=tmp_dir) self.assertIsNone(rv) - # test os.defpath: missing PATH environment variable + # PATH='': no match, except in the current directory with test_support.EnvironmentVarGuard() as env: - with mock.patch('distutils.spawn.os.defpath', tmp_dir): - env.pop('PATH') + env['PATH'] = '' + with unittest.mock.patch('distutils.spawn.os.confstr', + return_value=tmp_dir, create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', + tmp_dir): + rv = find_executable(program) + self.assertIsNone(rv) + # look in current directory + with test_support.change_cwd(tmp_dir): + rv = find_executable(program) + self.assertEqual(rv, program) + + # PATH=':': explicitly looks in the current directory + with test_support.EnvironmentVarGuard() as env: + env['PATH'] = os.pathsep + with unittest.mock.patch('distutils.spawn.os.confstr', + return_value='', create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', ''): + rv = find_executable(program) + self.assertIsNone(rv) + + # look in current directory + with test_support.change_cwd(tmp_dir): + rv = find_executable(program) + self.assertEqual(rv, program) + + # missing PATH: test os.confstr("CS_PATH") and os.defpath + with test_support.EnvironmentVarGuard() as env: + env.pop('PATH', None) + + # without confstr + with unittest.mock.patch('distutils.spawn.os.confstr', + side_effect=ValueError, + create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', + tmp_dir): + rv = find_executable(program) + self.assertEqual(rv, filename) + + # with confstr + with unittest.mock.patch('distutils.spawn.os.confstr', + return_value=tmp_dir, create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', ''): rv = find_executable(program) self.assertEqual(rv, filename) diff --git a/Lib/shutil.py b/Lib/shutil.py index 7dd470dfaba..34df9cc4746 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -1309,9 +1309,20 @@ def which(cmd, mode=os.F_OK | os.X_OK, path=None): use_bytes = isinstance(cmd, bytes) if path is None: - path = os.environ.get("PATH", os.defpath) + path = os.environ.get("PATH", None) + if path is None: + try: + path = os.confstr("CS_PATH") + except (AttributeError, ValueError): + # os.confstr() or CS_PATH is not available + path = os.defpath + # bpo-35755: Don't use os.defpath if the PATH environment variable is + # set to an empty string to mimick Unix which command behavior + + # PATH='' doesn't match, whereas PATH=':' looks in the current directory if not path: return None + if use_bytes: path = os.fsencode(path) path = path.split(os.fsencode(os.pathsep)) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 678a190bcf5..e709a5661bf 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -1619,6 +1619,57 @@ class TestWhich(unittest.TestCase): rv = shutil.which(self.file) self.assertEqual(rv, self.temp_file.name) + def test_environ_path_empty(self): + # PATH='': no match + with support.EnvironmentVarGuard() as env: + env['PATH'] = '' + with unittest.mock.patch('os.confstr', return_value=self.dir, \ + create=True), \ + support.swap_attr(os, 'defpath', self.dir), \ + support.change_cwd(self.dir): + rv = shutil.which(self.file) + self.assertIsNone(rv) + + def test_environ_path_cwd(self): + expected_cwd = os.path.basename(self.temp_file.name) + if sys.platform == "win32": + curdir = os.curdir + if isinstance(expected_cwd, bytes): + curdir = os.fsencode(curdir) + expected_cwd = os.path.join(curdir, expected_cwd) + + # PATH=':': explicitly looks in the current directory + with support.EnvironmentVarGuard() as env: + env['PATH'] = os.pathsep + with unittest.mock.patch('os.confstr', return_value=self.dir, \ + create=True), \ + support.swap_attr(os, 'defpath', self.dir): + rv = shutil.which(self.file) + self.assertIsNone(rv) + + # look in current directory + with support.change_cwd(self.dir): + rv = shutil.which(self.file) + self.assertEqual(rv, expected_cwd) + + def test_environ_path_missing(self): + with support.EnvironmentVarGuard() as env: + env.pop('PATH', None) + + # without confstr + with unittest.mock.patch('os.confstr', side_effect=ValueError, \ + create=True), \ + support.swap_attr(os, 'defpath', self.dir): + rv = shutil.which(self.file) + self.assertEqual(rv, self.temp_file.name) + + # with confstr + with unittest.mock.patch('os.confstr', return_value=self.dir, \ + create=True), \ + support.swap_attr(os, 'defpath', ''): + rv = shutil.which(self.file) + self.assertEqual(rv, self.temp_file.name) + def test_empty_path(self): base_dir = os.path.dirname(self.dir) with support.change_cwd(path=self.dir), \ @@ -1633,6 +1684,23 @@ class TestWhich(unittest.TestCase): rv = shutil.which(self.file) self.assertIsNone(rv) + @unittest.skipUnless(sys.platform == "win32", 'test specific to Windows') + def test_pathext(self): + ext = ".xyz" + temp_filexyz = tempfile.NamedTemporaryFile(dir=self.temp_dir, + prefix="Tmp2", suffix=ext) + os.chmod(temp_filexyz.name, stat.S_IXUSR) + self.addCleanup(temp_filexyz.close) + + # strip path and extension + program = os.path.basename(temp_filexyz.name) + program = os.path.splitext(program)[0] + + with support.EnvironmentVarGuard() as env: + env['PATHEXT'] = ext + rv = shutil.which(program, path=self.temp_dir) + self.assertEqual(rv, temp_filexyz.name) + class TestWhichBytes(TestWhich): def setUp(self): diff --git a/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst b/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst new file mode 100644 index 00000000000..8e92ffdec23 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst @@ -0,0 +1,6 @@ +:func:`shutil.which` and :func:`distutils.spawn.find_executable` now use +``os.confstr("CS_PATH")`` if available instead of :data:`os.defpath`, if the +``PATH`` environment variable is not set. Moreover, don't use +``os.confstr("CS_PATH")`` nor :data:`os.defpath` if the ``PATH`` environment +variable is set to an empty string to mimick Unix ``which`` command +behavior.