-
Notifications
You must be signed in to change notification settings - Fork 229
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,12 @@ | |
import functools | ||
import platform as platform_module | ||
import re | ||
import sys | ||
from collections.abc import Set | ||
from enum import Enum | ||
|
||
from ._compat.typing import Final, Literal, assert_never | ||
from .typing import PlatformName | ||
from .util import HOST_PLATFORM | ||
|
||
PRETTY_NAMES: Final[dict[PlatformName, str]] = { | ||
"linux": "Linux", | ||
|
@@ -74,20 +74,14 @@ def parse_config(config: str, platform: PlatformName) -> set[Architecture]: | |
def auto_archs(platform: PlatformName) -> set[Architecture]: | ||
native_machine = platform_module.machine() | ||
|
||
# Cross-platform support. Used for --print-build-identifiers or docker builds. | ||
host_platform: PlatformName = ( | ||
"windows" | ||
if sys.platform.startswith("win") | ||
else ("macos" if sys.platform.startswith("darwin") else "linux") | ||
) | ||
Comment on lines
-78
to
-82
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
assert HOST_PLATFORM | ||
native_architecture = Architecture(native_machine) | ||
|
||
# we might need to rename the native arch to the machine we're running | ||
# on, as the same arch can have different names on different platforms | ||
if host_platform != platform: | ||
if platform != HOST_PLATFORM: | ||
for arch_synonym in ARCH_SYNONYMS: | ||
if native_machine == arch_synonym.get(host_platform): | ||
if native_machine == arch_synonym.get(HOST_PLATFORM): | ||
synonym = arch_synonym[platform] | ||
|
||
if synonym is None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,7 +93,24 @@ def build_frontend_or_default( | |
os.environ.get("CIBW_CACHE_PATH", DEFAULT_CIBW_CACHE_PATH) | ||
).resolve() | ||
|
||
IS_WIN: Final[bool] = sys.platform.startswith("win") | ||
|
||
def get_host_platform() -> PlatformName | None: | ||
# Cross-platform support. Used for --print-build-identifiers or docker builds. | ||
if sys.platform.startswith("linux"): | ||
return "linux" | ||
elif sys.platform.startswith("darwin"): | ||
return "macos" | ||
elif sys.platform.startswith("win"): | ||
return "windows" | ||
else: | ||
return None | ||
|
||
|
||
HOST_PLATFORM: Final[PlatformName | None] = get_host_platform() | ||
|
||
HOST_IS_WINDOWS: Final[bool] = get_host_platform() == "windows" | ||
|
||
HOST_IS_WIN32: Final[bool] = sys.platform == "win32" | ||
Comment on lines
+111
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
|
||
@typing.overload | ||
|
@@ -135,7 +152,7 @@ def call( | |
if capture_stdout: | ||
kwargs["universal_newlines"] = True | ||
kwargs["stdout"] = subprocess.PIPE | ||
result = subprocess.run(args_, check=True, shell=IS_WIN, env=env, cwd=cwd, **kwargs) | ||
result = subprocess.run(args_, check=True, shell=HOST_IS_WINDOWS, env=env, cwd=cwd, **kwargs) | ||
if not capture_stdout: | ||
return None | ||
return typing.cast(str, result.stdout) | ||
|
@@ -585,7 +602,7 @@ def virtualenv( | |
# version of pip that will end-up installed. | ||
# c.f. https://virtualenv.pypa.io/en/latest/cli_interface.html#section-seeder | ||
if ( | ||
not IS_WIN | ||
not HOST_IS_WINDOWS | ||
and constraints["pip"] != "embed" | ||
and Version(constraints["pip"]) >= Version("19.3") | ||
): | ||
|
@@ -602,7 +619,7 @@ def virtualenv( | |
python, | ||
venv_path, | ||
) | ||
if IS_WIN: | ||
if HOST_IS_WINDOWS: | ||
paths = [str(venv_path), str(venv_path / "Scripts")] | ||
else: | ||
paths = [str(venv_path / "bin")] | ||
|
There was a problem hiding this comment.
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 tostartswith("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 usestartswith("win")
.