-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wheel cache link look up in the new resolver #8066
Conversation
9cfa997
to
1f0adb5
Compare
Alright, I got @sbidoul Would you mind clarifying what Edit: I’m failing |
1f0adb5
to
feed67d
Compare
@uranusjr In pip/src/pip/_internal/resolution/legacy/resolver.py Lines 290 to 291 in 6f7d5b9
is meant to record if the original link was found in cache. As you noted, this only necessary so far to know that the VCS link contained a commit id without actually doing a VCS checkout. 'hope this helps. I've tried to look into this PR but I'm still struggling with the concept of creating an InstallRequirement from a parent InstallRequirement and a link. |
So what are the implications of The name |
Interlinking #8005 (comment) |
I think it’s better to pause work on this until #8005 goes in. The internal link state in |
Also need to consider #8123 because wheel cache lookup loses us the yanked information. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
feed67d
to
ed8b7ee
Compare
Yay, I think I’ve managed to solve the PEP 610 related logic. We still need to deal with the problem that a wheel cache lookup looses the |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
ed8b7ee
to
37b4598
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the answer to the TODO is "yes", LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one typo and a couple of minor comments, this LGTM
I'm faster than you @BrownTruck! |
f07c682
to
225087e
Compare
Python 2 is always going to be a PITA. :) |
The “Linux” job is stale from before the CI change. It will stay failed because now I can’t trigger it to pass anymore (without readding the job). |
Is that an Azure thing? Why isn't it possible to rerun the job? I have a "Re-run" link on the check results page, I'll click that in case it helps. |
Ah yes, I see, that was one of the renamings @pradyunsg did. OK, so I guess the only options is to rebase on master. Merging with failing checks isn't allowed. |
This is useful when resolving the wheel cache.
Co-authored-by: Paul Moore <p.f.moore@gmail.com>
9a3868e
to
80f3b3e
Compare
You won this race Paul, but I won't say that was graceful. I'd taken a break for dinner. :P |
When installing a cached VCS requirement, I can see that the cache lookup from this PR does correctly find the cache entry. However when reaching |
OOoo. So, I just noticed that this breaks the presentation logic in
Notice the initial wheel files that are from the cache, printed in full. |
Very much a work in progress. I managed to fix
test_pip_wheel_build_cache
, but nottest_install_git_sha_cached
. I’m not even sure if the latter one is actually related to the wheel cache now.I’m also having trouble implementing PEP 610 (equivalent of #7612).
test_install_find_links_no_direct_url
does not pass, and I can’t figure out if myoriginal_link_is_in_wheel_cache
is sane. Are these two the same thing? I have no idea :(