From 095d45240e6c9dba1ec200836c4f25de98f1c5c7 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 19 Oct 2022 23:15:34 -0500 Subject: [PATCH 1/5] Allow non-existant path dependencies --- .../core/packages/directory_dependency.py | 57 ++++++++++++------- .../non_python_project/pyproject.toml | 1 + tests/masonry/test_api.py | 11 ++-- tests/packages/test_directory_dependency.py | 19 ++++++- tests/test_factory.py | 12 ---- 5 files changed, 59 insertions(+), 41 deletions(-) create mode 100644 tests/fixtures/non_python_project/pyproject.toml diff --git a/src/poetry/core/packages/directory_dependency.py b/src/poetry/core/packages/directory_dependency.py index 5b3cfa5d6..fe9ec2795 100644 --- a/src/poetry/core/packages/directory_dependency.py +++ b/src/poetry/core/packages/directory_dependency.py @@ -4,6 +4,7 @@ from pathlib import Path from typing import Iterable +from warnings import warn from poetry.core.packages.dependency import Dependency from poetry.core.packages.utils.utils import is_python_project @@ -25,26 +26,8 @@ def __init__( self._path = path self._base = base or Path.cwd() self._full_path = path - - if not self._path.is_absolute(): - try: - self._full_path = self._base.joinpath(self._path).resolve() - except FileNotFoundError: - raise ValueError(f"Directory {self._path} does not exist") - self._develop = develop - if not self._full_path.exists(): - raise ValueError(f"Directory {self._path} does not exist") - - if self._full_path.is_file(): - raise ValueError(f"{self._path} is a file, expected a directory") - - if not is_python_project(self._full_path): - raise ValueError( - f"Directory {self._full_path} does not seem to be a Python package" - ) - super().__init__( name, "*", @@ -55,10 +38,46 @@ def __init__( source_url=self._full_path.as_posix(), extras=extras, ) - # cache this function to avoid multiple IO reads and parsing self.supports_poetry = functools.lru_cache(maxsize=1)(self._supports_poetry) + self._validate() + + def _validate(self) -> None: + # If the directory exists do some general validation on it. + # There is no valid reason to have a path dependency to something + # that is not a valid Python project. + # If the directory doesn't exist, emit a warning and continue. + # The most common cause of a directory not existing is likely user error + # (e.g. a typo) so we want to do _something_ to alert them. + # But there are also valid situations where we might want to build + # a directory dependency pointing to a non-existing folder, for example: + # - We are re-locking a project where a path dependency was removed + # It still exists in the lockfile and once we finish re-locking + # we'll delete it from the lockfile, but we need to load it from the + # lockfile first to begin solving. + # - A user wants to dynamically link to / insert the dependency at runtime. + # This may seem a bit strange but in theory is totally valid and we should + # allow it. + if not self._path.is_absolute(): + try: + self._full_path = self._base.joinpath(self._path).resolve() + except FileNotFoundError: + warn(f"Directory {self._path} does not exist", category=UserWarning) + return + + if not self._full_path.exists(): + warn(f"Directory {self._path} does not exist", category=UserWarning) + return + + if self._full_path.is_file(): + raise ValueError(f"{self._path} is a file, expected a directory") + + if not is_python_project(self._full_path): + raise ValueError( + f"Directory {self._full_path} does not seem to be a Python package", + ) + @property def path(self) -> Path: return self._path diff --git a/tests/fixtures/non_python_project/pyproject.toml b/tests/fixtures/non_python_project/pyproject.toml new file mode 100644 index 000000000..c8002a6a3 --- /dev/null +++ b/tests/fixtures/non_python_project/pyproject.toml @@ -0,0 +1 @@ +# missing build section diff --git a/tests/masonry/test_api.py b/tests/masonry/test_api.py index 1ba6d8edd..3f8c99d5a 100644 --- a/tests/masonry/test_api.py +++ b/tests/masonry/test_api.py @@ -72,12 +72,11 @@ def test_build_wheel_with_bad_path_dev_dep_succeeds() -> None: api.build_wheel(tmp_dir) -def test_build_wheel_with_bad_path_dep_fails() -> None: - with pytest.raises(ValueError) as err, temporary_directory() as tmp_dir, cwd( +def test_build_wheel_with_bad_path_dep_succeeds() -> None: + with temporary_directory() as tmp_dir, cwd( os.path.join(fixtures, "with_bad_path_dep") ): api.build_wheel(tmp_dir) - assert "does not exist" in str(err.value) @pytest.mark.skipif( @@ -124,11 +123,10 @@ def test_build_sdist_with_bad_path_dev_dep_succeeds() -> None: def test_build_sdist_with_bad_path_dep_fails() -> None: - with pytest.raises(ValueError) as err, temporary_directory() as tmp_dir, cwd( + with temporary_directory() as tmp_dir, cwd( os.path.join(fixtures, "with_bad_path_dep") ): api.build_sdist(tmp_dir) - assert "does not exist" in str(err.value) def test_prepare_metadata_for_build_wheel() -> None: @@ -210,11 +208,10 @@ def test_prepare_metadata_for_build_wheel_with_bad_path_dev_dep_succeeds() -> No def test_prepare_metadata_for_build_wheel_with_bad_path_dep_succeeds() -> None: - with pytest.raises(ValueError) as err, temporary_directory() as tmp_dir, cwd( + with temporary_directory() as tmp_dir, cwd( os.path.join(fixtures, "with_bad_path_dep") ): api.prepare_metadata_for_build_wheel(tmp_dir) - assert "does not exist" in str(err.value) def test_build_editable_wheel() -> None: diff --git a/tests/packages/test_directory_dependency.py b/tests/packages/test_directory_dependency.py index db865e0b1..f1e47368b 100644 --- a/tests/packages/test_directory_dependency.py +++ b/tests/packages/test_directory_dependency.py @@ -9,13 +9,26 @@ from poetry.core.packages.directory_dependency import DirectoryDependency +FIXTURES_PATH = Path(__file__).parent.parent / "fixtures" DIST_PATH = Path(__file__).parent.parent / "fixtures" / "git" / "github.com" / "demo" SAMPLE_PROJECT = Path(__file__).parent.parent / "fixtures" / "sample_project" +TEST_FILE = Path(__file__).parent.parent / "fixtures" / "distributions" / "demo-0.1.0.tar.gz" -def test_directory_dependency_must_exist() -> None: - with pytest.raises(ValueError): - DirectoryDependency("demo", DIST_PATH / "invalid") + +def test_directory_dependency_warns_if_not_exists() -> None: + with pytest.warns(UserWarning, match="does not exist"): + DirectoryDependency("demo", FIXTURES_PATH / "invalid") + + +def test_directory_dependency_errors_if_is_not_a_directory() -> None: + with pytest.raises(ValueError, match="is a file"): + DirectoryDependency("demo", TEST_FILE) + + +def test_directory_dependency_errors_if_not_a_valid_python_project() -> None: + with pytest.raises(ValueError, match="does not seem to be a Python package"): + DirectoryDependency("demo", FIXTURES_PATH / "non_python_project") def _test_directory_dependency_pep_508( diff --git a/tests/test_factory.py b/tests/test_factory.py index 6ca71b041..a11eca11e 100644 --- a/tests/test_factory.py +++ b/tests/test_factory.py @@ -298,18 +298,6 @@ def test_create_poetry_omits_dev_dependencies_iff_with_dev_is_false() -> None: assert any("dev" in r.groups for r in poetry.package.all_requires) -def test_create_poetry_fails_with_invalid_dev_dependencies_iff_with_dev_is_true() -> ( - None -): - with pytest.raises(ValueError) as err: - Factory().create_poetry(fixtures_dir / "project_with_invalid_dev_deps") - assert "does not exist" in str(err.value) - - Factory().create_poetry( - fixtures_dir / "project_with_invalid_dev_deps", with_groups=False - ) - - def test_create_poetry_with_groups_and_legacy_dev() -> None: poetry = Factory().create_poetry( fixtures_dir / "project_with_groups_and_legacy_dev" From d891024e285ebf1608c5b354d313ae73ee16abb6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 20 Oct 2022 04:30:02 +0000 Subject: [PATCH 2/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/poetry/core/packages/directory_dependency.py | 2 +- tests/packages/test_directory_dependency.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/poetry/core/packages/directory_dependency.py b/src/poetry/core/packages/directory_dependency.py index fe9ec2795..13565e05d 100644 --- a/src/poetry/core/packages/directory_dependency.py +++ b/src/poetry/core/packages/directory_dependency.py @@ -53,7 +53,7 @@ def _validate(self) -> None: # But there are also valid situations where we might want to build # a directory dependency pointing to a non-existing folder, for example: # - We are re-locking a project where a path dependency was removed - # It still exists in the lockfile and once we finish re-locking + # It still exists in the lockfile and once we finish re-locking # we'll delete it from the lockfile, but we need to load it from the # lockfile first to begin solving. # - A user wants to dynamically link to / insert the dependency at runtime. diff --git a/tests/packages/test_directory_dependency.py b/tests/packages/test_directory_dependency.py index f1e47368b..2a0a05cd9 100644 --- a/tests/packages/test_directory_dependency.py +++ b/tests/packages/test_directory_dependency.py @@ -13,7 +13,9 @@ DIST_PATH = Path(__file__).parent.parent / "fixtures" / "git" / "github.com" / "demo" SAMPLE_PROJECT = Path(__file__).parent.parent / "fixtures" / "sample_project" -TEST_FILE = Path(__file__).parent.parent / "fixtures" / "distributions" / "demo-0.1.0.tar.gz" +TEST_FILE = ( + Path(__file__).parent.parent / "fixtures" / "distributions" / "demo-0.1.0.tar.gz" +) def test_directory_dependency_warns_if_not_exists() -> None: From d8b868ab7063827be4eef50fe744a032d51f2c48 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 20 Oct 2022 00:25:17 -0500 Subject: [PATCH 3/5] validate before calling super() --- src/poetry/core/packages/directory_dependency.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/poetry/core/packages/directory_dependency.py b/src/poetry/core/packages/directory_dependency.py index fe9ec2795..37aa5fc4b 100644 --- a/src/poetry/core/packages/directory_dependency.py +++ b/src/poetry/core/packages/directory_dependency.py @@ -28,6 +28,8 @@ def __init__( self._full_path = path self._develop = develop + self._validate() + super().__init__( name, "*", @@ -41,8 +43,6 @@ def __init__( # cache this function to avoid multiple IO reads and parsing self.supports_poetry = functools.lru_cache(maxsize=1)(self._supports_poetry) - self._validate() - def _validate(self) -> None: # If the directory exists do some general validation on it. # There is no valid reason to have a path dependency to something From efbba4bd5d37626c16c814ad826a33b670eaedd3 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sun, 6 Nov 2022 11:30:54 -0600 Subject: [PATCH 4/5] lazy validator --- .../core/packages/directory_dependency.py | 69 ++++++++++--------- src/poetry/core/packages/file_dependency.py | 54 ++++++++++----- 2 files changed, 74 insertions(+), 49 deletions(-) diff --git a/src/poetry/core/packages/directory_dependency.py b/src/poetry/core/packages/directory_dependency.py index dbdb74d57..6a877204d 100644 --- a/src/poetry/core/packages/directory_dependency.py +++ b/src/poetry/core/packages/directory_dependency.py @@ -13,6 +13,8 @@ class DirectoryDependency(Dependency): + _full_path: Path | None + def __init__( self, name: str, @@ -25,10 +27,10 @@ def __init__( ) -> None: self._path = path self._base = base or Path.cwd() - self._full_path = path + self._full_path = None self._develop = develop - self._validate() + full_path = self._get_full_path(False, name) super().__init__( name, @@ -37,54 +39,55 @@ def __init__( optional=optional, allows_prereleases=True, source_type="directory", - source_url=self._full_path.as_posix(), + source_url=full_path.as_posix(), extras=extras, ) # cache this function to avoid multiple IO reads and parsing self.supports_poetry = functools.lru_cache(maxsize=1)(self._supports_poetry) - def _validate(self) -> None: - # If the directory exists do some general validation on it. - # There is no valid reason to have a path dependency to something - # that is not a valid Python project. - # If the directory doesn't exist, emit a warning and continue. - # The most common cause of a directory not existing is likely user error - # (e.g. a typo) so we want to do _something_ to alert them. - # But there are also valid situations where we might want to build - # a directory dependency pointing to a non-existing folder, for example: - # - We are re-locking a project where a path dependency was removed - # It still exists in the lockfile and once we finish re-locking - # we'll delete it from the lockfile, but we need to load it from the - # lockfile first to begin solving. - # - A user wants to dynamically link to / insert the dependency at runtime. - # This may seem a bit strange but in theory is totally valid and we should - # allow it. + def _get_full_path(self, raise_if_invalid: bool, name: str) -> Path: + if self._full_path is not None: + return self._full_path + full_path = self._path if not self._path.is_absolute(): try: - self._full_path = self._base.joinpath(self._path).resolve() + full_path = self._base.joinpath(self._path).resolve() except FileNotFoundError: - warn(f"Directory {self._path} does not exist", category=UserWarning) - return - - if not self._full_path.exists(): - warn(f"Directory {self._path} does not exist", category=UserWarning) - return - - if self._full_path.is_file(): - raise ValueError(f"{self._path} is a file, expected a directory") + msg = f"Source directory {self._path} for {name} does not exist" + if raise_if_invalid: + raise ValueError(msg) + warn(msg, category=UserWarning) + return full_path + + if not full_path.exists(): + msg = f"Source directory {self._path} for {name} does not exist" + if raise_if_invalid: + raise ValueError(msg) + warn(msg, category=UserWarning) + return full_path + + if full_path.is_file(): + raise ValueError( + f"Expected source for {name} to be a" + f" directory but {self._path} is a file" + ) - if not is_python_project(self._full_path): + if not is_python_project(full_path): raise ValueError( - f"Directory {self._full_path} does not seem to be a Python package", + f"The source directory {self._full_path} for {name}" + " does not seem to be a Python package", ) + self._full_path = full_path + return full_path + @property def path(self) -> Path: return self._path @property def full_path(self) -> Path: - return self._full_path + return self._get_full_path(True, self.name) @property def base(self) -> Path: @@ -95,7 +98,7 @@ def develop(self) -> bool: return self._develop def _supports_poetry(self) -> bool: - return PyProjectTOML(self._full_path / "pyproject.toml").is_poetry_project() + return PyProjectTOML(self.full_path / "pyproject.toml").is_poetry_project() def is_directory(self) -> bool: return True diff --git a/src/poetry/core/packages/file_dependency.py b/src/poetry/core/packages/file_dependency.py index 2b71000c6..64dd50350 100644 --- a/src/poetry/core/packages/file_dependency.py +++ b/src/poetry/core/packages/file_dependency.py @@ -5,12 +5,15 @@ from pathlib import Path from typing import Iterable +from warnings import warn from poetry.core.packages.dependency import Dependency from poetry.core.packages.utils.utils import path_to_url class FileDependency(Dependency): + _full_path: Path | None + def __init__( self, name: str, @@ -22,19 +25,8 @@ def __init__( ) -> None: self._path = path self._base = base or Path.cwd() - self._full_path = path - - if not self._path.is_absolute(): - try: - self._full_path = self._base.joinpath(self._path).resolve() - except FileNotFoundError: - raise ValueError(f"Directory {self._path} does not exist") - - if not self._full_path.exists(): - raise ValueError(f"File {self._path} does not exist") - - if self._full_path.is_dir(): - raise ValueError(f"{self._path} is a directory, expected a file") + self._full_path = None + full_path = self._get_full_path(False, name) super().__init__( name, @@ -43,10 +35,40 @@ def __init__( optional=optional, allows_prereleases=True, source_type="file", - source_url=self._full_path.as_posix(), + source_url=full_path.as_posix(), extras=extras, ) + def _get_full_path(self, raise_if_invalid: bool, name: str) -> Path: + if self._full_path is not None: + return self._full_path + full_path = self._path + if not self._path.is_absolute(): + try: + full_path = self._base.joinpath(self._path).resolve() + except FileNotFoundError: + msg = f"Source file {self._path} for {name} does not exist" + if raise_if_invalid: + raise ValueError(msg) + warn(msg, category=UserWarning) + return full_path + + if not full_path.exists(): + msg = f"Source file {self._path} for {name} does not exist" + if raise_if_invalid: + raise ValueError(msg) + warn(msg, category=UserWarning) + return full_path + + if full_path.is_dir(): + raise ValueError( + f"Expected source for {name} to be a" + f" file but {self._path} is a directory" + ) + + self._full_path = full_path + return full_path + @property def base(self) -> Path: return self._base @@ -57,14 +79,14 @@ def path(self) -> Path: @property def full_path(self) -> Path: - return self._full_path + return self._get_full_path(True, self.name) def is_file(self) -> bool: return True def hash(self, hash_name: str = "sha256") -> str: h = hashlib.new(hash_name) - with self._full_path.open("rb") as fp: + with self.full_path.open("rb") as fp: for content in iter(lambda: fp.read(io.DEFAULT_BUFFER_SIZE), b""): h.update(content) From b6922e38c90cdd1794ac7c9b92eaa2e9a3ec2d34 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Sun, 6 Nov 2022 11:34:23 -0600 Subject: [PATCH 5/5] revert test chaanges --- tests/masonry/test_api.py | 11 +++++++---- tests/packages/test_directory_dependency.py | 21 +++------------------ tests/packages/test_file_dependency.py | 2 +- 3 files changed, 11 insertions(+), 23 deletions(-) diff --git a/tests/masonry/test_api.py b/tests/masonry/test_api.py index 8917df817..4b8c6feec 100644 --- a/tests/masonry/test_api.py +++ b/tests/masonry/test_api.py @@ -72,11 +72,12 @@ def test_build_wheel_with_bad_path_dev_dep_succeeds() -> None: api.build_wheel(tmp_dir) -def test_build_wheel_with_bad_path_dep_succeeds() -> None: - with temporary_directory() as tmp_dir, cwd( +def test_build_wheel_with_bad_path_dep_fails() -> None: + with pytest.raises(ValueError) as err, temporary_directory() as tmp_dir, cwd( os.path.join(fixtures, "with_bad_path_dep") ): api.build_wheel(tmp_dir) + assert "does not exist" in str(err.value) @pytest.mark.skipif( @@ -123,10 +124,11 @@ def test_build_sdist_with_bad_path_dev_dep_succeeds() -> None: def test_build_sdist_with_bad_path_dep_fails() -> None: - with temporary_directory() as tmp_dir, cwd( + with pytest.raises(ValueError) as err, temporary_directory() as tmp_dir, cwd( os.path.join(fixtures, "with_bad_path_dep") ): api.build_sdist(tmp_dir) + assert "does not exist" in str(err.value) def test_prepare_metadata_for_build_wheel() -> None: @@ -208,10 +210,11 @@ def test_prepare_metadata_for_build_wheel_with_bad_path_dev_dep_succeeds() -> No def test_prepare_metadata_for_build_wheel_with_bad_path_dep_succeeds() -> None: - with temporary_directory() as tmp_dir, cwd( + with pytest.raises(ValueError) as err, temporary_directory() as tmp_dir, cwd( os.path.join(fixtures, "with_bad_path_dep") ): api.prepare_metadata_for_build_wheel(tmp_dir) + assert "does not exist" in str(err.value) def test_build_editable_wheel() -> None: diff --git a/tests/packages/test_directory_dependency.py b/tests/packages/test_directory_dependency.py index 6fad7de77..dc8d1f3a7 100644 --- a/tests/packages/test_directory_dependency.py +++ b/tests/packages/test_directory_dependency.py @@ -9,28 +9,13 @@ from poetry.core.packages.directory_dependency import DirectoryDependency -FIXTURES_PATH = Path(__file__).parent.parent / "fixtures" DIST_PATH = Path(__file__).parent.parent / "fixtures" / "git" / "github.com" / "demo" SAMPLE_PROJECT = Path(__file__).parent.parent / "fixtures" / "sample_project" -TEST_FILE = ( - Path(__file__).parent.parent / "fixtures" / "distributions" / "demo-0.1.0.tar.gz" -) - - -def test_directory_dependency_warns_if_not_exists() -> None: - with pytest.warns(UserWarning, match="does not exist"): - DirectoryDependency("demo", FIXTURES_PATH / "invalid") - -def test_directory_dependency_errors_if_is_not_a_directory() -> None: - with pytest.raises(ValueError, match="is a file"): - DirectoryDependency("demo", TEST_FILE) - - -def test_directory_dependency_errors_if_not_a_valid_python_project() -> None: - with pytest.raises(ValueError, match="does not seem to be a Python package"): - DirectoryDependency("demo", FIXTURES_PATH / "non_python_project") +def test_directory_dependency_must_exist() -> None: + with pytest.raises(ValueError): + DirectoryDependency("demo", DIST_PATH / "invalid").full_path def _test_directory_dependency_pep_508( diff --git a/tests/packages/test_file_dependency.py b/tests/packages/test_file_dependency.py index ee1b1199c..2d2972b38 100644 --- a/tests/packages/test_file_dependency.py +++ b/tests/packages/test_file_dependency.py @@ -22,7 +22,7 @@ def test_file_dependency_wrong_path() -> None: with pytest.raises(ValueError): - FileDependency("demo", DIST_PATH / "demo-0.2.0.tar.gz") + FileDependency("demo", DIST_PATH / "demo-0.2.0.tar.gz").full_path def test_file_dependency_dir() -> None: