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.

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

Fixed bug in extras handling for link requirements #12392

Merged
merged 2 commits into from
Dec 17, 2023

Conversation

sanderr
Copy link
Contributor

@sanderr sanderr commented Nov 6, 2023

Fixed bug in extras handling where constraints on a package are not compatible with an explicit link candidate that also has an extra.

Closes #12372

return base
return self._make_extras_candidate(base, extras, comes_from=template)

def _make_base_candidate_from_link(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_make_candidate_from_link actually always returned a BaseCandidate when no extras where specified. The new extras handling requires a base candidate so I extracted this part.

@notatallshaw notatallshaw mentioned this pull request Nov 16, 2023
@sbidoul sbidoul added this to the 23.3 milestone Nov 17, 2023
@sbidoul
Copy link
Member

sbidoul commented Nov 17, 2023

@uranusjr if you are available to review this that would be super helpful.

@notatallshaw
Copy link
Member

Recently created a workflow which involves installing local extras, tested this branch and everything works fine.

@sbidoul
Copy link
Member

sbidoul commented Dec 10, 2023

I'm preparing to do the last 23.3 patch release and I'm trying to determine if this PR fixes a regression of 23.3.

To do so, I've tried to run the test added in this PR (test_new_resolver_constraint_on_link_with_extra) and it looks like it fails equally in 23.1.2 and 23.2.1. @sanderr Could it be this PR addresses something new, or that the test you added does not actually reveal the regression but something else?

@sanderr
Copy link
Contributor Author

sanderr commented Dec 10, 2023

@sbidoul This is definitely a fix for a regression in 23.3. But you're right that the test case doesn't cover the regression directly. I have to admit I can't exactly recall all details, but I believe the test case covers a case that is strictly broader than the regression. Either way, I've just added a second test case that more closely resembles the scenario reported in #12373, and I've confirmed that it succeeds on both this branch and 23.2.1, while it fails on 23.3.1.

@sbidoul
Copy link
Member

sbidoul commented Dec 17, 2023

Thank you for your work on this @sanderr.

@sbidoul sbidoul merged commit 417ca92 into pypa:main Dec 17, 2023
26 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip install local with extra fails dependency resolution
3 participants