-
Notifications
You must be signed in to change notification settings - Fork 770
Use sysconfig.get_platform for machine detection #1835
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
base: master
Are you sure you want to change the base?
Conversation
This should function correctly when cross compiling unlike platform.machine.
a7debfd to
7326757
Compare
Review of PR #1835Thanks for the contribution! Your change to use The Root CauseThe problem has two parts:
Your PR fixes part 1, but not part 2. Why the Current PR is IncompleteAfter your change, the code path is: machine = sysconfig.get_platform().lower().split("-")[-1] # Now correctly: "aarch64"
# ...
else:
# Building from source locally: use -march=native for maximum performance
return base_args + ["-march=native"] # ❌ Still fails!In buildroot's cross-compilation:
The error happens because Complete Fix NeededThe fix needs to also detect cross-compilation and avoid import platform
import sysconfig
def is_cross_compiling():
"""Detect if we're cross-compiling (host != target)."""
host_machine = platform.machine().lower()
target_platform = sysconfig.get_platform().lower()
target_machine = target_platform.split("-")[-1]
# Normalize architecture names
host_normalized = _normalize_arch(host_machine)
target_normalized = _normalize_arch(target_machine)
return host_normalized != target_normalized
def _normalize_arch(arch):
"""Normalize architecture names for comparison."""
if arch in ("x86_64", "amd64", "x64"):
return "x86_64"
elif arch in ("aarch64", "arm64"):
return "aarch64"
return archThen in elif is_cross_compiling():
# Cross-compiling: -march=native is invalid, use safe baseline for target
arch_flag = _get_safe_march_flag(machine)
if arch_flag:
return base_args + [arch_flag]
else:
return base_args
else:
# Native build: -march=native is safe
return base_args + ["-march=native"]CI Failure NoteThe type check ( Would be cool if you could add that as well, because then the PR should run fully green! Would you like to update your PR with the complete cross-compilation detection, or would you prefer I handle this in a separate PR? |
|
There's a rather subtle bug in this code that I've seen in cargo as well, you can't actually reliably detect cross compilation with this approach at all. It fails to cover scenarios like cross compiling on a x86_64 host for a x86_64 target(say with a different userspace and/or cpu variant) which can result in a number of problems: Best option IMO is to avoid def is_cross_compiling():
"""Detect if we're cross-compiling (host != target)."""
host_machine = platform.machine().lower()
target_platform = sysconfig.get_platform().lower()
target_machine = target_platform.split("-")[-1]
# Normalize architecture names
host_normalized = _normalize_arch(host_machine)
target_normalized = _normalize_arch(target_machine)
return host_normalized != target_normalized
Yeah, I was testing with AUTOBAHN_ARCH_TARGET=safe in the env to avoid |
|
Technically buildroot can pass explicit optimization flags kinda like this as well via the env to a python package, this should be fairly reliable but adds a little bit of complexity on the packaging side of things. |
|
so in summary: Complete Fix NeededThe fix should change the default from else:
# Building from source: use safe baseline by default
# This ensures cross-compilation works (buildroot, etc.)
# Users who want -march=native can set AUTOBAHN_ARCH_TARGET=native
arch_flag = _get_safe_march_flag(machine)
if arch_flag:
return base_args + [arch_flag]
else:
return base_argsThis way:
Summary of Required Changes
CI Failure NoteThe type check ( |
Do you prefer opt-in like this for |
|
Anyways I think the default is probably a separate issue than the machine detection issue here to some degree so maybe leave that for a follow-up PR? |
good question;) @mgorny : any opinions? I'd guess Gentoo would want "native" - even though practical difference vs safe options is likely small (if measurable at all). but Gentoo isn't doing cross compile. Buildroot and Yocto do. personally, I mainly care about our own published native binary wheels for Debian ARM64, and for PyPy. and we are using Qemu with PyPA Docker images, not cross-compile, to build those wheels. |
This should function correctly when cross compiling unlike platform.machine.
Description
Uses sysconfig to detect target architecture correctly.
Related Issue(s)
Closes or relates to #1834
Checklist
the style guidelines of this project
is effective or that my feature works
updated the changelog
in this PR