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

chore: Simplify host platform logic #1498

Closed
wants to merge 3 commits into from
Closed

Conversation

hoodmane
Copy link
Contributor

There is some duplicated code for deciding what the host platform is. This groups it together in util.py.

I was slightly confused in some places whether startswith("win") or == "win32" was desired. I added a new HOST_IS_WIN32 variable since I think it's as explicit as possible that the intended check is specifically for win32 and not for windows in general. Since HOST_IS_WINDOWS is for that.

There is some duplicated code for deciding what the host platform
is. This groups it together in util.py
return "linux"
elif sys.platform == "darwin":
return "macos"
elif sys.platform == "win32":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to say sys.platform == "win32" but this changes it to startswith("win"). Am I introducing a bug with this behavior change? If so I'd like to add a comment explaining why we use == "win32" here but in architecture we use startswith("win").

Comment on lines -78 to -82
host_platform: PlatformName = (
"windows"
if sys.platform.startswith("win")
else ("macos" if sys.platform.startswith("darwin") else "linux")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this PR because it was bothering me that the logic here does not exactly match the logic in _compute_platform_ci().

Comment on lines +111 to +113
HOST_IS_WINDOWS: Final[bool] = get_host_platform() == "windows"

HOST_IS_WIN32: Final[bool] = sys.platform == "win32"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a bit excessive but I think it makes the intended distinction between these two checks 100% clear.

@hoodmane
Copy link
Contributor Author

Well this seems to pass the tests. Maybe there should be more unit test coverage for the difference between sys.platform == win32 and sys.platform.startswith('win')...

@joerick
Copy link
Contributor

joerick commented May 15, 2023

This is one of those cases where backward compatibility was preferred over correctness - the 'win32' value applies even on 64-bit windows. See here:

https://github.com/python/cpython/blob/b15a1a6ac6ea0d7792036e639e90f0e51400c2ee/PC/pyconfig.h#L321-L333

(Found via this SO answer)

So there's no functional difference between sys.platform.startswith("win") and sys.platform == 'win32'.

@henryiii
Copy link
Contributor

henryiii commented May 15, 2023

It's a recommended practice to always use sys.platform.startswith - for some platforms it matters, and it should be possible to "extend" this without breaking checks. MyPy, etc. support this. So my recommendation is never to check via ==. The docs recommend that as an idiom too: https://docs.python.org/3/library/sys.html#sys.platform

I personally don't like pulling this check out - I'd rather it just always be sys.platform.startswith("win")1. It used to be better to do that statically, though these days you can use Final and it should get assigned a Literal boolean value (though, there's a chance some type checkers will complain about that, some don't seem to like Final deduction!). But I like the explicit way to do it - no need to look up some magic constant when you can specify exactly what you mean in the same number of lines.

Footnotes

  1. I prefer avoiding the 32 since it confuses the reader into thinking it has something to do with 32-bit Windows.

@joerick
Copy link
Contributor

joerick commented May 15, 2023

Agreed on the literal sys.platform.startswith("win") over a constant - it's clear and means I don't have to look up a constant definition if i'm in doubt. I'm okay with the startswith() convention too - I guess if we had used that everywhere there wouldn't be the 32/64 confusion in the first place!

@hoodmane
Copy link
Contributor Author

Okay will close this and do another which switches to using startswith("win") throughout since I think that is the consensus here.

@hoodmane hoodmane closed this May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants