GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix build error for python 3.11 on win/arm64 target by niyas-sait · Pull Request #28491 · python/cpython

Conversation

niyas-sait commented Sep 21, 2021

Copy link

Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix build error for python 3.11 on win/arm64 target

41581b2

frozen_modules was set to compile for amd64 target for win/arm64
configuration. Project files updated to use correct target

niyas-sait requested a review from a team as a code owner

September 21, 2021 15:20

the-knights-who-say-ni added the CLA not signed label

Sep 21, 2021

bedevere-bot added the awaiting review label

Sep 21, 2021

the-knights-who-say-ni commented Sep 21, 2021

Copy link

Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba requested changes

Sep 21, 2021

View reviewed changes

Comment thread

PCbuild/pcbuild.proj Outdated

		<Platform>$(Platform)</Platform>
		<Platform Condition="$(Platform) == 'ARM'">Win32</Platform>
		<Platform Condition="$(Platform) == 'ARM64'">x64</Platform>
		<Platform Condition="$(Platform) == 'ARM64'">ARM64</Platform>

zooba Sep 21, 2021

Copy link

Copy Markdown

Member

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 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

PCbuild/pcbuild.sln Outdated

		{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

zooba Sep 21, 2021

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/scripts/freeze_modules.py Outdated

		else:
		def find_tool():
		for arch in ['amd64', 'win32']:
		for arch in ['amd64', 'win32', 'arm64']:

zooba Sep 21, 2021

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting review labels

Sep 21, 2021

bedevere-bot commented Sep 21, 2021

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

niyas-sait commented Sep 23, 2021

Copy link

Copy Markdown

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          pcbuild.sln: Revert target for _freeze_module to win32/x64

6e311e1

zooba commented Sep 23, 2021

Copy link

Copy Markdown

Member

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

niyas-sait commented Sep 24, 2021

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Use PreferredToolArchitecture property to build _freezemodule

4df75b6

the-knights-who-say-ni added CLA signed and removed CLA not signed labels

Sep 24, 2021

niyas-sait commented Sep 24, 2021

Copy link

Copy Markdown

Contributor Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting change review and removed awaiting changes labels

Sep 24, 2021

bedevere-bot commented Sep 24, 2021

Copy link

Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot requested a review from zooba

September 24, 2021 14:44

zooba added skip issue skip news labels

Sep 27, 2021


          Merge branch 'main' into fix_python_311_build_win_arm64

09aa043

zooba merged commit adc5d32 into python:main

Sep 27, 2021

bedevere-bot removed the awaiting change review label

Sep 27, 2021

chris-eibl mentioned this pull request

Oct 9, 2026

gh-158948: Generate Modules/getbuildinfo.h #158951

Open

Reviewers

zooba

Awaiting requested review from zooba

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.