From 74deb20a3e2498c7d353e969f6f3ae49bc16a576 Mon Sep 17 00:00:00 2001 From: Johan Herland Date: Sat, 20 Jan 2024 01:27:12 +0100 Subject: [PATCH] test_real_projects: Clean up how we create and populate venvs This fixes issue #406. Instead of generating lines of a shell script that need to be run with shell=True in subprocess.run(), generate a sequence of argv-style commands. These are safer (no shell involved) and more easily portable to Windows (for one, our quoting of command line args on Windows was dubious, at best...). Also move the first and last commands in the previous shell script (i.e. the preliminary removal of venv_dir, and the creation of the venv_dir/.installed marker file) out of venv_script_lines() and into the __call__() method. Since these are no longer part of the generated commands, they are also no longer part of the venv_hash(). If we make changes to these two steps (without making any other changes that are captured by venv_hash(), we will end up reusing a cache entry. If this is problematic, then some other modification to change the venv_hash() must be added at the same time. --- tests/project_helpers.py | 99 ++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 59 deletions(-) diff --git a/tests/project_helpers.py b/tests/project_helpers.py index 5de68480..7b3b58c7 100644 --- a/tests/project_helpers.py +++ b/tests/project_helpers.py @@ -4,7 +4,7 @@ import hashlib import logging import os -import shlex +import shutil import subprocess import sys from abc import ABC, abstractmethod @@ -129,81 +129,62 @@ class CachedExperimentVenv: requirements: List[str] # PEP 508 requirements, passed to 'pip install' - def venv_script_lines(self, venv_path: Path) -> List[str]: - if sys.platform.startswith("win"): - pip_path = venv_path / "Scripts" / "pip.exe" - python_path = venv_path / "Scripts" / "python.exe" - return ( - [ - f"rd /s /q {venv_path}", - f"{sys.executable} -m venv {venv_path}", - f"{python_path} -m pip install --upgrade pip", - ] - + [ - f'{python_path} -m pip install --no-deps "{req}"' - for req in self.requirements - ] - + [ - f"type nul > {venv_path / '.installed'}", - ] - ) - - pip_path = venv_path / "bin" / "pip" - return ( - [ - f"rm -rf {venv_path}", - f"{sys.executable} -m venv {venv_path}", - f"{pip_path} install --upgrade pip", - ] - + [ - f"{pip_path} install --no-deps {shlex.quote(req)}" - for req in self.requirements - ] - + [ - f"touch {venv_path / '.installed'}", - ] - ) + def venv_commands( + self, venv_path: Path, python_exe: Optional[str] = None + ) -> Iterator[List[str]]: + """Yield commands to run in order to create and populate the given venv. + + The commands are yielded as argv-style lists of strings. The commands + must be run in sequence, and each command must return successfully in + order for the venv to be considered successfully created. + """ + if python_exe is None: # Default to current Python executable + python_exe = sys.executable + + if sys.platform.startswith("win"): # Windows + venv_python = str(venv_path / "Scripts" / "python.exe") + else: # Assume POSIX + venv_python = str(venv_path / "bin" / "python") + + yield [python_exe, "-m", "venv", str(venv_path)] + yield [venv_python, "-m", "pip", "install", "--upgrade", "pip"] + for req in self.requirements: + yield [venv_python, "-m", "pip", "install", "--no-deps", req] def venv_hash(self) -> str: - """Returns a hash that depends on the venv script and python version. + """Returns a hash that depends on the venv script and Python version. - The installation script will change if the code to setup the venv in - venv_script_lines() changes, or if the requirements of the experiment + The venv script will change if the code to setup the venv in + venv_commands() changes, or if the requirements of the experiment changes. It will also be different for different Python versions. - The Python version currently used to run the tests is used to compute - the hash and create the venv. """ - dummy_script = self.venv_script_lines(Path(os.devnull)) - py_version = f"{sys.version_info.major},{sys.version_info.minor}" - script_and_version_bytes = ("".join(dummy_script) + py_version).encode() - return hashlib.sha256(script_and_version_bytes).hexdigest() + dummy_script = [ + " ".join(argv) for argv in self.venv_commands(Path(os.devnull), "python") + ] + [f"# Python {sys.version_info.major}.{sys.version_info.minor}"] + return hashlib.sha256("\n".join(dummy_script).encode()).hexdigest() def __call__(self, cache: pytest.Cache) -> Path: - """Get this venv's dir and create it if necessary. + """Get this venv's dir from cache, or create it if necessary. The venv_dir is where we install the dependencies of the current - experiment. It is keyed by the sha256 checksum of the requirements - file and the script we use for setting up the venv. This way, we - don't risk using a previously cached venv for a different if the - script or the requirements to create that venv change. + experiment. It is keyed by the sha256 checksum of the commands we use + to create and populate the venv with this experiment's requirements. + This way, we don't risk reusing a previously cached venv if the commands + or the requirements has changed. """ - # We cache venv dirs using the hash from create_venv_hash + # We cache venv dirs using the hash from create_venv_hash() as key cached_str = cache.get(str(Path("fawltydeps", self.venv_hash())), None) if cached_str is not None and Path(cached_str, ".installed").is_file(): return Path(cached_str) # already cached - # Must run the script to set up the venv + # Must run the commands to set up the venv venv_dir = Path(cache.mkdir(f"fawltydeps_venv_{self.venv_hash()}")) logger.info(f"Creating venv at {venv_dir}...") - venv_script = self.venv_script_lines(venv_dir) - subprocess.run( - " && ".join(venv_script), - check=True, # fail if any of the commands fail - shell=True, # pass multiple shell commands to the subprocess - ) - # Make sure the venv has been installed - assert (venv_dir / ".installed").is_file() + shutil.rmtree(venv_dir) # start from a clean slate + for argv in self.venv_commands(venv_dir): + subprocess.run(argv, check=True) # fail if any of the commands fail + (venv_dir / ".installed").touch() # touch install marker on no errors cache.set(str(Path("fawltydeps", self.venv_hash())), str(venv_dir)) return venv_dir