-
Notifications
You must be signed in to change notification settings - Fork 68
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
Use arm64 MSVC on arm64 Windows #285
Conversation
An alternative approach would be to conditionally construct The host platform should not be changing due to settings or anything else at runtime (the target platform theoretically should be able to be configured at runtime, though I'm not sure if it can). |
It was really bothering me that we had two copies of essentially the same mapping, so I spent some time thinking about what is it that these two mappings represent and distilled from that a function that will construct the spec. This new function |
diffcov tests are now failing for the changes in Let me know if you have any lingering concerns; otherwise, it's ready to go. |
Confirmed the behavior locally, LGTM |
It's a bigger compatibility change from what's currently there, and it seems possible (I won't say likely) that someone could be depending on that dict being there, either to read it or to update it. Your call, of course, but if you're going to break compatibility then why not just implement it better in setuptools? (Otherwise you'll never get rid of distutils ;) ) |
My compatibility strategy for distutils is to first deprecate all such interactions with distutils (such that only setuptools is interacting with distutils), making distutils an implementation detail that Setuptools can then absorb (accept and run the tests) and converge. The deprecation is in place, so there shouldn't be any users relying on this variable. I know there's a possibility users are reliant on it and just haven't admitted such, but I'm willing to take that risk and provide a compatibility layer or back it out if necessary.
I see much of what Setuptools provides over distutils as smallish extensions of the core distutils functionality. I'm expecting that many (most?) of the interfaces in distutils will become the canonical implementation for Setuptools. I've been consolidating some behaviors and directing contributions here when relevant, rather than expanding the customizations in Setuptools. |
distutils/_msvccompiler.py
Outdated
_vcvars_names = { | ||
'win32': 'x86', | ||
'win-amd64': 'amd64', | ||
'win-arm32': 'arm', | ||
'win-arm64': 'arm64', | ||
} | ||
|
||
|
||
def _get_vcvars_spec(host_platform, platform): | ||
""" | ||
Given a host platform and platform, determine the spec for vcvarsall. | ||
|
||
Uses the native MSVC host if the host platform would need expensive | ||
emulation for x86. | ||
|
||
>>> _get_vcvars_spec('win-arm64', 'win32') | ||
'arm64_x86' | ||
>>> _get_vcvars_spec('win-arm64', 'win-amd64') | ||
'arm64_amd64' | ||
|
||
Always cross-compile from x86 to work with the lighter-weight MSVC | ||
installs that do not include native 64-bit tools. | ||
|
||
>>> _get_vcvars_spec('win32', 'win32') | ||
'x86' | ||
>>> _get_vcvars_spec('win-arm32', 'win-arm32') | ||
'x86_arm' | ||
>>> _get_vcvars_spec('win-amd64', 'win-arm64') | ||
'x86_arm64' | ||
""" | ||
if host_platform != 'win-arm64': | ||
host_platform = 'win32' | ||
vc_hp = _vcvars_names[host_platform] | ||
vc_plat = _vcvars_names[platform] | ||
return vc_hp if vc_hp == vc_plat else f'{vc_hp}_{vc_plat}' |
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.
@jaraco looks like this specific change may have broken cross-compilation (at least in some contexts). See investigation done in pypa/setuptools#4648 (comment)
) | ||
|
||
# Get the vcvarsall.bat spec for the requested platform. | ||
plat_spec = PLAT_TO_VCVARS[plat_name] | ||
plat_spec = _get_vcvars_spec(get_host_platform(), get_platform()) |
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.
Seems the bug is in line 276 where it's ignoring the plat_name
parameter and instead calling get_platform()
again. 👀
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.
I think you're right, the following is enough of a change:
plat_spec = _get_vcvars_spec(get_host_platform(), get_platform()) | |
plat_spec = _get_vcvars_spec(get_host_platform(), plat_name) |
Related to pypa/setuptools#4555 but with no dependency.