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

Discard merged any marker constraints #7098

Conversation

radoering
Copy link
Member

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.

@radoering radoering requested a review from a team November 25, 2022 16:59
src/poetry/puzzle/provider.py Outdated Show resolved Hide resolved
src/poetry/puzzle/provider.py Outdated Show resolved Hide resolved
src/poetry/puzzle/provider.py Show resolved Hide resolved
@radoering radoering force-pushed the discard-merged-any-marker-constraints branch from b0aef76 to 16fb395 Compare November 25, 2022 17:14
@dimbleby
Copy link
Contributor

dimbleby commented Nov 26, 2022

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Contributor

dimbleby commented Nov 30, 2022

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 radoering force-pushed the discard-merged-any-marker-constraints branch from 8204654 to 641898c Compare November 30, 2022 16:35
@radoering
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

... 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
Copy link
Contributor

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

…er merging is empty, not compatible with the project's python constraint or not compatible with the set environment
@radoering radoering force-pushed the discard-merged-any-marker-constraints branch from 641898c to ee80212 Compare December 1, 2022 04:47
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

LGTM!

@neersighted neersighted merged commit b5ab46e into python-poetry:master Dec 1, 2022
@neersighted neersighted added kind/bug Something isn't working as expected area/solver Related to the dependency resolver impact/changelog Requires a changelog entry labels Dec 21, 2022
@neersighted neersighted added this to the 1.3 milestone Dec 21, 2022
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/solver Related to the dependency resolver impact/changelog Requires a changelog entry kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants