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

Package.satisfies() considers sources more carefully #497

Merged

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Oct 8, 2022

Package.satisfies(dependency) has been asking the wrong question about the sources: it doesn't need the sources to be identical, it needs the package source to satisfy the dependency source.

These are different mostly because of the confusing handling of pypi and legacy repositories

  • on a dependency, setting source type and source name to None means "any source is fine"
  • on a dependency, setting source type to None but specifying source name means "this specific repository"
  • on a package, setting source type to None means "pypi"

I've also removed the check on features: it's not wanted by callers of this method in poetry, so it was just getting in the way.

A companion MR in poetry will follow, between them these should fix python-poetry/poetry#6710

@neersighted
Copy link
Member

neersighted commented Oct 10, 2022

In the medium term PEP 658 should hopefully be supported by PyPI, and PyPiRepository and LegacyRepository will collapse into SimpleRepository or similar, with a type of simple or index. That will clean this up quite a bit. In the medium term, how invasive would it be to make the source_type for PypiRepository pypi to reduce this ambiguity?

@dimbleby
Copy link
Contributor Author

how invasive would it be to make the source_type for PypiRepository pypi

moderately. eg this gets written to the lockfile so it's a breaking change there.

@neersighted
Copy link
Member

We have yet to release lock file 2.0 into the wild, so this is a very convenient time to do it.

@dimbleby
Copy link
Contributor Author

still, seems like more effort than it justifies to me - doubly so if this is all liable to be undone by PEP691 in the foreseeable future.

If we were starting from scratch I certainly wouldn't want to use None to mean "pypi": but it is not worse than confusing...

@neersighted
Copy link
Member

691 is the JSON format and already supported -- I think you mean 658, which is the METADATA file.

You're not wrong and your comment in this PR is more than sufficient for future code spelunkers. It was simply an observation that if you wanted to solve this wart, this is an excellent time.

@radoering radoering force-pushed the packages-dependencies-source-satisfies branch from c820cad to d539a0b Compare October 15, 2022 12:49
@sonarcloud
Copy link

sonarcloud bot commented Oct 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@radoering radoering merged commit a0b2fa5 into python-poetry:main Oct 15, 2022
@dimbleby dimbleby deleted the packages-dependencies-source-satisfies branch October 15, 2022 17:02
DavidVujic pushed a commit to DavidVujic/poetry-core that referenced this pull request Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enviroment markers ignored when multiple version options coexist with legacy source
3 participants