Flatten requirements eagerly in get_dependencies
#4430
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
|
Does this affect non-forking resolution? |
If i implemented everything correctly, no, then it's just a refactoring, |
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
8665a88 to
4f77bc4
Compare
konstin
added a commit
that referenced
this pull request
Jun 21, 2024
## 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. ## Registry and URL requirement in the same package 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_requirement`, but we only fork after the current method. Instead, we can check if the registry and the URL requirement for the same name are disjoint: If they are, they must end up in different branches on forking, if they aren't, we know the URL overrides the registry. An alternative implementation would be splitting in the lookahead resolver so that all URLs are already tagged by the markers of their fork. This would require duplicating the forking logic while making sure that we follow the exact same precedence rules, which is why i didn't do it. ## Determining the URL of a Requirement For a registry requirement: * If there is a URL override: Use that override, done. * If there is a fork-specific URL: Use that, done. * If there is another requirement in the same package with a URL requirement find a matching URL in `Urls`, use it to canonicalize the current URL. * After forking: Check that this URL is the only URL in this fork. For a URL requirement: * If there is a URL override: Use that override, done. * Find a matching URL in `Urls`, use it to canonicalize the current URL. * After forking: Check that this URL is the only URL in this fork. ## 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 Best reviewed commit-by-commit, the tests are verbose. 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` yet.
charliermarsh
approved these changes
Jun 23, 2024
| if extra.is_none() && dev.is_none() { | ||
| for dev in &self.dev { | ||
| if metadata.dev_dependencies.contains_key(dev) { | ||
| // TODO(konsti): Why don't we eagerly pull in dev deps, like we do with extras? |
There was a problem hiding this comment.
What does this mean? Can I help explain it?
There was a problem hiding this comment.
Yes please, my question is: For the extras, we add the actual requirements from the extra to the dependencies; for dev-dependencies, why do we only add the proxy package?
konstin
added a commit
that referenced
this pull request
Jun 24, 2024
## 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. ## Registry and URL requirement in the same package 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_requirement`, but we only fork after the current method. Instead, we can check if the registry and the URL requirement for the same name are disjoint: If they are, they must end up in different branches on forking, if they aren't, we know the URL overrides the registry. An alternative implementation would be splitting in the lookahead resolver so that all URLs are already tagged by the markers of their fork. This would require duplicating the forking logic while making sure that we follow the exact same precedence rules, which is why i didn't do it. ## Determining the URL of a Requirement For a registry requirement: * If there is a URL override: Use that override, done. * If there is a fork-specific URL: Use that, done. * If there is another requirement in the same package with a URL requirement find a matching URL in `Urls`, use it to canonicalize the current URL. * After forking: Check that this URL is the only URL in this fork. For a URL requirement: * If there is a URL override: Use that override, done. * Find a matching URL in `Urls`, use it to canonicalize the current URL. * After forking: Check that this URL is the only URL in this fork. ## 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 Best reviewed commit-by-commit, the tests are verbose. 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` yet.
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
4f77bc4 to
4965708
Compare
konstin
added a commit
that referenced
this pull request
Jun 24, 2024
## 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. ## Registry and URL requirement in the same package 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_requirement`, but we only fork after the current method. Instead, we can check if the registry and the URL requirement for the same name are disjoint: If they are, they must end up in different branches on forking, if they aren't, we know the URL overrides the registry. An alternative implementation would be splitting in the lookahead resolver so that all URLs are already tagged by the markers of their fork. This would require duplicating the forking logic while making sure that we follow the exact same precedence rules, which is why i didn't do it. ## Determining the URL of a Requirement For a registry requirement: * If there is a URL override: Use that override, done. * If there is a fork-specific URL: Use that, done. * If there is another requirement in the same package with a URL requirement find a matching URL in `Urls`, use it to canonicalize the current URL. * After forking: Check that this URL is the only URL in this fork. For a URL requirement: * If there is a URL override: Use that override, done. * Find a matching URL in `Urls`, use it to canonicalize the current URL. * After forking: Check that this URL is the only URL in this fork. ## 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 Best reviewed commit-by-commit, the tests are verbose. 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` yet.
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
4965708 to
8fb9c69
Compare
konstin
added a commit
that referenced
this pull request
Jun 24, 2024
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
added a commit
that referenced
this pull request
Jun 24, 2024
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
added a commit
that referenced
this pull request
Jun 25, 2024
Upstack PR: #4430 Split out from #4430 according to #4430 (comment).
konstin
added a commit
that referenced
this pull request
Jun 25, 2024
Upstack PR: #4430 Split out from #4430 according to #4430 (comment).
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
9d5114f to
8cedb04
Compare
konstin
added a commit
that referenced
this pull request
Jun 25, 2024
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
added a commit
that referenced
this pull request
Jun 25, 2024
Upstack PR: #4430 Split out from #4430 according to #4430 (comment).
konstin
changed the base branch from
main
to
konsti/unify-dependency-iteration-in-get-dependencies
konstin
force-pushed
the
konsti/fork-before-transforming
branch
from
8cedb04 to
312e2f8
Compare
konstin
added a commit
that referenced
this pull request
Jun 25, 2024
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.
Base automatically changed from
konsti/unify-dependency-iteration-in-get-dependencies
to
main
June 25, 2024 21:04
konstin
added a commit
that referenced
this pull request
Jun 25, 2024
Upstack PR: #4430 Split out from #4430 according to #4430 (comment).
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.
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
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
added a commit
that referenced
this pull request
Jun 25, 2024
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
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.
charliermarsh
added a commit
that referenced
this pull request
Jun 26, 2024
## Summary In the dependency refactor (#4430), the logic for requirements and constraints was combined. Specifically, we were applying constraints _before_ filtering on markers and extras, and then applying that same filtering to the constraints. As a result, constraints that should only be activated when an extra is enabled were being enabled unconditionally. Closes #4569.
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.
Downstack PR: #4515 Upstack PR: #4481
Consider these two cases:
A:
B:
In the first case,
werkzeug==2.0.0should be overridden by the url. In the second caseiniconfig == 1.1.1is in a different fork and must remain a registry distribution.That means the conversion from
RequirementtoPubGrubPackageis 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 toPubGrubDependenciesinstead of after. Either version requires a flat list ofRequirements 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.