Move conflicting dependencies into PubGrub #1796
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(No reviews yet, this doesn't pass the test suite.) |
|
\cc @zanieb for visibility. |
charliermarsh
force-pushed
the
charlie/transitive
branch
9 times, most recently
from
40304a0
to
e5368db
Compare
charliermarsh
force-pushed
the
charlie/transitive
branch
5 times, most recently
from
2ccd7c9
to
b5fc027
Compare
charliermarsh
commented
Feb 21, 2024
| ╰─▶ Because only albatross==1.0.0 is available and albatross==1.0.0 depends on crow>=2.0.0b1, we can conclude that all versions of albatross depend on crow>=2.0.0b1. | ||
| And because only crow<2.0.0b1 is available, we can conclude that all versions of albatross depend on crow>3.0.0. | ||
| And because bluebird==1.0.0 depends on crow and only bluebird==1.0.0 is available, we can conclude that all versions of albatross and all versions of bluebird are incompatible. | ||
| And because you require albatross and you require bluebird, we can conclude that the requirements are unsatisfiable. |
There was a problem hiding this comment.
I believe these are generally the result of dependencies being presented to PubGrub in a different order.
There was a problem hiding this comment.
Yeah it's a little weird but a known issue.
charliermarsh
force-pushed
the
charlie/transitive
branch
from
b5fc027
to
b599711
Compare
charliermarsh
force-pushed
the
charlie/transitive
branch
3 times, most recently
from
b22f853
to
9573239
Compare
zanieb
reviewed
Feb 21, 2024
crates/uv/tests/pip_compile.rs
Outdated
Comment on lines
1279
to
1281
| × No solution found when resolving dependencies: | ||
| ╰─▶ your requirements cannot be used because there are conflicting URLs for | ||
| package `werkzeug`: | ||
| - https://files.pythonhosted.org/packages/bd/24/11c3ea5a7e866bf2d97f0501d0b4b1c9bbeade102bb4b588f0d2919a5212/Werkzeug-2.0.1-py3-none-any.whl | ||
| - https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl | ||
| error: There are conflicting URLs for package `werkzeug`: | ||
| - https://files.pythonhosted.org/packages/bd/24/11c3ea5a7e866bf2d97f0501d0b4b1c9bbeade102bb4b588f0d2919a5212/Werkzeug-2.0.1-py3-none-any.whl | ||
| - https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl |
There was a problem hiding this comment.
Is this an improvement? Should we clarify that the URLs come from the provided requirements?
zanieb
reviewed
Feb 21, 2024
Comment on lines
-1601
to
+1626
| ╰─▶ my-project cannot be used because there are conflicting versions for | ||
| `django`: `django==5.0b1` does not intersect with `django==5.0a1` | ||
| ╰─▶ Because my-project depends on django==5.0b1 and my-project depends on | ||
| django==5.0a1, we can conclude that the requirements are unsatisfiable. |
There was a problem hiding this comment.
Nice. It'll be better once we combine clauses #1009, maybe someday we should even track the source of dependencies so we can say what file each of these came from.
zanieb
reviewed
Feb 21, 2024
crates/uv/tests/pip_compile.rs
Outdated
Comment on lines
3753
to
3782
| /// Resolve a package from a `requirements.in` file, with a `constraints.txt` file pinning it to | ||
| /// a specific URL. |
zanieb
reviewed
Feb 21, 2024
Comment on lines
+4000
to
+4030
| × No solution found when resolving dependencies: | ||
| ╰─▶ Because you require filelock==1.0.0 and you require filelock==3.8.0, we | ||
| can conclude that the requirements are unsatisfiable. |
There was a problem hiding this comment.
This one definitely makes me want to open an issue to display the locations of direct requirements in error messages.
zanieb
reviewed
Feb 21, 2024
Cargo.toml
Outdated
| pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "9b6d89cb8a0c7902815c8b2ae99106ba322ffb14" } | ||
| pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "5d0270cc2932ab233bb7f66a09fb0764fed7154e" } |
There was a problem hiding this comment.
Let's make sure to merge upstream and use the main commit before merging this one.
zanieb
approved these changes
Feb 21, 2024
charliermarsh
force-pushed
the
charlie/transitive
branch
5 times, most recently
from
6bfed7a
to
4df39a9
Compare
charliermarsh
force-pushed
the
charlie/transitive
branch
from
4df39a9
to
7e7e523
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This revives a PR from long ago (#383 and astral-sh/pubgrub#24) that modifies how we deal with dependencies that are declared multiple times within a single package.
To quote from the originating PR:
The main issue with that PR was around its handling of URL dependencies, so this PR also refactors how we handle those. Previously, we stored URL dependencies on
PubGrubPackage, but they were omitted from the hash and equality implementations ofPubGrubPackage. This led to some really careful codepaths wherein we had to ensure that we always visited URLs before non-URL packages, so that the URL-inclusive versions were included in any hashmaps, etc. I considered preserving this approach, but it would require us to rely on lots of internal details of PubGrub (since we'd now be relying on PubGrub to merge those packages in the "right" order).So, instead, we now always set the URL on a given package, whenever that package was given a URL upfront. I think this is easier to reason about: if the user provided a URL for
flask, then we should just always add the URL forflask. If we see some other URL forflask, we error, like before. If we see some unknown URL forflask, we error, like before.Closes #1522.
Closes #1821.
Closes #1615.