Conversation
| let dependencies = match dependencies { | ||
| Ok(dependencies) => dependencies, | ||
| Err(err) => { | ||
| return Ok(Dependencies::Unavailable( | ||
| UnavailableVersion::ResolverError(uncapitalize(err.to_string())), | ||
| )); | ||
| } | ||
| }; |
There was a problem hiding this comment.
@charliermarsh Do you know what this was about? It's only in this branch (in the non-root package branch we short-circuit on error) and the errors we're handling here look like they should be raised.
There was a problem hiding this comment.
Why should they be raised? You don't think we should backtrack in these cases?
There was a problem hiding this comment.
As far as i can tell, the current implementation means that if there is a direct dependency (a root package dependency) where PubGrubSpecifier::try_from fails or which uses a banned URL, we mark those dependencies as unavailable, so the root package becomes unavailable(?). For regular dependencies, we already raise those errors directly using from_requirements(...)?.
This code looks like an artifact to me, but going through the git history unfortunately didn't clear this up either.
There was a problem hiding this comment.
It looks like there are some errors that we'd want to recover from though, like InvalidVersion or perhaps PubGrubSpecifierError?
There was a problem hiding this comment.
I don't think they are recoverable, as far as i can tell they lead to the root package being rejected in the next step, so you get the same error except it's wrapped in a message about the root package being unavailable.
There was a problem hiding this comment.
We also do this only for the root package, which looks wrong.
There was a problem hiding this comment.
Where are they rejected? Note that this does not return an error; it returns Ok(Dependencies::Unavailable).
There was a problem hiding this comment.
This code is unreachable (both error cases are raised earlier), so i'm removing it
Upstack PR: #4430 Split out from #4430 according to #4430 (comment).
konstin
force-pushed
the
konsti/unify-dependency-iteration-in-get-dependencies
branch
from
7eb4fdc to
384a290
Compare
konstin
deleted the
konsti/unify-dependency-iteration-in-get-dependencies
branch
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.
2 participants
Upstack PR: #4430
Split out from #4430 according to #4430 (comment).