New Resolver: Add support for direct URLs with extras #8291
Conversation
| @@ -417,20 +417,16 @@ def iter_dependencies(self): | |||
| extra | |||
| ) | |||
|
|
|||
| # Add a dependency on the exact base. | |||
| yield factory.make_requirement_from_candidate(self.base) | |||
There was a problem hiding this comment.
This would clone a pkg>=1.0 without pinning the version if base is not pinned. I think we need to introduce some kind of abstraction to BaseCandidate to handle this.
There was a problem hiding this comment.
I'm not sure I follow. self.base is a candidate.
There was a problem hiding this comment.
I sort of recall there’s a thing that may go wrong with this, but I forgot what that is now :( Maybe it’s no longer a problem with the new find_matches() implementation.
There was a problem hiding this comment.
Ah, there is a different problem if constraints are involved. We don’t allow constraints to filter direct requirements, so given the following scenario:
# constraints.txt
foo>=1.0
# requirements.txt
foo[add]==1.0
This will fail with installation from path or url cannot be constrained to a version because we are adding a URL requirement to represent the base.
I’m thinking maybe we should get rid of the check altogether, since constraining a URL requirement by checking the version the URL points to kind of still makes sense? @pfmoore
There was a problem hiding this comment.
I think that's probably true. I suspect the limitation was there because the old resolver couldn't cope with that situation.
But if we relax that restriction, we'll get additional test failures, and I think we should start being much stricter about not allowing regressions like that (I've just submitted #8297 which should help a bit here, and the fact that the new markers mean that the test suite now succeeds on the new resolver will help as well) so we'd need to fix those tests at the same time (and not by just adding the "fails with the new resolver" flag 🙂 - maybe an xfail on the new resolver?).
There was a problem hiding this comment.
Maybe it’s no longer a problem with the new
find_matches()implementation.
I think it was a much bigger problem before we had the new implementation, but maybe not so much now. Let's assume it works for the moment, and then when stuff breaks, we'll have a better idea of where there's still an issue (and we can blame @pradyunsg 🙂)
There was a problem hiding this comment.
we can blame @pradyunsg
Oh no. 😮
pradyunsg
force-pushed
the
direct-url-with-extras
branch
from
9d65df4 to
a0e4eb6
Compare
pradyunsg
added
the
skip news
pradyunsg
force-pushed
the
direct-url-with-extras
branch
from
66b333a to
50c9ea2
Compare
| ireq.link, extras=set(ireq.extras), parent=ireq, | ||
| ) | ||
| return ExplicitRequirement(cand) | ||
| return self.make_requirement_from_candidate(candidate) |
There was a problem hiding this comment.
Not really related, but this might read better if we flip the conditionals (if not ireq.link:)
There was a problem hiding this comment.
Didn't I see exactly that change in a different PR I was reviewing a few minutes ago? There may be merge conflicts in our future... But yes, I agree switching the order reads better.
There was a problem hiding this comment.
Seems like my coding style is fairly consistent 😆
There was a problem hiding this comment.
I wanted to make the same change, but resisted the urge in favor of keeping the PR smaller. :)
Having this come in via #8066 works well for me.
Fixes #8289
Waiting on #8276 prior to updating the tests' markers. A quick manual test and local test run confirms that this should fix the issue. :)