Allow non-existant path dependencies by adriangb · Pull Request #507 · python-poetry/poetry-core

adriangb · 2022-10-20T04:29:34Z

Currently poetry lock --no-update fails when you remove any path dependency because it first loads all dependencies from the lock file (which still has a reference to the dependency you just deleted) and it fails at that point because when DirectoryDependency is instantiated with a non-existent path it immediately throws an exception.

This change allows the process to continue, which is the same behavior VCS dependencies have.

If the pyproject.toml has a path dependency that doesn't exist we will want to error for poetry install and poetry lock (poetry build with path dependencies is already kinda a no-go). poetry install already fails gracefully (if you lock the project then delete the path dependency and try to install; i.e. when no locking happens before we start installing), I opened python-poetry/poetry#6844 to make poetry lock fail gracefully.

for more information, see https://pre-commit.ci

adriangb · 2022-10-20T04:34:03Z

I looked into the version history for this file to see if I could find some rational for the way it currently works. This goes all the way back to the "Initial commit"

poetry-core/poetry_core/packages/directory_dependency.py

Line 35 in d095e4a

raise ValueError("Directory {} does not exist".format(self._path))

so I guess at this point only @sdispater knows.

…iangb/poetry-core into allow-non-existant-path-deps

adriangb · 2022-11-02T22:57:59Z

@radoering any thoughts on this?

radoering · 2022-11-05T08:06:40Z

I think it makes sense, but I have to take a deeper look if python-poetry/poetry#6844 is sufficient (i.e. covers all commands). For example, does poetry show still fails gracefully?

Further, I think we should do a similar change for file dependencies because it's the same use case.

radoering · 2022-11-05T08:15:04Z

related: python-poetry/poetry#668, #210

radoering · 2022-11-06T15:53:53Z

src/poetry/core/packages/directory_dependency.py

+            return
+
+        if self._full_path.is_file():
+            raise ValueError(f"{self._path} is a file, expected a directory")


Either all checks should raise an error or all checks should emit a warning (different modes). If I don't care if the directory exists, I won't care if it has been replaced with a file.

radoering · 2022-11-06T15:55:12Z

src/poetry/core/packages/directory_dependency.py

+            raise ValueError(f"{self._path} is a file, expected a directory")
+
+        if not is_python_project(self._full_path):
+            raise ValueError(


Same here. If I don't care for existence, I won't care if it's not a Python package.

I'd like to push back on this a bit. There's a real use case for creating a DirectoryDependency pointing to a path that does not exist: you deleted or renamed the dependency and are updating the lock file. I can't think of any use case where you'd want to point at an existing but invalid dependency, so in those cases I think failing fast is the best option.

radoering · 2022-11-06T16:03:26Z

src/poetry/core/packages/directory_dependency.py

+            try:
+                self._full_path = self._base.joinpath(self._path).resolve()
+            except FileNotFoundError:
+                warn(f"Directory {self._path} does not exist", category=UserWarning)


I like mentioning the name of the dependency (how you did in the companion PR). Maybe, we can add it in the warning/error messages here, too.

radoering · 2022-11-06T16:06:52Z

src/poetry/core/packages/directory_dependency.py

        # 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:


I think we should try the approach mentioned in #210 (comment). We need two modes for _validate: a warning mode and an exception mode. The warning mode can be used in the __init__ and the exception mode in full_path. I think it should be save to assume that when accessing full_path the directory must exist (and be a valid project).

sonarqubecloud · 2022-11-06T17:35:07Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

adriangb · 2022-11-06T17:35:15Z

tests/test_factory.py

-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
-    )
-
-


Removing this test since we actually want to be able to create a Poetry object with invalid path deps, we just want it to fail for certain operations (install, show, etc.).

adriangb · 2022-11-06T17:36:16Z

tests/packages/test_file_dependency.py

 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


I think these tests should be testing each individual failure mode, but this was good enough for years so I suppose it will do

adriangb · 2022-11-06T17:37:05Z

src/poetry/core/packages/file_dependency.py

            extras=extras,
        )

+    def _get_full_path(self, raise_if_invalid: bool, name: str) -> Path:


Adding name was necessary because .name doesn't get set until super().__init__ is called which already requires full_path.

