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

Combine sys_platform and platform_system in marker algebra #9344

Open
wants to merge 1 commit into
base: charlie/lower-ii
Choose a base branch
from

Conversation

charliermarsh
Copy link
Member

Summary

This PR adds a representation to the marker algebra to capture "known pairs" of sys_platform and platform_system values.

For example, we want to be able to detect that sys_platform == 'win32' and platform_system == 'Darwin' are disjoint.

There is some risk here, in that if there are platforms for which, e.g., sys_platform == 'win32 but platform_system is not Windows, we'll get things wrong. I'm trying to weigh whether that's a real risk.

In theory, we could maybe also include os.name here?

Closes #7760.
Closes #9275.

@charliermarsh charliermarsh changed the base branch from main to charlie/lower-ii November 22, 2024 02:28
@charliermarsh charliermarsh added enhancement New feature or improvement to existing functionality performance Potential performance improvement labels Nov 22, 2024
@charliermarsh charliermarsh force-pushed the charlie/lower-iii branch 4 times, most recently from cfc5550 to 1c03cb9 Compare November 22, 2024 02:34
@Vinno97
Copy link

Vinno97 commented Nov 22, 2024

In theory, we could maybe also include os.name here?

Yes. I was just about to comment on #9275 about this.

For example, jupyter-server has a dependency on pywinpty for os_name == 'nt'. If want linux-only lock-file, a full dependency string would then have to be: platform_system == 'Linux' and sys_platform == 'linux' and os_name == 'posix'

@charliermarsh
Copy link
Member Author

Yeah. I think it's a bit more challenging because there isn't a 1-to-1 mapping between os.name and sys.platform for Linux and macOS (they both use posix).

@charliermarsh
Copy link
Member Author

(Not impossible but different than what I've encoded here.)

@Vinno97
Copy link

Vinno97 commented Nov 22, 2024

Since os_name == 'posix' could also be true for BSD and other more esoteric POSIX operating systems, perhaps os_name == ' posix' should entail os_name != 'nt', with its subsequent platform_system and sys_platform restrictions?

@charliermarsh
Copy link
Member Author

On reflection, what I have here is not correct. It fails this test (i.e., it thinks these are disjoint):

assert!(!is_disjoint(
    "sys_platform == 'cygwin'",
    "platform_system == 'Java'"
));

@charliermarsh
Copy link
Member Author

Somewhat losing confidence here because it turns out Android also returns "Linux" for platform_system? Despite sys_platform returning "android".

@Vinno97
Copy link

Vinno97 commented Nov 25, 2024

It seems like Poetry has gone though quite the rabbit hole for this themselves: python-poetry/poetry#5192. But even after quite some pull requests, they closed the isue with this comment:

Closing since there haven't been much improvements/fixes anymore in the last time and I can't come up with a good solution for merging sys_platform and platform_system. - python-poetry/poetry#5192 (comment)

It appears this may be an unresolvable issue, though perhaps it's feasible to just cull known impossible environments? I.e. we know that platform_system=='Linux' will never occur together with sys_platform=='win32' or sys_platform=='darwin'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality performance Potential performance improvement
Projects
None yet
2 participants