From 5b16309b26d4045a41c44f59e3580798aff096b9 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Thu, 2 Mar 2023 21:49:56 +0100 Subject: [PATCH 01/16] Adjusted Chef.prepare() to allways write to a temporary directory. Otherwise we would overwrite an already existing cached wheel. This could then make the installation fail due to missmatched hashes. Changed Executor._download_link() to validate package hashes via _populate_hashes_dict before building a new wheel incase we use a non wheel archive. --- src/poetry/installation/chef.py | 16 ++++++---------- src/poetry/installation/executor.py | 11 ++++++++--- tests/installation/test_chef.py | 6 +++++- tests/installation/test_executor.py | 2 +- 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index 0115e439200..9fcd06c53e9 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -86,6 +86,8 @@ def install(self, requirements: Collection[str]) -> None: class Chef: + tmp_dir_prefix = "poetry-chef-" + def __init__(self, config: Config, env: Env, pool: RepositoryPool) -> None: self._env = env self._pool = pool @@ -93,18 +95,15 @@ def __init__(self, config: Config, env: Env, pool: RepositoryPool) -> None: Path(config.get("cache-dir")).expanduser().joinpath("artifacts") ) - def prepare( - self, archive: Path, output_dir: Path | None = None, *, editable: bool = False - ) -> Path: + def prepare(self, archive: Path, *, editable: bool = False) -> Path: if not self._should_prepare(archive): return archive + tmp_dir = tempfile.mkdtemp(prefix=self.tmp_dir_prefix) if archive.is_dir(): - tmp_dir = tempfile.mkdtemp(prefix="poetry-chef-") - return self._prepare(archive, Path(tmp_dir), editable=editable) - return self._prepare_sdist(archive, destination=output_dir) + return self._prepare_sdist(archive, destination=Path(tmp_dir)) def _prepare( self, directory: Path, destination: Path, *, editable: bool = False @@ -154,7 +153,7 @@ def _prepare( return path - def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path: + def _prepare_sdist(self, archive: Path, destination: Path) -> Path: from poetry.core.packages.utils.link import Link suffix = archive.suffix @@ -181,9 +180,6 @@ def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path if not sdist_dir.is_dir(): sdist_dir = archive_dir - if destination is None: - destination = self.get_cache_directory_for_link(Link(archive.as_uri())) - destination.mkdir(parents=True, exist_ok=True) return self._prepare( diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index 22972882fcb..cfce4ac1cf8 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -5,6 +5,7 @@ import itertools import json import os +import tempfile import threading from concurrent.futures import ThreadPoolExecutor @@ -514,6 +515,10 @@ def _install(self, operation: Install | Update) -> int: else: archive = self._download(operation) + tmp_path_prefix = str(Path(tempfile.gettempdir(), self._chef.tmp_dir_prefix)) + if str(archive).startswith(tmp_path_prefix): + cleanup_archive = True + operation_message = self.get_operation_message(operation) message = ( f" • {operation_message}:" @@ -708,6 +713,8 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: raise + self._populate_hashes_dict(archive, package) + if archive.suffix != ".whl": message = ( f" • {self.get_operation_message(operation)}:" @@ -715,9 +722,7 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: ) self._write(operation, message) - archive = self._chef.prepare(archive, output_dir=output_dir) - - self._populate_hashes_dict(archive, package) + archive = self._chef.prepare(archive) return archive diff --git a/tests/installation/test_chef.py b/tests/installation/test_chef.py index e9046c5433f..1603ccc51e3 100644 --- a/tests/installation/test_chef.py +++ b/tests/installation/test_chef.py @@ -1,5 +1,6 @@ from __future__ import annotations +import tempfile from pathlib import Path from typing import TYPE_CHECKING from zipfile import ZipFile @@ -140,7 +141,10 @@ def test_prepare_sdist(config: Config, config_cache_dir: Path) -> None: wheel = chef.prepare(archive) - assert wheel.parent == destination + assert wheel.parent != destination + + tmp_file_prefix = str(Path(tempfile.gettempdir(), chef.tmp_dir_prefix)) + assert str(wheel.parent).startswith(tmp_file_prefix) assert wheel.name == "demo-0.1.0-py3-none-any.whl" diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 34f397e6bd4..01640a21433 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -63,7 +63,7 @@ def set_sdist_wheel(self, wheels: Path | list[Path]) -> None: self._sdist_wheels = wheels - def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path: + def _prepare_sdist(self, archive: Path, destination: Path) -> Path: if self._sdist_wheels is not None: wheel = self._sdist_wheels.pop(0) self._sdist_wheels.append(wheel) From cb35709ff45986dfe880dc305120e0f68013f01d Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Thu, 2 Mar 2023 22:38:00 +0100 Subject: [PATCH 02/16] Fixed formatting and linting issues --- src/poetry/installation/chef.py | 2 -- src/poetry/installation/executor.py | 1 - tests/installation/test_chef.py | 1 + 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index 9fcd06c53e9..ed1c97c8017 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -154,8 +154,6 @@ def _prepare( return path def _prepare_sdist(self, archive: Path, destination: Path) -> Path: - from poetry.core.packages.utils.link import Link - suffix = archive.suffix context: Callable[ [str], AbstractContextManager[zipfile.ZipFile | tarfile.TarFile] diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index cfce4ac1cf8..b6bd5ab0e19 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -697,7 +697,6 @@ def _download(self, operation: Install | Update) -> Path: def _download_link(self, operation: Install | Update, link: Link) -> Path: package = operation.package - output_dir = self._chef.get_cache_directory_for_link(link) archive = self._chef.get_cached_archive_for_link(link) if archive is None: # No cached distributions was found, so we download and prepare it diff --git a/tests/installation/test_chef.py b/tests/installation/test_chef.py index 1603ccc51e3..126ba625bf4 100644 --- a/tests/installation/test_chef.py +++ b/tests/installation/test_chef.py @@ -1,6 +1,7 @@ from __future__ import annotations import tempfile + from pathlib import Path from typing import TYPE_CHECKING from zipfile import ZipFile From cead0bbb0340066360062ac81376487a2afa5170 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Sat, 4 Mar 2023 22:33:36 +0100 Subject: [PATCH 03/16] First solution with broken test --- src/poetry/installation/chef.py | 15 +++-- src/poetry/installation/executor.py | 26 +++++++-- tests/installation/test_chef.py | 7 +-- tests/installation/test_executor.py | 87 +++++++++++++++++++++++++++-- 4 files changed, 113 insertions(+), 22 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index ed1c97c8017..7ba51b1e64a 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -95,15 +95,17 @@ def __init__(self, config: Config, env: Env, pool: RepositoryPool) -> None: Path(config.get("cache-dir")).expanduser().joinpath("artifacts") ) - def prepare(self, archive: Path, *, editable: bool = False) -> Path: + def prepare( + self, archive: Path, output_dir: Path | None = None, *, editable: bool = False + ) -> Path: if not self._should_prepare(archive): return archive - tmp_dir = tempfile.mkdtemp(prefix=self.tmp_dir_prefix) if archive.is_dir(): + tmp_dir = tempfile.mkdtemp(prefix=self.tmp_dir_prefix) return self._prepare(archive, Path(tmp_dir), editable=editable) - return self._prepare_sdist(archive, destination=Path(tmp_dir)) + return self._prepare_sdist(archive, destination=output_dir) def _prepare( self, directory: Path, destination: Path, *, editable: bool = False @@ -153,7 +155,9 @@ def _prepare( return path - def _prepare_sdist(self, archive: Path, destination: Path) -> Path: + def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path: + from poetry.core.packages.utils.link import Link + suffix = archive.suffix context: Callable[ [str], AbstractContextManager[zipfile.ZipFile | tarfile.TarFile] @@ -178,6 +182,9 @@ def _prepare_sdist(self, archive: Path, destination: Path) -> Path: if not sdist_dir.is_dir(): sdist_dir = archive_dir + if destination is None: + destination = self.get_cache_directory_for_link(Link(archive.as_uri())) + destination.mkdir(parents=True, exist_ok=True) return self._prepare( diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index b6bd5ab0e19..458e350b00c 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -697,6 +697,7 @@ def _download(self, operation: Install | Update) -> Path: def _download_link(self, operation: Install | Update, link: Link) -> Path: package = operation.package + output_dir = self._chef.get_cache_directory_for_link(link) archive = self._chef.get_cached_archive_for_link(link) if archive is None: # No cached distributions was found, so we download and prepare it @@ -712,7 +713,10 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: raise - self._populate_hashes_dict(archive, package) + # validate the original (downloaded or cached) archive only if we can. e.g. if + # we use the cached wheel or the actual downloaded archive + if package.files and link.filename == archive.name: + self._validate_archive_hash(archive, package) if archive.suffix != ".whl": message = ( @@ -721,21 +725,31 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: ) self._write(operation, message) - archive = self._chef.prepare(archive) + archive = self._chef.prepare(archive, output_dir=output_dir) + + elif link.is_wheel: + self._populate_hashes_dict( + archive, package, validate=False + ) # already validated return archive - def _populate_hashes_dict(self, archive: Path, package: Package) -> None: + def _populate_hashes_dict( + self, archive: Path, package: Package, validate=True + ) -> None: if package.files and archive.name in {f["file"] for f in package.files}: - archive_hash = self._validate_archive_hash(archive, package) + if validate: + archive_hash = self._validate_archive_hash(archive, package) + else: + archive_hash: str = "sha256:" + get_file_hash(archive) self._hashes[package.name] = archive_hash @staticmethod def _validate_archive_hash(archive: Path, package: Package) -> str: archive_hash: str = "sha256:" + get_file_hash(archive) - known_hashes = {f["hash"] for f in package.files} + known_hashes = {f["hash"] for f in package.files if f["file"] == archive.name} - if archive_hash not in known_hashes: + if known_hashes and archive_hash not in known_hashes: raise RuntimeError( f"Hash for {package} from archive {archive.name} not found in" f" known hashes (was: {archive_hash})" diff --git a/tests/installation/test_chef.py b/tests/installation/test_chef.py index 126ba625bf4..e9046c5433f 100644 --- a/tests/installation/test_chef.py +++ b/tests/installation/test_chef.py @@ -1,7 +1,5 @@ from __future__ import annotations -import tempfile - from pathlib import Path from typing import TYPE_CHECKING from zipfile import ZipFile @@ -142,10 +140,7 @@ def test_prepare_sdist(config: Config, config_cache_dir: Path) -> None: wheel = chef.prepare(archive) - assert wheel.parent != destination - - tmp_file_prefix = str(Path(tempfile.gettempdir(), chef.tmp_dir_prefix)) - assert str(wheel.parent).startswith(tmp_file_prefix) + assert wheel.parent == destination assert wheel.name == "demo-0.1.0-py3-none-any.whl" diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 01640a21433..c381a5a60b1 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -63,7 +63,7 @@ def set_sdist_wheel(self, wheels: Path | list[Path]) -> None: self._sdist_wheels = wheels - def _prepare_sdist(self, archive: Path, destination: Path) -> Path: + def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path: if self._sdist_wheels is not None: wheel = self._sdist_wheels.pop(0) self._sdist_wheels.append(wheel) @@ -139,9 +139,14 @@ def callback( ) if not fixture.exists(): - fixture = Path(__file__).parent.parent.joinpath( - "fixtures/distributions/demo-0.1.0-py2.py3-none-any.whl" - ) + if name == "demo-0.1.0.tar.gz": + fixture = Path(__file__).parent.parent.joinpath( + "fixtures/distributions/demo-0.1.0.tar.gz" + ) + else: + fixture = Path(__file__).parent.parent.joinpath( + "fixtures/distributions/demo-0.1.0-py2.py3-none-any.whl" + ) return [200, headers, fixture.read_bytes()] @@ -611,7 +616,7 @@ def test_executor_should_not_write_pep610_url_references_for_cached_package( verify_installed_distribution(tmp_venv, package) -def test_executor_should_write_pep610_url_references_for_files( +def test_executor_should_write_pep610_url_references_for_wheel_files( tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, io: BufferedIO ): url = ( @@ -645,6 +650,40 @@ def test_executor_should_write_pep610_url_references_for_files( verify_installed_distribution(tmp_venv, package, expected_url_reference) +def test_executor_should_write_pep610_url_references_for_non_wheel_files( + tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, io: BufferedIO +): + url = ( + Path(__file__) + .parent.parent.joinpath( + "fixtures/distributions/demo-0.1.0.tar.gz" + ) + .resolve() + ) + package = Package("demo", "0.1.0", source_type="file", source_url=url.as_posix()) + # Set package.files so the executor will attempt to hash the package + package.files = [ + { + "file": "demo-0.1.0.tar.gz", + "hash": "sha256:9fa123ad707a5c6c944743bf3e11a0e80d86cb518d3cf25320866ca3ef43e2ad", # noqa: E501 + } + ] + + executor = Executor(tmp_venv, pool, config, io) + executor.execute([Install(package)]) + expected_url_reference = { + "archive_info": { + "hashes": { + "sha256": ( + "9fa123ad707a5c6c944743bf3e11a0e80d86cb518d3cf25320866ca3ef43e2ad" + ) + }, + }, + "url": url.as_uri(), + } + verify_installed_distribution(tmp_venv, package, expected_url_reference) + + def test_executor_should_write_pep610_url_references_for_directories( tmp_venv: VirtualEnv, pool: RepositoryPool, @@ -703,7 +742,7 @@ def test_executor_should_write_pep610_url_references_for_editable_directories( ) -def test_executor_should_write_pep610_url_references_for_urls( +def test_executor_should_write_pep610_url_references_for_wheel_urls( tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, @@ -739,6 +778,42 @@ def test_executor_should_write_pep610_url_references_for_urls( verify_installed_distribution(tmp_venv, package, expected_url_reference) +def test_executor_should_not_write_pep610_url_references_for_non_wheel_urls( + tmp_venv: VirtualEnv, + pool: RepositoryPool, + config: Config, + io: BufferedIO, + mock_file_downloads: None, +): + package = Package( + "demo", + "0.1.0", + source_type="url", + source_url="https://files.pythonhosted.org/demo-0.1.0.tar.gz", + ) + # Set package.files so the executor will attempt to hash the package + package.files = [ + { + "file": "demo-0.1.0.tar.gz", + "hash": "sha256:9fa123ad707a5c6c944743bf3e11a0e80d86cb518d3cf25320866ca3ef43e2ad", # noqa: E501 + } + ] + + executor = Executor(tmp_venv, pool, config, io) + executor.execute([Install(package)]) + expected_url_reference = { + "archive_info": { + "hashes": { + "sha256": ( + "9fa123ad707a5c6c944743bf3e11a0e80d86cb518d3cf25320866ca3ef43e2ad" + ) + }, + }, + "url": package.source_url, + } + verify_installed_distribution(tmp_venv, package, expected_url_reference) + + def test_executor_should_write_pep610_url_references_for_git( tmp_venv: VirtualEnv, pool: RepositoryPool, From 65c36b20c8cb56176da541bcfb0da1cd744686f0 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Sat, 4 Mar 2023 23:37:58 +0100 Subject: [PATCH 04/16] Fix the test by also providing the non_wheel archive hash --- src/poetry/installation/executor.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index 458e350b00c..a1b20aa23f5 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -716,7 +716,7 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: # validate the original (downloaded or cached) archive only if we can. e.g. if # we use the cached wheel or the actual downloaded archive if package.files and link.filename == archive.name: - self._validate_archive_hash(archive, package) + self._populate_hashes_dict(archive, package) if archive.suffix != ".whl": message = ( @@ -727,11 +727,6 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: archive = self._chef.prepare(archive, output_dir=output_dir) - elif link.is_wheel: - self._populate_hashes_dict( - archive, package, validate=False - ) # already validated - return archive def _populate_hashes_dict( From 6143b4e7b2fc63a85bd069a17959d009291ac729 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Sat, 4 Mar 2023 23:40:37 +0100 Subject: [PATCH 05/16] remove validate flag for now --- src/poetry/installation/executor.py | 9 ++------- tests/installation/test_executor.py | 16 +++++++--------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index a1b20aa23f5..e69a376ce1f 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -729,14 +729,9 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: return archive - def _populate_hashes_dict( - self, archive: Path, package: Package, validate=True - ) -> None: + def _populate_hashes_dict(self, archive: Path, package: Package) -> None: if package.files and archive.name in {f["file"] for f in package.files}: - if validate: - archive_hash = self._validate_archive_hash(archive, package) - else: - archive_hash: str = "sha256:" + get_file_hash(archive) + archive_hash = self._validate_archive_hash(archive, package) self._hashes[package.name] = archive_hash @staticmethod diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index c381a5a60b1..fe0ce46cb44 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -651,13 +651,11 @@ def test_executor_should_write_pep610_url_references_for_wheel_files( def test_executor_should_write_pep610_url_references_for_non_wheel_files( - tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, io: BufferedIO + tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, io: BufferedIO ): url = ( Path(__file__) - .parent.parent.joinpath( - "fixtures/distributions/demo-0.1.0.tar.gz" - ) + .parent.parent.joinpath("fixtures/distributions/demo-0.1.0.tar.gz") .resolve() ) package = Package("demo", "0.1.0", source_type="file", source_url=url.as_posix()) @@ -779,11 +777,11 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( def test_executor_should_not_write_pep610_url_references_for_non_wheel_urls( - tmp_venv: VirtualEnv, - pool: RepositoryPool, - config: Config, - io: BufferedIO, - mock_file_downloads: None, + tmp_venv: VirtualEnv, + pool: RepositoryPool, + config: Config, + io: BufferedIO, + mock_file_downloads: None, ): package = Package( "demo", From 87181dcc585e9d493361fcf8796b27d0f910e9ac Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Sun, 5 Mar 2023 23:26:24 +0100 Subject: [PATCH 06/16] Implement review suggestions to also make add the hash for cached non wheel archives --- src/poetry/installation/chef.py | 9 +++- src/poetry/installation/executor.py | 16 +++---- tests/installation/test_chef.py | 68 +++++++++++++++++++++++++++-- tests/installation/test_executor.py | 24 +++++++++- 4 files changed, 102 insertions(+), 15 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index 7ba51b1e64a..f866dd3946a 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -199,13 +199,20 @@ def _should_prepare(self, archive: Path) -> bool: def _is_wheel(cls, archive: Path) -> bool: return archive.suffix == ".whl" - def get_cached_archive_for_link(self, link: Link) -> Path | None: + def get_cached_archive_for_link(self, link: Link, strict: bool) -> Path | None: archives = self.get_cached_archives_for_link(link) if not archives: return None candidates: list[tuple[float | None, Path]] = [] for archive in archives: + if strict: + # in strict mode return the original cached archive instead of the + # prioritized archive type. + if link.filename == archive.name: + return archive + else: + continue if archive.suffix != ".whl": candidates.append((float("inf"), archive)) continue diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index e69a376ce1f..2972ec72a5a 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -698,11 +698,11 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: package = operation.package output_dir = self._chef.get_cache_directory_for_link(link) - archive = self._chef.get_cached_archive_for_link(link) - if archive is None: + original_archive = self._chef.get_cached_archive_for_link(link, strict=True) + if original_archive is None: # No cached distributions was found, so we download and prepare it try: - archive = self._download_archive(operation, link) + original_archive = self._download_archive(operation, link) except BaseException: cache_directory = self._chef.get_cache_directory_for_link(link) cached_file = cache_directory.joinpath(link.filename) @@ -713,10 +713,8 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: raise - # validate the original (downloaded or cached) archive only if we can. e.g. if - # we use the cached wheel or the actual downloaded archive - if package.files and link.filename == archive.name: - self._populate_hashes_dict(archive, package) + # get potential previously built and cached wheel + archive = self._chef.get_cached_archive_for_link(link, strict=False) if archive.suffix != ".whl": message = ( @@ -725,7 +723,9 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: ) self._write(operation, message) - archive = self._chef.prepare(archive, output_dir=output_dir) + archive = self._chef.prepare(original_archive, output_dir=output_dir) + + self._populate_hashes_dict(original_archive, package) return archive diff --git a/tests/installation/test_chef.py b/tests/installation/test_chef.py index e9046c5433f..bda5102f469 100644 --- a/tests/installation/test_chef.py +++ b/tests/installation/test_chef.py @@ -38,20 +38,80 @@ def setup(mocker: MockerFixture, pool: RepositoryPool) -> None: @pytest.mark.parametrize( - ("link", "cached"), + ("link", "strict", "available_packages"), + [ + ( + "https://files.python-poetry.org/demo-0.1.0.tar.gz", + True, + [ + Path("/cache/demo-0.1.0-py2.py3-none-any"), + Path("/cache/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl"), + Path("/cache/demo-0.1.0-cp37-cp37-macosx_10_15_x86_64.whl"), + ], + ), + ( + "https://example.com/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl", + False, + [], + ), + ], +) +def test_get_not_found_cached_archive_for_link( + config: Config, + mocker: MockerFixture, + link: str, + strict: bool, + available_packages: list[Path], +): + chef = Chef( + config, + MockEnv( + version_info=(3, 8, 3), + marker_env={"interpreter_name": "cpython", "interpreter_version": "3.8.3"}, + supported_tags=[ + Tag("cp38", "cp38", "macosx_10_15_x86_64"), + Tag("py3", "none", "any"), + ], + ), + Factory.create_pool(config), + ) + + mocker.patch.object( + chef, "get_cached_archives_for_link", return_value=available_packages + ) + + archive = chef.get_cached_archive_for_link(Link(link), strict=strict) + + assert None is archive + + +@pytest.mark.parametrize( + ("link", "cached", "strict"), [ ( "https://files.python-poetry.org/demo-0.1.0.tar.gz", "/cache/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl", + False, + ), + ( + "https://example.com/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl", + "/cache/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl", + False, + ), + ( + "https://files.python-poetry.org/demo-0.1.0.tar.gz", + "/cache/demo-0.1.0.tar.gz", + True, ), ( "https://example.com/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl", "/cache/demo-0.1.0-cp38-cp38-macosx_10_15_x86_64.whl", + True, ), ], ) -def test_get_cached_archive_for_link( - config: Config, mocker: MockerFixture, link: str, cached: str +def test_get_found_cached_archive_for_link( + config: Config, mocker: MockerFixture, link: str, cached: str, strict: bool ): chef = Chef( config, @@ -77,7 +137,7 @@ def test_get_cached_archive_for_link( ], ) - archive = chef.get_cached_archive_for_link(Link(link)) + archive = chef.get_cached_archive_for_link(Link(link), strict=strict) assert Path(cached) == archive diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index fe0ce46cb44..21f698ffe26 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -538,7 +538,7 @@ def test_executor_should_delete_incomplete_downloads( ) mocker.patch( "poetry.installation.chef.Chef.get_cached_archive_for_link", - side_effect=lambda link: None, + side_effect=lambda link, strict: None, ) mocker.patch( "poetry.installation.chef.Chef.get_cache_directory_for_link", @@ -740,13 +740,23 @@ def test_executor_should_write_pep610_url_references_for_editable_directories( ) +@pytest.mark.parametrize("cached", [False, True]) def test_executor_should_write_pep610_url_references_for_wheel_urls( tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, io: BufferedIO, mock_file_downloads: None, + mocker: MockerFixture, + fixture_dir: FixtureDirGetter, + cached: bool, ): + if cached: + link_cached = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" + mocker.patch( + "poetry.installation.chef.Chef.get_cached_archive_for_link", + return_value=link_cached, + ) package = Package( "demo", "0.1.0", @@ -776,13 +786,23 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( verify_installed_distribution(tmp_venv, package, expected_url_reference) -def test_executor_should_not_write_pep610_url_references_for_non_wheel_urls( +@pytest.mark.parametrize("cached", [False, True]) +def test_executor_should_write_pep610_url_references_for_non_wheel_urls( tmp_venv: VirtualEnv, pool: RepositoryPool, config: Config, io: BufferedIO, mock_file_downloads: None, + mocker: MockerFixture, + fixture_dir: FixtureDirGetter, + cached: bool, ): + if cached: + link_cached = fixture_dir("distributions") / "demo-0.1.0.tar.gz" + mocker.patch( + "poetry.installation.chef.Chef.get_cached_archive_for_link", + return_value=link_cached, + ) package = Package( "demo", "0.1.0", From bca42891564abfbf021768ae51e22881a3f6aa48 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Sun, 5 Mar 2023 23:49:59 +0100 Subject: [PATCH 07/16] make mypy happy and improve some code comments --- src/poetry/installation/executor.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index 2972ec72a5a..e5a4d9d6df3 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -698,9 +698,10 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: package = operation.package output_dir = self._chef.get_cache_directory_for_link(link) + # Try to get cached original package for the link provided original_archive = self._chef.get_cached_archive_for_link(link, strict=True) if original_archive is None: - # No cached distributions was found, so we download and prepare it + # No cached original distributions was found, so we download and prepare it try: original_archive = self._download_archive(operation, link) except BaseException: @@ -713,8 +714,12 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: raise - # get potential previously built and cached wheel + # Get potential higher prioritized cached archive, otherwise it will fall back + # to the original archive. archive = self._chef.get_cached_archive_for_link(link, strict=False) + # 'archive' can at this point never be None. Since we previously downloaded + # an archive, we now should have something cached that we can use here + assert archive is not None if archive.suffix != ".whl": message = ( @@ -723,8 +728,9 @@ def _download_link(self, operation: Install | Update, link: Link) -> Path: ) self._write(operation, message) - archive = self._chef.prepare(original_archive, output_dir=output_dir) + archive = self._chef.prepare(archive, output_dir=output_dir) + # Use the original archive to provide the correct hash. self._populate_hashes_dict(original_archive, package) return archive From 5bc6c03820185d09ac4c62d2bc7cd81af15d47ff Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Mon, 6 Mar 2023 02:09:34 +0100 Subject: [PATCH 08/16] Fix test_executor_should_write_pep610_url_references_for_non_wheel_urls being stuck due to incomplete mocking --- tests/installation/test_executor.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 21f698ffe26..95aed4827ff 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -799,9 +799,17 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( ): if cached: link_cached = fixture_dir("distributions") / "demo-0.1.0.tar.gz" + original_func = Chef.get_cached_archive_for_link + + def mock_get_cached_archive_for_link(self, link: Link, strict: bool): + if link.filename == "demo-0.1.0.tar.gz": + return link_cached + else: + return original_func(self, link, strict) + mocker.patch( "poetry.installation.chef.Chef.get_cached_archive_for_link", - return_value=link_cached, + new=mock_get_cached_archive_for_link, ) package = Package( "demo", @@ -829,6 +837,8 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( }, "url": package.source_url, } + print(io.fetch_output()) + print(io.fetch_error()) verify_installed_distribution(tmp_venv, package, expected_url_reference) From e672978948bc861dda1a96e6b776d5d284e33305 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Mon, 6 Mar 2023 02:19:52 +0100 Subject: [PATCH 09/16] make flake happy and removed some print debug statements :) --- tests/installation/test_executor.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 95aed4827ff..eef2c1d7481 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -22,6 +22,7 @@ from cleo.io.outputs.output import Verbosity from poetry.core.packages.package import Package from poetry.core.packages.utils.link import Link +from typing_extensions import Self from poetry.factory import Factory from poetry.installation.chef import Chef as BaseChef @@ -801,7 +802,7 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( link_cached = fixture_dir("distributions") / "demo-0.1.0.tar.gz" original_func = Chef.get_cached_archive_for_link - def mock_get_cached_archive_for_link(self, link: Link, strict: bool): + def mock_get_cached_archive_for_link(self: Self, link: Link, strict: bool): if link.filename == "demo-0.1.0.tar.gz": return link_cached else: @@ -837,8 +838,6 @@ def mock_get_cached_archive_for_link(self, link: Link, strict: bool): }, "url": package.source_url, } - print(io.fetch_output()) - print(io.fetch_error()) verify_installed_distribution(tmp_venv, package, expected_url_reference) From 9029971f682029afd74ff5482de27af065d5dd50 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Mon, 6 Mar 2023 02:23:08 +0100 Subject: [PATCH 10/16] make flake really happy --- tests/installation/test_executor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index eef2c1d7481..454f3baa45c 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -22,7 +22,6 @@ from cleo.io.outputs.output import Verbosity from poetry.core.packages.package import Package from poetry.core.packages.utils.link import Link -from typing_extensions import Self from poetry.factory import Factory from poetry.installation.chef import Chef as BaseChef @@ -41,6 +40,7 @@ from httpretty.core import HTTPrettyRequest from pytest_mock import MockerFixture + from typing_extensions import Self from poetry.config.config import Config from poetry.installation.operations.operation import Operation From 62d92baf7206a24ea86cdddc4c1765d79f204f73 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Mon, 6 Mar 2023 10:48:28 +0100 Subject: [PATCH 11/16] Code review improvements --- src/poetry/installation/chef.py | 2 +- tests/installation/test_executor.py | 27 +++++++++++++-------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index f866dd3946a..146d2a06289 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -199,7 +199,7 @@ def _should_prepare(self, archive: Path) -> bool: def _is_wheel(cls, archive: Path) -> bool: return archive.suffix == ".whl" - def get_cached_archive_for_link(self, link: Link, strict: bool) -> Path | None: + def get_cached_archive_for_link(self, link: Link, *, strict: bool) -> Path | None: archives = self.get_cached_archives_for_link(link) if not archives: return None diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 454f3baa45c..b86fbee5956 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -40,7 +40,6 @@ from httpretty.core import HTTPrettyRequest from pytest_mock import MockerFixture - from typing_extensions import Self from poetry.config.config import Config from poetry.installation.operations.operation import Operation @@ -741,7 +740,7 @@ def test_executor_should_write_pep610_url_references_for_editable_directories( ) -@pytest.mark.parametrize("cached", [False, True]) +@pytest.mark.parametrize("is_artifact_cached", [False, True]) def test_executor_should_write_pep610_url_references_for_wheel_urls( tmp_venv: VirtualEnv, pool: RepositoryPool, @@ -750,9 +749,9 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( mock_file_downloads: None, mocker: MockerFixture, fixture_dir: FixtureDirGetter, - cached: bool, + is_artifact_cached: bool, ): - if cached: + if is_artifact_cached: link_cached = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" mocker.patch( "poetry.installation.chef.Chef.get_cached_archive_for_link", @@ -787,7 +786,7 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( verify_installed_distribution(tmp_venv, package, expected_url_reference) -@pytest.mark.parametrize("cached", [False, True]) +@pytest.mark.parametrize("is_artifact_cached", [False, True]) def test_executor_should_write_pep610_url_references_for_non_wheel_urls( tmp_venv: VirtualEnv, pool: RepositoryPool, @@ -796,21 +795,21 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( mock_file_downloads: None, mocker: MockerFixture, fixture_dir: FixtureDirGetter, - cached: bool, + is_artifact_cached: bool, ): - if cached: - link_cached = fixture_dir("distributions") / "demo-0.1.0.tar.gz" - original_func = Chef.get_cached_archive_for_link + if is_artifact_cached: + cached_sdist = fixture_dir("distributions") / "demo-0.1.0.tar.gz" + cached_wheel = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" - def mock_get_cached_archive_for_link(self: Self, link: Link, strict: bool): - if link.filename == "demo-0.1.0.tar.gz": - return link_cached + def mock_get_cached_archive_for_link(_: Link, strict: bool): + if strict: + return cached_sdist else: - return original_func(self, link, strict) + return cached_wheel mocker.patch( "poetry.installation.chef.Chef.get_cached_archive_for_link", - new=mock_get_cached_archive_for_link, + side_effect=mock_get_cached_archive_for_link, ) package = Package( "demo", From ede8dbb84d0ad9729de3240a7aed1977f6e59ad6 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Mon, 6 Mar 2023 11:12:38 +0100 Subject: [PATCH 12/16] Further code review improvements and minor test improvements (cleanup of generated files) --- src/poetry/installation/chef.py | 4 +--- src/poetry/installation/executor.py | 5 ----- tests/installation/test_chef.py | 15 +++++++++++++++ 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index 146d2a06289..333130b4fda 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -86,8 +86,6 @@ def install(self, requirements: Collection[str]) -> None: class Chef: - tmp_dir_prefix = "poetry-chef-" - def __init__(self, config: Config, env: Env, pool: RepositoryPool) -> None: self._env = env self._pool = pool @@ -102,7 +100,7 @@ def prepare( return archive if archive.is_dir(): - tmp_dir = tempfile.mkdtemp(prefix=self.tmp_dir_prefix) + tmp_dir = tempfile.mkdtemp(prefix="poetry-chef-") return self._prepare(archive, Path(tmp_dir), editable=editable) return self._prepare_sdist(archive, destination=output_dir) diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index e5a4d9d6df3..2722959e9d0 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -5,7 +5,6 @@ import itertools import json import os -import tempfile import threading from concurrent.futures import ThreadPoolExecutor @@ -515,10 +514,6 @@ def _install(self, operation: Install | Update) -> int: else: archive = self._download(operation) - tmp_path_prefix = str(Path(tempfile.gettempdir(), self._chef.tmp_dir_prefix)) - if str(archive).startswith(tmp_path_prefix): - cleanup_archive = True - operation_message = self.get_operation_message(operation) message = ( f" • {operation_message}:" diff --git a/tests/installation/test_chef.py b/tests/installation/test_chef.py index bda5102f469..3359c4383fe 100644 --- a/tests/installation/test_chef.py +++ b/tests/installation/test_chef.py @@ -1,5 +1,8 @@ from __future__ import annotations +import os +import tempfile + from pathlib import Path from typing import TYPE_CHECKING from zipfile import ZipFile @@ -213,6 +216,10 @@ def test_prepare_directory(config: Config, config_cache_dir: Path): assert wheel.name == "simple_project-1.2.3-py2.py3-none-any.whl" + assert wheel.parent.parent == Path(tempfile.gettempdir()) + # cleanup generated tmp dir artifact + os.unlink(wheel) + def test_prepare_directory_with_extensions( config: Config, config_cache_dir: Path @@ -228,8 +235,12 @@ def test_prepare_directory_with_extensions( wheel = chef.prepare(archive) + assert wheel.parent.parent == Path(tempfile.gettempdir()) assert wheel.name == f"extended-0.1-{env.supported_tags[0]}.whl" + # cleanup generated tmp dir artifact + os.unlink(wheel) + def test_prepare_directory_editable(config: Config, config_cache_dir: Path): chef = Chef(config, EnvManager.get_system_env(), Factory.create_pool(config)) @@ -238,7 +249,11 @@ def test_prepare_directory_editable(config: Config, config_cache_dir: Path): wheel = chef.prepare(archive, editable=True) + assert wheel.parent.parent == Path(tempfile.gettempdir()) assert wheel.name == "simple_project-1.2.3-py2.py3-none-any.whl" with ZipFile(wheel) as z: assert "simple_project.pth" in z.namelist() + + # cleanup generated tmp dir artifact + os.unlink(wheel) From 1d844c0cd98bade01859d82f3174c301725e1425 Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Mon, 6 Mar 2023 19:45:28 +0100 Subject: [PATCH 13/16] Improved tests further --- src/poetry/installation/executor.py | 2 +- tests/installation/test_executor.py | 110 +++++++++++++++++----------- 2 files changed, 70 insertions(+), 42 deletions(-) diff --git a/src/poetry/installation/executor.py b/src/poetry/installation/executor.py index 2722959e9d0..70e61a2cea6 100644 --- a/src/poetry/installation/executor.py +++ b/src/poetry/installation/executor.py @@ -740,7 +740,7 @@ def _validate_archive_hash(archive: Path, package: Package) -> str: archive_hash: str = "sha256:" + get_file_hash(archive) known_hashes = {f["hash"] for f in package.files if f["file"] == archive.name} - if known_hashes and archive_hash not in known_hashes: + if archive_hash not in known_hashes: raise RuntimeError( f"Hash for {package} from archive {archive.name} not found in" f" known hashes (was: {archive_hash})" diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index b86fbee5956..8d4d637dde7 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -606,6 +606,12 @@ def test_executor_should_not_write_pep610_url_references_for_cached_package( io: BufferedIO, ): link_cached = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" + package.files = [ + { + "file": "demo-0.1.0-py2.py3-none-any.whl", + "hash": "sha256:70e704135718fffbcbf61ed1fc45933cfd86951a744b681000eaaa75da31f17a", # noqa: E501 + } + ] mocker.patch( "poetry.installation.executor.Executor._download", return_value=link_cached @@ -757,6 +763,8 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( "poetry.installation.chef.Chef.get_cached_archive_for_link", return_value=link_cached, ) + download_spy = mocker.spy(Executor, "_download_archive") + package = Package( "demo", "0.1.0", @@ -772,7 +780,8 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( ] executor = Executor(tmp_venv, pool, config, io) - executor.execute([Install(package)]) + operation = Install(package) + executor.execute([operation]) expected_url_reference = { "archive_info": { "hashes": { @@ -784,9 +793,28 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( "url": package.source_url, } verify_installed_distribution(tmp_venv, package, expected_url_reference) + if is_artifact_cached: + download_spy.assert_not_called() + else: + download_spy.assert_called_once_with( + mocker.ANY, operation, Link(package.source_url) + ) -@pytest.mark.parametrize("is_artifact_cached", [False, True]) +@pytest.mark.parametrize( + ( + "is_sdist_artifact_cached", + "is_wheel_cached", + "expect_artifact_building", + "expect_artifact_download", + ), + [ + (True, False, True, False), + (True, True, False, False), + (False, False, True, True), + (False, True, False, True), + ], +) def test_executor_should_write_pep610_url_references_for_non_wheel_urls( tmp_venv: VirtualEnv, pool: RepositoryPool, @@ -795,22 +823,41 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( mock_file_downloads: None, mocker: MockerFixture, fixture_dir: FixtureDirGetter, - is_artifact_cached: bool, + is_sdist_artifact_cached: bool, + is_wheel_cached: bool, + expect_artifact_building: bool, + expect_artifact_download: bool, ): - if is_artifact_cached: + built_wheel = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" + mock_prepare = mocker.patch( + "poetry.installation.chef.Chef._prepare", + return_value=built_wheel, + ) + download_spy = mocker.spy(Executor, "_download_archive") + + if is_sdist_artifact_cached | is_wheel_cached: cached_sdist = fixture_dir("distributions") / "demo-0.1.0.tar.gz" cached_wheel = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" - def mock_get_cached_archive_for_link(_: Link, strict: bool): + def mock_get_cached_archive_for_link_func(_: Link, strict: bool): if strict: - return cached_sdist + if is_sdist_artifact_cached: + return cached_sdist + else: + return None else: - return cached_wheel + if is_wheel_cached: + return cached_wheel + elif is_sdist_artifact_cached: + return cached_sdist + else: + return None mocker.patch( "poetry.installation.chef.Chef.get_cached_archive_for_link", - side_effect=mock_get_cached_archive_for_link, + side_effect=mock_get_cached_archive_for_link_func, ) + package = Package( "demo", "0.1.0", @@ -826,7 +873,8 @@ def mock_get_cached_archive_for_link(_: Link, strict: bool): ] executor = Executor(tmp_venv, pool, config, io) - executor.execute([Install(package)]) + operation = Install(package) + executor.execute([operation]) expected_url_reference = { "archive_info": { "hashes": { @@ -839,6 +887,18 @@ def mock_get_cached_archive_for_link(_: Link, strict: bool): } verify_installed_distribution(tmp_venv, package, expected_url_reference) + if expect_artifact_building: + mock_prepare.assert_called_once() + else: + mock_prepare.assert_not_called() + + if expect_artifact_download: + download_spy.assert_called_once_with( + mocker.ANY, operation, Link(package.source_url) + ) + else: + download_spy.assert_not_called() + def test_executor_should_write_pep610_url_references_for_git( tmp_venv: VirtualEnv, @@ -947,38 +1007,6 @@ def test_executor_should_write_pep610_url_references_for_git_with_subdirectories ) -def test_executor_should_use_cached_link_and_hash( - tmp_venv: VirtualEnv, - pool: RepositoryPool, - config: Config, - io: BufferedIO, - mocker: MockerFixture, - fixture_dir: FixtureDirGetter, -): - link_cached = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" - - mocker.patch( - "poetry.installation.chef.Chef.get_cached_archive_for_link", - return_value=link_cached, - ) - - package = Package("demo", "0.1.0") - # Set package.files so the executor will attempt to hash the package - package.files = [ - { - "file": "demo-0.1.0-py2.py3-none-any.whl", - "hash": "sha256:70e704135718fffbcbf61ed1fc45933cfd86951a744b681000eaaa75da31f17a", # noqa: E501 - } - ] - - executor = Executor(tmp_venv, pool, config, io) - archive = executor._download_link( - Install(package), - Link("https://example.com/demo-0.1.0-py2.py3-none-any.whl"), - ) - assert archive == link_cached - - @pytest.mark.parametrize( ("max_workers", "cpu_count", "side_effect", "expected_workers"), [ From 1772a6ae3487b1526e94436da4d1ea639e9d98ed Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Tue, 7 Mar 2023 09:14:28 +0100 Subject: [PATCH 14/16] refactor test mocking logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Randy Döring <30527984+radoering@users.noreply.github.com> --- tests/installation/test_executor.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 8d4d637dde7..6044983733b 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -840,18 +840,11 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( cached_wheel = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" def mock_get_cached_archive_for_link_func(_: Link, strict: bool): - if strict: - if is_sdist_artifact_cached: - return cached_sdist - else: - return None - else: - if is_wheel_cached: - return cached_wheel - elif is_sdist_artifact_cached: - return cached_sdist - else: - return None + if is_wheel_cached and not strict: + return cached_wheel + if is_sdist_cached: + return cached_sdist + return None mocker.patch( "poetry.installation.chef.Chef.get_cached_archive_for_link", From 41e2727f88e4309801bfcd1ee99a2b6e0abbe9ea Mon Sep 17 00:00:00 2001 From: quantum-byte Date: Tue, 7 Mar 2023 09:16:18 +0100 Subject: [PATCH 15/16] Additional Review improvements --- src/poetry/installation/chef.py | 3 +-- tests/installation/test_executor.py | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/poetry/installation/chef.py b/src/poetry/installation/chef.py index 333130b4fda..6eb1ae3f21b 100644 --- a/src/poetry/installation/chef.py +++ b/src/poetry/installation/chef.py @@ -209,8 +209,7 @@ def get_cached_archive_for_link(self, link: Link, *, strict: bool) -> Path | Non # prioritized archive type. if link.filename == archive.name: return archive - else: - continue + continue if archive.suffix != ".whl": candidates.append((float("inf"), archive)) continue diff --git a/tests/installation/test_executor.py b/tests/installation/test_executor.py index 6044983733b..578b270354b 100644 --- a/tests/installation/test_executor.py +++ b/tests/installation/test_executor.py @@ -803,7 +803,7 @@ def test_executor_should_write_pep610_url_references_for_wheel_urls( @pytest.mark.parametrize( ( - "is_sdist_artifact_cached", + "is_sdist_cached", "is_wheel_cached", "expect_artifact_building", "expect_artifact_download", @@ -823,7 +823,7 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( mock_file_downloads: None, mocker: MockerFixture, fixture_dir: FixtureDirGetter, - is_sdist_artifact_cached: bool, + is_sdist_cached: bool, is_wheel_cached: bool, expect_artifact_building: bool, expect_artifact_download: bool, @@ -835,7 +835,7 @@ def test_executor_should_write_pep610_url_references_for_non_wheel_urls( ) download_spy = mocker.spy(Executor, "_download_archive") - if is_sdist_artifact_cached | is_wheel_cached: + if is_sdist_cached | is_wheel_cached: cached_sdist = fixture_dir("distributions") / "demo-0.1.0.tar.gz" cached_wheel = fixture_dir("distributions") / "demo-0.1.0-py2.py3-none-any.whl" From 1e1c331e10bbc37ec5aa6ebb2877626aacf94244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Tue, 7 Mar 2023 17:04:59 +0100 Subject: [PATCH 16/16] coding style --- tests/installation/test_chef.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/installation/test_chef.py b/tests/installation/test_chef.py index 3359c4383fe..19283cdeaaa 100644 --- a/tests/installation/test_chef.py +++ b/tests/installation/test_chef.py @@ -85,7 +85,7 @@ def test_get_not_found_cached_archive_for_link( archive = chef.get_cached_archive_for_link(Link(link), strict=strict) - assert None is archive + assert archive is None @pytest.mark.parametrize(