adriangb · 2022-11-06T17:37:58Z

src/poetry/core/packages/file_dependency.py

+        if self._full_path is not None:
+            return self._full_path


This little bit of caching means that if we already checked in __init__ we don't need to check again. We could also cache the negative case but it doesn't seem worth the complexity IMO.

adriangb · 2022-11-06T17:39:15Z

src/poetry/core/packages/directory_dependency.py

+            raise ValueError(f"{self._path} is a file, expected a directory")
+
+        if not is_python_project(self._full_path):
+            raise ValueError(


I'd like to push back on this a bit. There's a real use case for creating a DirectoryDependency pointing to a path that does not exist: you deleted or renamed the dependency and are updating the lock file. I can't think of any use case where you'd want to point at an existing but invalid dependency, so in those cases I think failing fast is the best option.

adriangb · 2022-11-06T18:06:12Z

So this works. Conceptually though I don't like the idea of having .full_path be the "validation". From the perspective of the poetry codebase it seems like it might be a bit confusing, even calling it something like get_full_path_and_validate_dependency() -> Path would be better to make it clear that this method gets called for validation. But that's more your call, you're more familiar with the coupling between the codebases.

radoering · 2022-11-06T18:45:07Z

You have got a point there. On the one hand it's an easier transition from "validation in __init__" and you can't forget to validate, on the other hand it might be a bit surprising. We could also make the validate method "public" and require to call it explicitly. Some more eyes might be needed here. I'll discuss this with other maintainers.

adriangb · 2022-11-06T18:47:53Z

Sounds good. Do you think we could move forward with what we've got in the meantime? I think what we've got here is better than what we had before and the behavior ("automatic" validation) is largely the same, so we could treat making validate public and somehow requiring it to be explicitly called a future refactoring. Mostly because I think there needs to be some thought put into what this means for other dependency types (VCS, direct, etc.) and where it should be called in poetry; I don't want that to stall this work.

radoering · 2022-11-12T18:32:43Z

I took a deeper look into this topic and believe to have found a cleaner solution in #520. Please take a look if this approach fits your needs.

adriangb · 2022-11-12T18:51:20Z

Yep should work

Allow non-existant path dependencies

095d452

adriangb mentioned this pull request Oct 20, 2022

Fail with comprehensible error message if path dependencies do not exist python-poetry/poetry#6844

Merged

[pre-commit.ci] auto fixes from pre-commit.com hooks

d891024

for more information, see https://pre-commit.ci

adriangb added 2 commits October 20, 2022 00:25

validate before calling super()

d8b868a

Merge branch 'allow-non-existant-path-deps' of https://github.com/adr…

87b5a4f

…iangb/poetry-core into allow-non-existant-path-deps

This was referenced Oct 20, 2022

Add option to skip installing directory dependencies python-poetry/poetry#6845

Merged

Monorepo support using groups and path dependencies python-poetry/poetry#6850

Closed

adriangb added 2 commits October 25, 2022 19:39

Merge branch 'main' into allow-non-existant-path-deps

a8368e7

Merge branch 'main' into allow-non-existant-path-deps

0755814

Merge branch 'main' into allow-non-existant-path-deps

953c91b

radoering requested changes Nov 6, 2022

View reviewed changes

adriangb added 2 commits November 6, 2022 11:30

lazy validator

efbba4b

revert test chaanges

b6922e3

adriangb commented Nov 6, 2022

View reviewed changes

adriangb requested a review from radoering November 6, 2022 18:03

radoering mentioned this pull request Nov 12, 2022

Allow non-existent path dependencies - reloaded #520

Merged

2 tasks

adriangb closed this Nov 12, 2022

Comments

Conversation

adriangb commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Oct 20, 2022

Uh oh!

adriangb commented Nov 2, 2022

Uh oh!

radoering commented Nov 5, 2022

Uh oh!

radoering commented Nov 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Nov 6, 2022

Uh oh!

radoering commented Nov 6, 2022

Uh oh!

adriangb commented Nov 6, 2022

Uh oh!

radoering commented Nov 12, 2022

Uh oh!

adriangb commented Nov 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adriangb commented Oct 20, 2022 •

edited

Loading