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

poetry check does not take into a account path dependencies #8633

Open
adriangb opened this issue Nov 7, 2023 · 4 comments
Open

poetry check does not take into a account path dependencies #8633

adriangb opened this issue Nov 7, 2023 · 4 comments
Labels
kind/bug Something isn't working as expected status/triage This issue needs to be triaged

Comments

@adriangb
Copy link
Contributor

adriangb commented Nov 7, 2023

If you have a path dependency and add a dependency to its pyproject.toml file then run poetry check --lock it will say everything is OK when it shouldn't be. It seems to me that in general the mechanism to check if the lockfile is up to date with pyproject.toml should take into account path dependencies.

I can think of two solutions:

  • Include the build files for every path dependency in the content-hash metadata of poetry.lock.
  • Never consider ourselves locked if there are path dependencies.

The first approach suffers from the problem that these path dependencies might not even use a pyproject.toml file / PEP 517. They could use setup.py or something super dynamic.
The second approach would be slow for the common case of path dependencies that do use PEP-517 (and likely even use poetry).

A compromise is best: if all of the path dependencies are PEP-517 dependencies then assume that the build backend is deterministic (I think this is part of the PEP / a fair assumption) and just check the content-hash (which would include a hash of all of the pyproject.toml files). If any path dependency is not PEP-517 compliant fall back to never assuming the lockfile is up to date based on the hash.

Does this seem reasonable? If so I can work on an implementation.

@adriangb adriangb added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Nov 7, 2023
@adriangb
Copy link
Contributor Author

adriangb commented Nov 7, 2023

cc @radoering since we worked together previously on #6843 and related issues

@radoering
Copy link
Member

if all of the path dependencies are PEP-517 dependencies then assume that the build backend is deterministic

I suppose with "PEP-517 dependencies" you are referring to projects that declare their dependencies according to PEP 621 in a dependencies section in the pyproject.toml (or in case of poetry in tool.poetry.dependencies).

the content-hash (which would include a hash of all of the pyproject.toml files)

I'd probably keep hashes of path dependencies' pyproject.toml content separately. That way we can still check the content-hash of the project itself even if some path dependencies, which might not be relevant for the current environment, are not available.

If any path dependency is not PEP-517 compliant fall back to never assuming the lockfile is up to date based on the hash.

I don't like the idea that poetry check --lock will always fail and you can't do anything about it. That should probably be opt-in.

In general, I tend to think that we should make such a behavior opt-in. There are several options for making it optional (not sure which would be best):

  • introduce a local config setting (so that users who want this behavior can activate it for some or all of their projects)
  • introduce another flag to the check command or make --lock accept values
  • make check lock a subcommand so that only the lockfile is checked (cf https://github.com/orgs/python-poetry/discussions/8583#discussioncomment-7417068) and add an option to it, which might make sense if further options for checking the lockfile will come up

@adriangb
Copy link
Contributor Author

I suppose with "PEP-517 dependencies" you are referring to projects that declare their dependencies according to PEP 621 in a dependencies section in the pyproject.toml (or in case of poetry in tool.poetry.dependencies).

Not really. I meant dependencies for which their sub-dependencies can be statically determined without executing code. That is, not a setup.py.

I'd probably keep hashes of path dependencies' pyproject.toml content separately. That way we can still check the content-hash of the project itself even if some path dependencies, which might not be relevant for the current environment, are not available.

Yup that makes sense!

I don't like the idea that poetry check --lock will always fail and you can't do anything about it. That should probably be opt-in.

I'd rather make it opt-out, the only users that would be impacted are those that are using path dependencies and those path dependencies are using setup.py / we can't statically determine the sub-dependencies and need to hash them. I'm guessing that's a small subset of users, I'd rather make the behavior safer / more correct for the rest of users even if we annoy that small fraction a bit. A compromise could be to still resolve the dependencies by running setup.py (or whatever Poetry would do to install the project) and emit a warning saying poetry check --lock will be slow because .... But I'm not sure how technically complex that might be vs. a --check-lock-on-path-dependencies-using-setup-py (or whatever a boolean flag would be called).

@sfarina
Copy link

sfarina commented Mar 26, 2024

Facing this at the moment with editable dependencies that have changed. I expect the project that relies on the editable dependencies to fail a check.

schematically:
HighLevelCode uses an editable install of ProjectLibrary
ProjectLibrary depends on version 1.0 of SomeLibrary
lock HighLevelCode, it depends on SomeLibrary v1.0
ProjectLibrary updates the dependency of SomeLibrary to 2.0
HighLevelCode is still locked to SomeLibrary v1.0, it needs to be re-locked, so poetry check --lock should fail

 ~/r/d/s/api $ poetry check
All set!
 ~/r/d/s/api $ poetry lock --check
poetry lock --check is deprecated, use `poetry check --lock` instead.
poetry.lock is consistent with pyproject.toml.
 ~/r/d/s/api $ poetry check --lock 
All set!

edit: sorry if this is missing the point of this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working as expected status/triage This issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants