Discard merged any marker constraints

[radoering]

Resolves an issue revealed in #6950 (comment)

Based on #4590, after adapting the marker of "any marker dependencies", these dependencies have to be discarded if the marker is empty, not compatible with the project's python constraint or (during install) not compatible with the environment.

• Added tests for changed code.
• Updated documentation for changed code.

[dimbleby]

I have doubts about correctness, especially in cases where the requirements actually are incompatible.

For instance consider

requests = [
  { version = "=2.24.0" },
  { version = "=2.25.0", markers = "implementation_name == 'pypy'" }
]

the result of this is that 2.24.0 is chosen unconditionally, which one way or the other surely wasn't what the user intended.

It would certainly be simpler to make the user responsible for ensuring that their markers are non-overlapping, rather than trying to infer what they probably meant.

That is, perhaps the simpler approach is just to search for, and refuse to process, overlapping markers.

Alternatively poetry could search for overlapping markers and reject them if and only if they have contradictory constraints - but still do something like what this MR does when their constraints are compatible. Feels like it might be getting a bit complicated though...

Edit: also the special-casing of "any" bothers me. That's not the only way to write overlapping markers.

[radoering]

I have doubts about correctness, especially in cases where the requirements actually are incompatible.

Good catch. (This has already been an issue before this PR.) I just added a fix for that, too.

It would certainly be simpler to make the user responsible for ensuring that their markers are non-overlapping, rather than trying to infer what they probably meant.

It would, but I'd rather support compatible overlapping markers if there isn't any PEP that says overlapping markers are forbidden.

also the special-casing of "any" bothers me. That's not the only way to write overlapping markers.

That's right. At the moment we are just dealing with one more common case of overlapping markers. Or more exactly with two cases because _merge_dependencies_by_marker is handling the special case of completely overlapping markers. I already have a follow up PR in mind to deal with overlapping markers in general, i.e. generalizing _handle_any_marker_dependencies and removing _merge_dependencies_by_marker (and probably also some of the code in between the two calls).

[neersighted]

In general, I think we're committed to this formulation of markers, as to do otherwise would be a breaking change, and likely break existing projects. I'm reasonably sure our extras implementation relies on part of this as well. Overall, I think trying to refactor this to be cleaner and better abstracted/architected over time is the only way forward.

[dimbleby]

this one makes my head hurt! perhaps it would be sensible to unblock #6950 and therefore the 1.3.0 release by rewriting the markers in that one so that they don't need this fix?

[radoering]

I was hoping that we were pretty far along with this review. Actually, this is just a small fix. Partial support for overlapping markers was not introduced here, but was already present before this PR. Anyway, IMO this doesn't really block the release as long as nobody has updated the changelog. 😉

[dimbleby]

IMO this doesn't really block the release

as I understood it, #6950 was being considered blocking - because if the release is cut without it then Windows python 3.9 users who are using the embedded pip will be broken.

And currently #6950 either needs to be reworked, or relies on this being merged first.

[radoering]

That's right. I just mean reworking #6950 is just a matter of minutes but even with that the changelog has to be updated and that's the real work. We might as well wait with reworking #6950 until that's the only thing the release is waiting for. (As you have probably already noticed, I'd prefer to keep #6950 as is which requires this PR.)

[dimbleby]

... until that's the only thing the release is waiting for

So far as I can tell from #6972, #6950 is the only thing the release is waiting for

[dimbleby]

left a note suggesting a little tidying, otherwise looks good to me

[neersighted]

LGTM!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.