Always merge extras from existing and incoming requirements

[uranusjr]

Fix #6239. I added a unit test based on the issue to reflect the situation, so this can be caught if we fail to implement this in the new resolver.

[pradyunsg]

This would merge extras from constraints too, IIUC.

Could you fix that (by moving it under the conditional) and add a unit test to ensure that doesn't happen in the future?

[uranusjr]

Could you fix that (by moving it under the conditional)

That’s exactly the old code 😛 I think the conditional needs to be split to make this work.

[uranusjr]

I’ve updated the code to not merge extras from an incoming constraint, and add cases to cover variants.

Please help me make sure the new comments about early returns make sense.

[pradyunsg]

Logic change and comments look good to me; though I haven't actually rigorously tested this.

[pradyunsg]

I think the conditional needs to be split to make this work.

Ah well. Sorry, I reviewed this on the web UI which is pretty bad for showing such things.

---

Does this fix the test in yaml/install/extras[3]? I think it would but again, haven't taken a good look yet.

[uranusjr]

Let’s give it a shot.

[uranusjr]

OK I get the test still fails… but why does it pass sometimes?

[cjerdonek]

I only glanced at this, but could it be because existing_req.extras isn't sorted in this line if new_extras != existing_req.extras:?

[uranusjr]

Not likely since new_extras is always a superset of existing_req.extras. But the current code is unnecessarily confusing, I’ll update it a bit to make intentions clear.

[uranusjr]

Hmmm getting the same failure, but from different environments. Interesting.

[pradyunsg]

the current code is unnecessarily confusing

I have confirmed this finding independently. 😉

[pradyunsg]

See also #3878.

[pradyunsg]

More specifically: #3878 (comment)

This seems clean enough but this would (if I'm not mistakenà merge a pkg[extra1]<1 and a pkg[extra2]>=2 into a pkg[extra1,extra2]<1 (or >=2 depending on the resolution order).

Which is a good point. ;)

[pradyunsg]

@uranusjr Did you find the time to take a look at this?

[uranusjr]

Sorry, I’ve been caught up at work recently, and not found much energy to work on this.

The problem this PR faces is that merging extras is messy without a proper resolver. It is mot difficult to solve the original issue (#6239), but merging in general (say merging A[x] with A[y] with different constraint combination) is difficult to get right. I’m sure there is a way, but don’t have the energy at the moment to really dive into the logic and investigate. Maybe it would be better to just solve the problem mentioned in the issue, and leave the general solution until the resolver is swapped out?

[pradyunsg]

Closing this, since well, none of us have the bandwidth to tackle this right now AFAICT.

[brainwane]

Let's reopen this now since we do have bandwidth now. :-)

[pypa-bot]

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 eligible for code review and hopefully merging!

[pradyunsg]

@brainwane This is a PR for pip's current "legacy" resolver. This specific behavior that this PR is for, has been fixed with the new resolver.

I don't think this should be revisited since we don't want to be trying to fix such bugs in the old resolver (it's painfully difficult, and that's why we're writing a new one!). I really think that we should leave this bug alone for now, letting it die along with the old resolver code.

[pradyunsg]

@pypa/pip-team Please make sure to unlock old issues/PRs when you re-open them, since we have @lock[bot] that automatically locks old+closed issues.

[uranusjr]

Maybe we should have the bot do that for us automatically. Where should I contribute to the bot?

[pradyunsg]

https://github.com/dessant/lock-threads, I believe.

[brainwane]

Sorry for misunderstanding, and thanks for the clarification and moving it to the right place, Pradyun!

[pradyunsg]

I don't think this should be revisited since we don't want to be trying to fix such bugs in the old resolver (it's painfully difficult, and that's why we're writing a new one!). I really think that we should leave this bug alone for now, letting it die along with the old resolver code.

Re-closing based on this rationale. :)