Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-existant path dependencies #507

Closed
66 changes: 44 additions & 22 deletions src/poetry/core/packages/directory_dependency.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -12,6 +13,8 @@


class DirectoryDependency(Dependency):
_full_path: Path | None

def __init__(
self,
name: str,
Expand All @@ -24,26 +27,10 @@ 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")

self._full_path = None
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"
)
full_path = self._get_full_path(False, name)

super().__init__(
name,
Expand All @@ -52,20 +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 _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 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(full_path):
raise ValueError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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:
Expand All @@ -76,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
Expand Down
54 changes: 38 additions & 16 deletions src/poetry/core/packages/file_dependency.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if self._full_path is not None:
return self._full_path
Comment on lines +43 to +44
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Expand All @@ -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)

Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/non_python_project/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# missing build section
2 changes: 1 addition & 1 deletion tests/packages/test_directory_dependency.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

def test_directory_dependency_must_exist() -> None:
with pytest.raises(ValueError):
DirectoryDependency("demo", DIST_PATH / "invalid")
DirectoryDependency("demo", DIST_PATH / "invalid").full_path


def _test_directory_dependency_pep_508(
Expand Down
2 changes: 1 addition & 1 deletion tests/packages/test_file_dependency.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



def test_file_dependency_dir() -> None:
Expand Down
12 changes: 0 additions & 12 deletions tests/test_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)


Comment on lines -301 to -312
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def test_create_poetry_with_groups_and_legacy_dev() -> None:
poetry = Factory().create_poetry(
fixtures_dir / "project_with_groups_and_legacy_dev"
Expand Down