From 4aa11e9cb3ef2296e1ad1904ae7143bda783a183 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Morin Date: Wed, 11 Oct 2023 20:43:42 -0400 Subject: [PATCH] Don't append system PATH to context when finding the python executables Signed-off-by: Jean-Christophe Morin --- src/rez_pip/cli.py | 2 +- src/rez_pip/rez.py | 11 +++-- tests/test_rez.py | 100 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 100 insertions(+), 13 deletions(-) diff --git a/src/rez_pip/cli.py b/src/rez_pip/cli.py index 537f6b1..cbadb66 100644 --- a/src/rez_pip/cli.py +++ b/src/rez_pip/cli.py @@ -177,7 +177,7 @@ def _run(args: argparse.Namespace, pipArgs: typing.List[str], pipWorkArea: str) args.packages, args.pip, pythonVersion, - pythonExecutable, + os.fspath(pythonExecutable), args.requirement or [], args.constraint or [], pipArgs, diff --git a/src/rez_pip/rez.py b/src/rez_pip/rez.py index 430f355..57a9558 100644 --- a/src/rez_pip/rez.py +++ b/src/rez_pip/rez.py @@ -226,7 +226,7 @@ def _convertMetadata( def getPythonExecutables( range_: typing.Optional[str], packageFamily: str = "python" -) -> typing.Dict[str, str]: +) -> typing.Dict[str, pathlib.Path]: """ Get the available python executable from rez packages. @@ -244,16 +244,19 @@ def getPythonExecutables( if range_ == "latest": packages = [packages[-1]] - pythons: typing.Dict[str, str] = {} + pythons: typing.Dict[str, pathlib.Path] = {} for package in packages: resolvedContext = rez.resolved_context.ResolvedContext( [f"{package.name}=={package.version}"] ) + # Make sure that system PATH doens't interfere with the "which" method. + resolvedContext.append_sys_path = False + for trimmedVersion in map(package.version.trim, [2, 1, 0]): - path = resolvedContext.which(f"python{trimmedVersion}") + path = resolvedContext.which(f"python{trimmedVersion}", parent_environ={}) if path: - pythons[str(package.version)] = path + pythons[str(package.version)] = pathlib.Path(path) break else: _LOG.warning( diff --git a/tests/test_rez.py b/tests/test_rez.py index 4dd0b44..78f2a26 100644 --- a/tests/test_rez.py +++ b/tests/test_rez.py @@ -1,5 +1,9 @@ +import os import sys +import stat import typing +import pathlib +import platform import unittest.mock import pytest @@ -179,28 +183,28 @@ def test_convertMetadata( ["1.0.0"], "latest", ["python1.0"], - {"1.0.0": "/path/python1.0"}, + {"1.0.0": pathlib.Path("/path/python1.0")}, id="latest", ), pytest.param( ["1.0.0"], None, ["python1.0"], - {"1.0.0": "/path/python1.0"}, + {"1.0.0": pathlib.Path("/path/python1.0")}, id="all-single-version", ), pytest.param( ["1.0.0"], None, ["", "python1"], - {"1.0.0": "/path/python1"}, + {"1.0.0": pathlib.Path("/path/python1")}, id="all-single-version-python1", ), pytest.param( ["1.0.0"], None, ["", "", "python"], - {"1.0.0": "/path/python"}, + {"1.0.0": pathlib.Path("/path/python")}, id="all-single-version-python-no-version", ), pytest.param( @@ -214,28 +218,31 @@ def test_convertMetadata( ["1.0.0", "2.0.0"], None, ["python1.0", "python2.0"], - {"1.0.0": "/path/python1.0", "2.0.0": "/path/python2.0"}, + { + "1.0.0": pathlib.Path("/path/python1.0"), + "2.0.0": pathlib.Path("/path/python2.0"), + }, id="all-multi-version", ), pytest.param( ["1.0.0", "2.0.0"], None, ["", "", "", "python2.0"], - {"2.0.0": "/path/python2.0"}, + {"2.0.0": pathlib.Path("/path/python2.0")}, id="all-multi-version-no-exe-1.0.0", ), pytest.param( ["1.0.0", "2.0.0"], "2+", ["python2.0"], - {"2.0.0": "/path/python2.0"}, + {"2.0.0": pathlib.Path("/path/python2.0")}, id="with-range-2plus", ), pytest.param( ["1.0.0", "2.0.0"], "<2", ["python1.0"], - {"1.0.0": "/path/python1.0"}, + {"1.0.0": pathlib.Path("/path/python1.0")}, id="with-range-less-than-2", ), ], @@ -275,3 +282,80 @@ def test_getPythonExecutables( rez_pip.rez.getPythonExecutables(range_, "python") == expectedExecutables ) + + +def test_getPythonExecutables_isolation( + monkeypatch: pytest.MonkeyPatch, + tmp_path: pathlib.Path, +) -> None: + """ + Test that getPythonExecutables is properly isolated. In other words, that + system wide PATH does not affect ResolvedContext.which calls. + + If ResolvedContext.append_sys_path is not False, this test should fail. + """ + packagePath = tmp_path / "package" + fakePath = tmp_path / "fake" + + escapedPath = os.fspath(packagePath).replace("\\", "\\\\") + # Create a fake python-1.0.0 package + repoData: typing.Dict[str, typing.Dict[str, typing.Dict[str, str]]] = { + "python": { + "1.0.0": { + "version": "1.0.0", + "commands": f"env.PATH.prepend('{escapedPath}')", + }, + } + } + + repo = typing.cast( + rez.package_repository.PackageRepository, + rez.package_repository.create_memory_package_repository(repoData), + ) + + with monkeypatch.context() as context: + context.setitem( + rez.package_repository.package_repository_manager.repositories, + f"memory@{repo.location}", + repo, + ) + + context.setattr(rez.config.config, "packages_path", [f"memory@{repo.location}"]) + + context.setenv("PATH", os.fspath(fakePath) + os.pathsep + os.environ["PATH"]) + + ext = ".bat" if platform.system() == "Windows" else "" + packagePath.mkdir() + + # Create the python executable in the python package. + # Note that we use python1 here and we will use python1.0 + # in the fake python install so that the fake would have hiher priority + # based on the algorithm in getPythonExecutables. + with open(packagePath / f"python1{ext}", "w") as f: + f.write("#FAKE EXECUTABLE") + + if platform.system() != "Windows": + os.chmod(packagePath / f"python1{ext}", stat.S_IRUSR | stat.S_IXUSR) + + fakePath.mkdir() + + # Now create the fake executable. + with open(fakePath / f"python1.0{ext}", "w") as f: + f.write("#FAKE EXECUTABLE") + + if platform.system() != "Windows": + os.chmod(fakePath / f"python1.0{ext}", stat.S_IRUSR | stat.S_IXUSR) + + def mockedFunc(self, *args, **kwargs): + self.env.PATH.append(fakePath) + + # Mock append_system_paths to force our fake path into the + # injected sys paths. If we don't do this, the fake path will not + # be set in the ResolvedContext when append_system_paths is True. + with unittest.mock.patch( + "rez.rex.RexExecutor.append_system_paths", + new=mockedFunc, + ): + assert rez_pip.rez.getPythonExecutables("1.0.0", "python") == { + "1.0.0": packagePath / f"python1{ext}" + }