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

Use arm64 MSVC on arm64 Windows #285

Merged
merged 12 commits into from
Aug 27, 2024
Merged

Use arm64 MSVC on arm64 Windows #285

merged 12 commits into from
Aug 27, 2024

Conversation

saschanaz
Copy link
Contributor

Related to pypa/setuptools#4555 but with no dependency.

@zooba
Copy link
Contributor

zooba commented Aug 26, 2024

An alternative approach would be to conditionally construct PLAT_TO_VCVARS based on get_host_platform (which would then let you cross-compile to X86 or AMD64 using the native ARM64 compilers on ARM64).

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).

distutils/_msvccompiler.py Outdated Show resolved Hide resolved
distutils/_msvccompiler.py Outdated Show resolved Hide resolved
distutils/_msvccompiler.py Outdated Show resolved Hide resolved
@jaraco
Copy link
Member

jaraco commented Aug 26, 2024

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 _get_vcvars_spec encodes the logic that was previously in comments only (that win32 supersedes everything but win-arm64). It additionally adds some doctests that perform some checks on the expected values. This logic should be more durable and maintainable than lookup tables.

@jaraco
Copy link
Member

jaraco commented Aug 26, 2024

diffcov tests are now failing for the changes in initialize, which is due to the fact that the coverage tests run on Linux and this project doesn't have coverage aggregation, so I'll ignore it for now.

Let me know if you have any lingering concerns; otherwise, it's ready to go.

@saschanaz
Copy link
Contributor Author

Confirmed the behavior locally, LGTM

@zooba
Copy link
Contributor

zooba commented Aug 26, 2024

This logic should be more durable and maintainable than lookup tables.

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 ;) )

@jaraco
Copy link
Member

jaraco commented Aug 27, 2024

This logic should be more durable and maintainable than lookup tables.

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.

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.

Why not just implement it better in setuptools? (Otherwise you'll never get rid of distutils ;) )

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.

@jaraco jaraco merged commit b8c70bb into pypa:main Aug 27, 2024
19 of 21 checks passed
@saschanaz saschanaz deleted the arm64 branch August 27, 2024 08:28
Comment on lines 181 to 215
_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}'
Copy link
Contributor

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())
Copy link
Contributor Author

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. 👀

Copy link
Contributor

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:

Suggested change
plat_spec = _get_vcvars_spec(get_host_platform(), get_platform())
plat_spec = _get_vcvars_spec(get_host_platform(), plat_name)

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.

4 participants