Replace PubGrubDependencies by PubGrubDependency
#4481
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
This was referenced Jun 24, 2024
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
9d5114f
to
8cedb04
Compare
konstin
force-pushed
the
konsti/PubGrubDependencies
branch
from
cfae851
to
8362f3b
Compare
BurntSushi
approved these changes
Jun 25, 2024
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
8cedb04
to
312e2f8
Compare
konstin
force-pushed
the
konsti/PubGrubDependencies
branch
from
8362f3b
to
9b7e47c
Compare
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
312e2f8
to
bed4bdb
Compare
konstin
added a commit
that referenced
this pull request
Jun 25, 2024
Downstack PR: #4515 Upstack PR: #4481 Consider these two cases: A: ``` werkzeug==2.0.0 werkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl ``` B: ```toml dependencies = [ "iniconfig == 1.1.1 ; python_version < '3.12'", "iniconfig @ git+https://github.com/pytest-dev/iniconfig@93f5930e668c0d1ddf4597e38dd0dea4e2665e7a ; python_version >= '3.12'", ] ``` In the first case, `werkzeug==2.0.0` should be overridden by the url. In the second case `iniconfig == 1.1.1` is in a different fork and must remain a registry distribution. That means the conversion from `Requirement` to `PubGrubPackage` is dependent on the other requirements of the package. We can either look into the other packages immediately, or we can move the forking before the conversion to `PubGrubDependencies` instead of after. Either version requires a flat list of `Requirement`s to use. This refactoring gives us this list. I'll add support for both of the above cases in the forking urls branch before merging this PR. I also have to move constraints over to this.
In the last PR (#4430), we flatten the requirements. In the next PR (#4435), we want to pass `Url` around next to `PubGrubPackage` and `Range<Version>` to keep track of which `Requirement`s added a url across forking. This PR is a refactoring split out from #4435 that rolls the dependency conversion into a single iterator and introduces a new `PubGrubDependency` struct as abstraction over `(PubGrubPackage, Range<Version>)` (or `(PubGrubPackage, Range<Version>, VerbatimParsedUrl)` in the next PR), and it removes the now unnecessary `PubGrubDependencies` abstraction.
konstin
force-pushed
the
konsti/PubGrubDependencies
branch
from
9b7e47c
to
c75b822
Compare
konstin
added a commit
that referenced
this pull request
Jun 26, 2024
Downstack PR: #4481 ## Introduction We support forking the dependency resolution to support conflicting registry requirements for different platforms, say on package range is required for an older python version while a newer is required for newer python versions, or dependencies that are different per platform. We need to extend this support to direct URL requirements. ```toml dependencies = [ "iniconfig @ https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl ; python_version >= '3.12'", "iniconfig @ https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl ; python_version < '3.12'" ] ``` This did not work because `Urls` was built on the assumption that there is a single allowed URL per package. We collect all allowed URL ahead of resolution by following direct URL dependencies (including path dependencies) transitively, i.e. a registry distribution can't require a URL. ## The same package can have Registry and URL requirements Consider the following two cases: requirements.in: ```text werkzeug==2.0.0 werkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl ``` pyproject.toml: ```toml dependencies = [ "iniconfig == 1.1.1 ; python_version < '3.12'", "iniconfig @ git+https://github.com/pytest-dev/iniconfig@93f5930e668c0d1ddf4597e38dd0dea4e2665e7a ; python_version >= '3.12'", ] ``` In the first case, we want the URL to override the registry dependency, in the second case we want to fork and have one branch use the registry and the other the URL. We have to know about this in `PubGrubRequirement::from_registry_requirement`, but we only fork after the current method. Consider the following case too: a: ``` c==1.0.0 b @ https://b.zip ``` b: ``` c @ https://c_new.zip ; python_version >= '3.12'", c @ https://c_old.zip ; python_version < '3.12'", ``` When we convert the requirements of `a`, we can't know the url of `c` yet. The solution is to remove the `Url` from `PubGrubPackage`: The `Url` is redundant with `PackageName`, there can be only one url per package name per fork. We now do the following: We track the urls from requirements in `PubGrubDependency`. After forking, we call `add_package_version_dependencies` where we apply override URLs, check if the URL is allowed and check if the url is unique in this fork. When we request a distribution, we ask the fork urls for the real URL. Since we prioritize url dependencies over registry dependencies and skip packages with `Urls` entries in pre-visiting, we know that when fetching a package, we know if it has a url or not. ## URL conflicts pyproject.toml (invalid): ```toml dependencies = [ "iniconfig @ https://files.pythonhosted.org/packages/44/39/e96292c7f7068e58877f476908c5974dc76c37c623f1fa332fe4ed6dfbec/iniconfig-1.1.0.tar.gz", "iniconfig @ https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl ; python_version < '3.12'", "iniconfig @ https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl ; python_version >= '3.12'", ] ``` On the fork state, we keep `ForkUrls` that check for conflicts after forking, rejecting the third case because we added two packages of the same name with different URLs. We need to flatten out the requirements before transformation into pubgrub requirements to get the full list of other requirements which may contain a URL, which was changed in a previous PR: #4430. ## Complex Example a: ```toml dependencies = [ # Force a split "anyio==4.3.0 ; python_version >= '3.12'", "anyio==4.2.0 ; python_version < '3.12'", # Include URLs transitively "b" ] ``` b: ```toml dependencies = [ # Only one is used in each split. "b1 ; python_version < '3.12'", "b2 ; python_version >= '3.12'", "b3 ; python_version >= '3.12'", ] ``` b1: ```toml dependencies = [ "iniconfig @ https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl", ] ``` b2: ```toml dependencies = [ "iniconfig @ https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", ] ``` b3: ```toml dependencies = [ "iniconfig @ https://files.pythonhosted.org/packages/44/39/e96292c7f7068e58877f476908c5974dc76c37c623f1fa332fe4ed6dfbec/iniconfig-1.1.0.tar.gz", ] ``` In this example, all packages are url requirements (directory requirements) and the root package is `a`. We first split on `a`, `b` being in each split. In the first fork, we reach `b1`, the fork URLs are empty, we insert the iniconfig 1.1.1 URL, and then we skip over `b2` and `b3` since the mark is disjoint with the fork markers. In the second fork, we skip over `b1`, visit `b2`, insert the iniconfig 2.0.0 URL into the again empty fork URLs, then visit `b3` and try to insert the iniconfig 1.1.0 URL. At this point we find a conflict for the iniconfig URL and error. ## Closing The git tests are slow, but they make the best example for different URL types i could find. Part of #3927. This PR does not handle `Locals` or pre-releases yet.
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.
In the last PR (#4430), we flatten the requirements. In the next PR (#4435), we want to pass
Urlaround next toPubGrubPackageandRange<Version>to keep track of whichRequirements added a url across forking. This PR is a refactoring split out from #4435 that rolls the dependency conversion into a single iterator and introduces a newPubGrubDependencystruct as abstraction over(PubGrubPackage, Range<Version>)(or(PubGrubPackage, Range<Version>, VerbatimParsedUrl)in the next PR), and it removes the now unnecessaryPubGrubDependenciesabstraction.