Skip to content

Fix build error for python 3.11 on win/arm64 target #28491

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

Merged
merged 4 commits into from
Sep 27, 2021

Conversation

niyas-sait
Copy link
Contributor

Python 3.11 build was failing on win/arm64 target due to one of the component (_frozen_module) set to target amd64 instead of arm64 target.

frozen_modules was set to compile for amd64 target for win/arm64
configuration. Project files updated to use correct target
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@nsait-linaro

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@@ -17,7 +17,7 @@
<FreezeProjects>
<Platform>$(Platform)</Platform>
<Platform Condition="$(Platform) == 'ARM'">Win32</Platform>
<Platform Condition="$(Platform) == 'ARM64'">x64</Platform>
<Platform Condition="$(Platform) == 'ARM64'">ARM64</Platform>
Copy link
Member

Choose a reason for hiding this comment

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

This platform needs to match the host machine (doing the build), not the target architecture, because these projects are run as part of build.

Maybe there's another property that can be used to check the host platform?

{19C0C13F-47CA-4432-AFF3-799A296A4DDC}.Debug|ARM64.ActiveCfg = Debug|x64
{19C0C13F-47CA-4432-AFF3-799A296A4DDC}.Debug|ARM64.Build.0 = Debug|x64
{19C0C13F-47CA-4432-AFF3-799A296A4DDC}.Debug|ARM64.ActiveCfg = Debug|ARM64
{19C0C13F-47CA-4432-AFF3-799A296A4DDC}.Debug|ARM64.Build.0 = Debug|ARM64
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. Until the only people building for ARM64 are running on ARM64 (which seems a long way off), these projects need to match the host machine.

@@ -34,7 +34,7 @@
sys.exit("ERROR: missing _freeze_module")
else:
def find_tool():
for arch in ['amd64', 'win32']:
for arch in ['amd64', 'win32', 'arm64']:
Copy link
Member

Choose a reason for hiding this comment

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

This change is fine, but should probably also check whether it's running in an ARM64 build of Python first. If this is an x86/x64 build, there's no point trying to launch the ARM64 binary.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@niyas-sait
Copy link
Contributor Author

Thanks, @zooba. I use native arm64 compilation of cpython a lot in my workflow so I am keen to have the native compilation supported as well.

Would it be acceptable if a new host platform property is added to the project file and keep x64 or x86 as default (depends on target) but allow users to configure a different host platform to allow native arm64 compilation?

@zooba
Copy link
Member

zooba commented Sep 23, 2021

Would it be acceptable if a new host platform property is added to the project file and keep x64 or x86 as default (depends on target) but allow users to configure a different host platform to allow native arm64 compilation?

Yes, but see if there's anything that already exists that it set automatically. Browsing through properties that I can see, PreferredToolArchitecture or VCToolArchitecture (or at worst, ProcessorArchitecture) might do it.

@niyas-sait
Copy link
Contributor Author

Would it be acceptable if a new host platform property is added to the project file and keep x64 or x86 as default (depends on target) but allow users to configure a different host platform to allow native arm64 compilation?

Yes, but see if there's anything that already exists that it set automatically. Browsing through properties that I can see, PreferredToolArchitecture or VCToolArchitecture (or at worst, ProcessorArchitecture) might do it.

Unfortunately, I cannot find any of the properties listed above set automatically for MSBuild. I've checked the documentaiton and I don't see any of them listed for use. I am fairly new to VS and MSBuild so I might be missing something obivious.

I managed to use another property called PROCESSOR_ARCHITECTURE which seems to work. I will prepare a patch with that. I am going to revert the changes PCBuild.sln as I am not sure how to use the properties there correctly. If MSBuild works then that is good enough for the time being

@niyas-sait
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@zooba: please review the changes made to this pull request.

@zooba zooba merged commit adc5d32 into python:main Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants