Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Correctly resolve requirement requested both as non-extra URL and non-URL with extras #9775

Merged
merged 4 commits into from
Apr 23, 2021
Merged

Correctly resolve requirement requested both as non-extra URL and non-URL with extras #9775

merged 4 commits into from
Apr 23, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Apr 4, 2021

Fix #8785. This depends resolvelib 0.6.0 and needs #9771 to be merged first.

@uranusjr uranusjr added the state: blocked Can not be done until something else is done label Apr 4, 2021
@uranusjr uranusjr changed the title Upgrade vendored resolvelib to 0.6.0 Correctly resolve requirement requested both as non-extra URL and non-URL with extras Apr 4, 2021
@uranusjr
Copy link
Member Author

uranusjr commented Apr 7, 2021

Note to self: #9644 should be able to be fixed with 0.6.0. Probably in a separate PR though, need to investigate.

@uranusjr uranusjr force-pushed the new-resolver-direct-url-with-extras branch from f8b84c7 to c02f0b6 Compare April 7, 2021 12:07
@uranusjr

This comment has been minimized.

Sign in to view
@uranusjr uranusjr force-pushed the new-resolver-direct-url-with-extras branch from c02f0b6 to b4fb4f2 Compare April 7, 2021 12:43
@uranusjr uranusjr marked this pull request as ready for review April 7, 2021 12:43
pradyunsg
pradyunsg approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a read-the-diff review, and this looks good to me!

@uranusjr uranusjr added this to the 21.1 milestone Apr 12, 2021
@uranusjr uranusjr removed the state: blocked Can not be done until something else is done label Apr 12, 2021
@uranusjr uranusjr force-pushed the new-resolver-direct-url-with-extras branch 2 times, most recently from b972040 to 6b22212 Compare April 12, 2021 20:01
@sbidoul
Copy link
Member

sbidoul commented Apr 17, 2021

I'm not familiar enough with the new resolver to do a meaningful review, so I'm going to merge this soon for 21.1 based on approvals and reputation. Holler if this is not ready for any reason.

@BrownTruck
Copy link
Contributor

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 master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 18, 2021
@uranusjr
Add failed test case
a31f8fd
@uranusjr
Implement extra-ed requirement merging
0305e0d 
When a requirement is requested multiple times, some via a direct URL
("req @ URL") and some not but with extras ("req[extra] VERSION"), the
resolver previous could not correctly find "req[extra]" if "req" is
available in an index.

This additional logic makes the resolver, when encountering a
requirement with identifier "req[extra]", to also look for explicit
candidates listed under "req", and add them as found matches for
"req[extra]".
@uranusjr uranusjr force-pushed the new-resolver-direct-url-with-extras branch from 6b22212 to 0305e0d Compare April 19, 2021 00:37
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Apr 19, 2021
@uranusjr
Copy link
Member Author

I added a commit to refactor Factory.find_candidates() since the function is becoming incredibly long for all the edge cases introduced recently. The function should be much more readable now.

@uranusjr uranusjr force-pushed the new-resolver-direct-url-with-extras branch 2 times, most recently from 8c2d448 to febe770 Compare April 19, 2021 04:09
@uranusjr uranusjr force-pushed the new-resolver-direct-url-with-extras branch from febe770 to 9cab983 Compare April 19, 2021 04:21
@sbidoul
Copy link
Member

sbidoul commented Apr 20, 2021

@uranusjr CI is red.

@uranusjr
Copy link
Member Author 

E       ValueError: signal only works in main thread of the main interpreter

That xdist bug again...

@uranusjr uranusjr closed this Apr 20, 2021
@uranusjr uranusjr reopened this Apr 20, 2021
@uranusjr
Remove stray id(candidate) call
cf4e3aa 
Also moves the incompatibility candidate calculation to closer to their
usages.
@sbidoul
Copy link
Member

sbidoul commented Apr 23, 2021 •
edited
Loading

This seems to work fine. Is it good to go for you @uranusjr ?

@uranusjr
Copy link
Member Author

Yup, feel free to merge 🙂

@sbidoul sbidoul merged commit 4b8004a into pypa:main Apr 23, 2021
@di di mentioned this pull request Apr 24, 2021
Open
@yuvalmarciano yuvalmarciano mentioned this pull request Apr 26, 2021
Closed
@uranusjr uranusjr mentioned this pull request May 31, 2021
Closed
altendky added a commit to altendky/ssst that referenced this pull request Jul 5, 2021
@altendky
Reduce QTrio version repetition
cb5fc01 
Shouldn't be needed in pip >= 21.1.

pypa/pip#9437
pypa/pip#8785
pypa/pip#9775
This was referenced Jul 5, 2021
Draft
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
@uranusjr uranusjr deleted the new-resolver-direct-url-with-extras branch December 4, 2021 13:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sbidoul sbidoul sbidoul left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21.1
Development

Successfully merging this pull request may close these issues.

New resolver: Failure when package only specified with extras is not available from index
5 participants
@uranusjr @sbidoul @BrownTruck @pradyunsg @pypa-bot