From 41b3dae552967439a737f1bfe436ae93e5650d87 Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Fri, 20 Nov 2020 18:08:39 -0800 Subject: [PATCH 1/7] Teach `poetry run` to find `.cmd` scripts on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Poetry script generation creates `.cmd` scripts, not `.exe` files, but `poetry run` did not know to look for `.cmd` scripts resulting in a `FileNotFoundError` when running custom scripts on Windows. This doesn’t reproduce with `poetry shell` because the executable search happens in the shell (like PowerShell). Because this function now needs to search across two suffixes on Windows (and this may increase in the future due to Windows’ use of file extensions to mark a file executable), it was refactored and the negative logic flipped to read more clearly: 1. Get the path to the bin in Poetry’s environment 2. On Windows, test if path.(exe, cmd) exists, and return it (handling the existing edge case with base paths). 3. If the path (without an extension) exists, return it 4. Return the original ‘bin’ if none of the above succeeded --- poetry/utils/env.py | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/poetry/utils/env.py b/poetry/utils/env.py index c247bf014f7..a37b7faa41c 100644 --- a/poetry/utils/env.py +++ b/poetry/utils/env.py @@ -1123,24 +1123,32 @@ def _bin(self, bin): # type: (str) -> str """ Return path to the given executable. """ - bin_path = (self._bin_dir / bin).with_suffix(".exe" if self._is_windows else "") - if not bin_path.exists(): - # On Windows, some executables can be in the base path - # This is especially true when installing Python with - # the official installer, where python.exe will be at - # the root of the env path. - # This is an edge case and should not be encountered - # in normal uses but this happens in the sonnet script - # that creates a fake virtual environment pointing to - # a base Python install. - if self._is_windows: - bin_path = (self._path / bin).with_suffix(".exe") - if bin_path.exists(): - return str(bin_path) - - return bin - - return str(bin_path) + bin_path = self._bin_dir / bin + + # Special case for Windows needing to find “.exe” and “.cmd” + # files first (not the extension-less file). + if self._is_windows: + for suffix in (".exe", ".cmd"): + win_bin_path = bin_path.with_suffix(suffix) + if win_bin_path.exists(): + return str(win_bin_path) + # On Windows, some executables can be in the base path + # This is especially true when installing Python with + # the official installer, where python.exe will be at + # the root of the env path. + # This is an edge case and should not be encountered + # in normal uses but this happens in the sonnet script + # that creates a fake virtual environment pointing to + # a base Python install. + win_bin_path = (self._path / bin).with_suffix(suffix) + if win_bin_path.exists(): + return str(win_bin_path) + + if bin_path.exists(): + return str(bin_path) + + # Return original path if our search failed. + return bin def __eq__(self, other): # type: (Env) -> bool return other.__class__ == self.__class__ and other.path == self.path From a60303bdd104d25ff62a03204320002646027ebd Mon Sep 17 00:00:00 2001 From: Huw Percival Date: Mon, 23 Nov 2020 18:03:27 -0800 Subject: [PATCH 2/7] Correctly use PATHEXT to on Windows for executable extensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the built-in Windows environment variable which lists all extensions that Windows recognizes as “executable” and should be used in place of our hard-coded list with exe and cmd. --- poetry/utils/env.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/poetry/utils/env.py b/poetry/utils/env.py index a37b7faa41c..5321e1c8868 100644 --- a/poetry/utils/env.py +++ b/poetry/utils/env.py @@ -1125,10 +1125,10 @@ def _bin(self, bin): # type: (str) -> str """ bin_path = self._bin_dir / bin - # Special case for Windows needing to find “.exe” and “.cmd” - # files first (not the extension-less file). + # Special case for Windows needing to find executable files + # first by extension (and not the extension-less file). if self._is_windows: - for suffix in (".exe", ".cmd"): + for suffix in os.environ.get("PATHEXT", ".exe").split(os.pathsep): win_bin_path = bin_path.with_suffix(suffix) if win_bin_path.exists(): return str(win_bin_path) From 57f6ef57c634a23a5ab53bbb332f81c0478f7bf4 Mon Sep 17 00:00:00 2001 From: Kevin Conley Date: Mon, 23 Nov 2020 18:16:34 -0800 Subject: [PATCH 3/7] Add test for poetry run on Windows fix --- tests/console/commands/test_run.py | 37 ++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/console/commands/test_run.py b/tests/console/commands/test_run.py index 351d869d1a9..7be4d7eeeb0 100644 --- a/tests/console/commands/test_run.py +++ b/tests/console/commands/test_run.py @@ -1,5 +1,7 @@ import pytest +from poetry.utils._compat import WINDOWS + @pytest.fixture def tester(command_tester_factory): @@ -14,3 +16,38 @@ def patches(mocker, env): def test_run_passes_all_args(tester, env): tester.execute("python -V") assert [["python", "-V"]] == env.executed + + +@pytest.mark.skipif( + not WINDOWS, reason="This test asserts Windows-specific compatibility", +) +def test_run_console_scripts_on_windows(tmp_venv, command_tester_factory): + """Test that `poetry run` on Windows finds console scripts. + + On Windows, Poetry installs console scripts of editable + dependencies by creating in the `Scripts/` directory both: + + 1. The Bash script one expects on Linux (an extension-less file), + with a shebang to launch Python, import the given module, and call + the given function. + + 2. A Batch script (with the `.cmd` file extension) which makes + this Bash script work on Windows by calling Python directly and + then executing the script from (1). + + This works because Windows programs (like the command prompt, + PowerShell, "run" box, `start`, `where`, etc.) know to append + extensions from the `PATHEXT` environment variable when looking + for named programs. This is the Windows version of `chmod +x`. + Sine Poetry is cross-platform, `poetry run` also needs to look for + programs with this algorithm, and so this is a regression test. + + This test asserts that you can a console script via `poetry run` + just by providing its name without the `.cmd` extension (the + common use case). + + """ + tester = command_tester_factory("run", environment=tmp_venv) + script = tmp_venv._bin_dir / "console_script.cmd" + script.write_text("exit 123") + assert tester.execute("console_script") == 123 From 5c08f6cb584f2a5943181646d9afa8539e3d0ea1 Mon Sep 17 00:00:00 2001 From: Huw Percival Date: Wed, 6 Jan 2021 15:32:10 +0000 Subject: [PATCH 4/7] updated to resolve problem scenarios --- poetry/utils/env.py | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/poetry/utils/env.py b/poetry/utils/env.py index 5321e1c8868..8d25b97a871 100644 --- a/poetry/utils/env.py +++ b/poetry/utils/env.py @@ -1125,24 +1125,39 @@ def _bin(self, bin): # type: (str) -> str """ bin_path = self._bin_dir / bin + def find_file(file, paths, suffixes): + """ + find file:str in paths:Iterable[Path or str], if file has no suffix + then all of suffixes:Iterable[str] will be checked + """ + file = Path(file) + if file.suffix: + suffixes = (file.suffix,) + for path in paths: + for suffix in suffixes: + found = Path(path) / file.with_suffix(suffix) + if found.exists(): + return found + return None + # Special case for Windows needing to find executable files # first by extension (and not the extension-less file). if self._is_windows: - for suffix in os.environ.get("PATHEXT", ".exe").split(os.pathsep): - win_bin_path = bin_path.with_suffix(suffix) - if win_bin_path.exists(): - return str(win_bin_path) - # On Windows, some executables can be in the base path - # This is especially true when installing Python with - # the official installer, where python.exe will be at - # the root of the env path. - # This is an edge case and should not be encountered - # in normal uses but this happens in the sonnet script - # that creates a fake virtual environment pointing to - # a base Python install. - win_bin_path = (self._path / bin).with_suffix(suffix) - if win_bin_path.exists(): - return str(win_bin_path) + suffixes = os.environ.get("PATHEXT", ".exe").split(os.pathsep) + # On Windows, some executables can be in the base path + # This is especially true when installing Python with + # the official installer, where python.exe will be at + # the root of the env path. + # This is an edge case and should not be encountered + # in normal uses but this happens in the sonnet script + # that creates a fake virtual environment pointing to + # a base Python install. + found = find_file(bin, (self._bin_dir, self._path), suffixes) + if not found: + # search system path + found = find_file(bin, os.environ["PATH"].split(os.pathsep), suffixes) + if found: + return str(found) if bin_path.exists(): return str(bin_path) From 8b37cea532835bed8c973fe06028966c9c2e9794 Mon Sep 17 00:00:00 2001 From: Huw Percival Date: Wed, 6 Jan 2021 15:55:37 +0000 Subject: [PATCH 5/7] update UT to cover case where files have the same name, different prefix --- tests/console/commands/test_run.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/console/commands/test_run.py b/tests/console/commands/test_run.py index 7be4d7eeeb0..8d6cedf119d 100644 --- a/tests/console/commands/test_run.py +++ b/tests/console/commands/test_run.py @@ -1,3 +1,5 @@ +import os + import pytest from poetry.utils._compat import WINDOWS @@ -47,7 +49,17 @@ def test_run_console_scripts_on_windows(tmp_venv, command_tester_factory): common use case). """ - tester = command_tester_factory("run", environment=tmp_venv) - script = tmp_venv._bin_dir / "console_script.cmd" - script.write_text("exit 123") - assert tester.execute("console_script") == 123 + path_ext_start = os.environ["PATHEXT"] + try: + os.environ["PATHEXT"] = ".BAT;.CMD" # ensure environ vars are deterministic + tester = command_tester_factory("run", environment=tmp_venv) + bat_script = tmp_venv._bin_dir / "console_script.bat" + cmd_script = tmp_venv._bin_dir / "console_script.cmd" + + cmd_script.write_text("exit 15") + bat_script.write_text("exit 30") + assert tester.execute("console_script") == 30 + assert tester.execute("console_script.bat") == 30 + assert tester.execute("console_script.cmd") == 15 + finally: + os.environ["PATHEXT"] = path_ext_start From 3c4040e8cfdb7618168ace37761e4d11e0ae8c76 Mon Sep 17 00:00:00 2001 From: Huw Percival Date: Wed, 6 Jan 2021 16:35:44 +0000 Subject: [PATCH 6/7] add test for scripts outside of the environment --- tests/console/commands/test_run.py | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/console/commands/test_run.py b/tests/console/commands/test_run.py index 8d6cedf119d..f437be9b3ce 100644 --- a/tests/console/commands/test_run.py +++ b/tests/console/commands/test_run.py @@ -1,8 +1,10 @@ import os +import tempfile import pytest from poetry.utils._compat import WINDOWS +from poetry.utils._compat import Path @pytest.fixture @@ -63,3 +65,33 @@ def test_run_console_scripts_on_windows(tmp_venv, command_tester_factory): assert tester.execute("console_script.cmd") == 15 finally: os.environ["PATHEXT"] = path_ext_start + + +@pytest.mark.skipif( + not WINDOWS, reason="This test asserts Windows-specific compatibility", +) +def test_script_external_to_env(tmp_venv, command_tester_factory): + """ + If a script exists on the path outside poetry, poetry run should + still work + """ + path_ext_start = os.environ["PATHEXT"] + path_at_start = os.environ["PATH"] + try: + os.environ["PATHEXT"] = ".BAT;.CMD" + tester = command_tester_factory("run", environment=tmp_venv) + + # create directory and add it to the PATH + with tempfile.TemporaryDirectory() as tmpdirname: + os.environ["PATH"] = path_at_start + os.pathsep + tmpdirname + + # add script to the new directory + script = Path(tmpdirname) / "console_script.cmd" + script.write_text("exit 15") + + # poetry run will find it as it searched the path + assert tester.execute("console_script") == 15 + + finally: + os.environ["PATHEXT"] = path_ext_start + os.environ["PATH"] = path_at_start From 3ccd884df149eaca2545349e0c5bbac6df81b964 Mon Sep 17 00:00:00 2001 From: Huw Percival Date: Fri, 8 Jan 2021 09:53:30 +0000 Subject: [PATCH 7/7] Refactor to use shutil.which as it is available in all supported python versions. Update tests to user mocker.patch rather than hacking my own patch. Update tests for the case where the script is in the current dir --- poetry/utils/env.py | 37 +++++----------- tests/console/commands/test_run.py | 71 +++++++++++++++++------------- 2 files changed, 53 insertions(+), 55 deletions(-) diff --git a/poetry/utils/env.py b/poetry/utils/env.py index 8d25b97a871..a358f106c1d 100644 --- a/poetry/utils/env.py +++ b/poetry/utils/env.py @@ -1123,27 +1123,7 @@ def _bin(self, bin): # type: (str) -> str """ Return path to the given executable. """ - bin_path = self._bin_dir / bin - - def find_file(file, paths, suffixes): - """ - find file:str in paths:Iterable[Path or str], if file has no suffix - then all of suffixes:Iterable[str] will be checked - """ - file = Path(file) - if file.suffix: - suffixes = (file.suffix,) - for path in paths: - for suffix in suffixes: - found = Path(path) / file.with_suffix(suffix) - if found.exists(): - return found - return None - - # Special case for Windows needing to find executable files - # first by extension (and not the extension-less file). if self._is_windows: - suffixes = os.environ.get("PATHEXT", ".exe").split(os.pathsep) # On Windows, some executables can be in the base path # This is especially true when installing Python with # the official installer, where python.exe will be at @@ -1152,12 +1132,19 @@ def find_file(file, paths, suffixes): # in normal uses but this happens in the sonnet script # that creates a fake virtual environment pointing to # a base Python install. - found = find_file(bin, (self._bin_dir, self._path), suffixes) - if not found: - # search system path - found = find_file(bin, os.environ["PATH"].split(os.pathsep), suffixes) + # On windows we also need to manually search the %PATH% + # as the OS won't do it for us. + # Note: shutil.which always searches the current + # directory first on windows, This is the behavior + # windows users will expect. + search_path = os.pathsep.join( + (str(self._bin_dir), str(self._path), os.environ["PATH"]) + ) + found = shutil.which(bin, path=search_path) if found: - return str(found) + return found + + bin_path = self._bin_dir / bin if bin_path.exists(): return str(bin_path) diff --git a/tests/console/commands/test_run.py b/tests/console/commands/test_run.py index f437be9b3ce..62e91fa18dc 100644 --- a/tests/console/commands/test_run.py +++ b/tests/console/commands/test_run.py @@ -25,7 +25,7 @@ def test_run_passes_all_args(tester, env): @pytest.mark.skipif( not WINDOWS, reason="This test asserts Windows-specific compatibility", ) -def test_run_console_scripts_on_windows(tmp_venv, command_tester_factory): +def test_run_console_scripts_on_windows(tmp_venv, command_tester_factory, mocker): """Test that `poetry run` on Windows finds console scripts. On Windows, Poetry installs console scripts of editable @@ -51,47 +51,58 @@ def test_run_console_scripts_on_windows(tmp_venv, command_tester_factory): common use case). """ - path_ext_start = os.environ["PATHEXT"] - try: - os.environ["PATHEXT"] = ".BAT;.CMD" # ensure environ vars are deterministic - tester = command_tester_factory("run", environment=tmp_venv) - bat_script = tmp_venv._bin_dir / "console_script.bat" - cmd_script = tmp_venv._bin_dir / "console_script.cmd" - - cmd_script.write_text("exit 15") - bat_script.write_text("exit 30") - assert tester.execute("console_script") == 30 - assert tester.execute("console_script.bat") == 30 - assert tester.execute("console_script.cmd") == 15 - finally: - os.environ["PATHEXT"] = path_ext_start + new_environ = {} + new_environ.update(os.environ) + new_environ["PATHEXT"] = ".BAT;.CMD" # ensure environ vars are deterministic + mocker.patch("os.environ", new_environ) + + tester = command_tester_factory("run", environment=tmp_venv) + bat_script = tmp_venv._bin_dir / "console_script.bat" + cmd_script = tmp_venv._bin_dir / "console_script.cmd" + + cmd_script.write_text("exit 15") + bat_script.write_text("exit 30") + assert tester.execute("console_script") == 30 + assert tester.execute("console_script.bat") == 30 + assert tester.execute("console_script.cmd") == 15 @pytest.mark.skipif( not WINDOWS, reason="This test asserts Windows-specific compatibility", ) -def test_script_external_to_env(tmp_venv, command_tester_factory): +def test_script_external_to_env(tmp_venv, command_tester_factory, mocker): """ - If a script exists on the path outside poetry, poetry run should - still work + If a script exists on the path outside poetry, or in the current directory, + poetry run should still work """ - path_ext_start = os.environ["PATHEXT"] - path_at_start = os.environ["PATH"] - try: - os.environ["PATHEXT"] = ".BAT;.CMD" - tester = command_tester_factory("run", environment=tmp_venv) + new_environ = {} + new_environ.update(os.environ) - # create directory and add it to the PATH - with tempfile.TemporaryDirectory() as tmpdirname: - os.environ["PATH"] = path_at_start + os.pathsep + tmpdirname + tester = command_tester_factory("run", environment=tmp_venv) + # create directory and add it to the PATH + with tempfile.TemporaryDirectory() as tmp_dir_name: + # add script to current directory + script_in_cur_dir = tempfile.NamedTemporaryFile( + "w", dir=".", suffix=".CMD", delete=False + ) + script_in_cur_dir.write("exit 30") + script_in_cur_dir.close() + + try: # add script to the new directory - script = Path(tmpdirname) / "console_script.cmd" + script = Path(tmp_dir_name) / "console_script.cmd" script.write_text("exit 15") + new_environ[ + "PATHEXT" + ] = ".BAT;.CMD" # ensure environ vars are deterministic + new_environ["PATH"] = os.environ["PATH"] + os.pathsep + tmp_dir_name + mocker.patch("os.environ", new_environ) + # poetry run will find it as it searched the path assert tester.execute("console_script") == 15 + assert tester.execute(script_in_cur_dir.name) == 30 - finally: - os.environ["PATHEXT"] = path_ext_start - os.environ["PATH"] = path_at_start + finally: + os.unlink(script_in_cur_dir.name)