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

fix: check for mutually exclusive markers #7970

Conversation

ralbertazzi
Copy link
Contributor

Pull Request Check List

Resolves: #5066

I might be mistaken, but I don't see cases where one wants to specify multiple constraints for the same dependency where markers have an intersection.

  • Added tests for changed code.

@ralbertazzi ralbertazzi force-pushed the fix/check-for-mutually-exclusive-markers branch from 7a57db5 to 27fee87 Compare May 20, 2023 13:03
@dimbleby
Copy link
Contributor

this is obviously pulling in the exact opposite direction than #7257

poetry itself used overlapping markers until very recently -

poetry/pyproject.toml

Lines 80 to 84 in c4c857e

virtualenv = [
{ version = "^20.4.3,!=20.4.5,!=20.4.6" },
# see https://github.com/python-poetry/poetry/pull/6950 for details
{ version = "<20.16.6", markers = "sys_platform == 'win32' and python_version == '3.9'" },
]

presumably forbidding something that is currently allowed will be a breaking change for anyone who is doing that thing

@radoering
Copy link
Member

radoering commented May 20, 2023

That goes too far. There are such cases. The most prominent one is something like:
(some general constraints and some more constraints depending on markers)

poetry/pyproject.toml

Lines 80 to 84 in c4c857e

virtualenv = [
{ version = "^20.4.3,!=20.4.5,!=20.4.6" },
# see https://github.com/python-poetry/poetry/pull/6950 for details
{ version = "<20.16.6", markers = "sys_platform == 'win32' and python_version == '3.9'" },
]

Further, see the opencv example in #7257

@dimbleby
Copy link
Contributor

ah yes, that's the other thing. poetry could forbid overlapping markers at the top level, but it's allowed for dependencies to declare sub-dependencies with overlapping markers anyway. So forbidding top-level overlaps doesn't achieve very much, right?

@ralbertazzi
Copy link
Contributor Author

While agree that a solution to better handle overlapping markers is to be implemented since that's commonly found in community packages, I would still find beneficial not to encourage writing others. Overlapping markers sound just a bad practice to me.

That said, it's 2 core contributors vs me 😄 so I won't fight to hard if you are opinionated on this, and close the PR.

@radoering
Copy link
Member

radoering commented May 20, 2023

I agree that it's often bad practice but I don't think it's always bad practice, e.g. in the example with a general constraint and an additional restriction depending on markers it avoids repetition (and people failing in their attempt to build the inverse marker).

A check that should not be controversial is: If intersection of markers is not empty, intersection of version must not be empty. Afaik, we are already doing this during dependency resolution. Not sure if there is any value to do it earlier.

@ralbertazzi
Copy link
Contributor Author

Sounds like it's best to just close this then ;)

@ralbertazzi ralbertazzi deleted the fix/check-for-mutually-exclusive-markers branch May 22, 2023 12:55
Copy link

github-actions bot commented Mar 3, 2024

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 Mar 3, 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.

Surprising behaviour (and missing warning?) with overlaping python versions
3 participants