bpo-35976: Enable Windows projects to build with platform ARM32 by paulmon · Pull Request #11825 · python/cpython

paulmon · 2019-02-12T04:06:35Z

These are the mechanical changes to Windows build files seperated from code changes.
The solution still builds.
Building of ctypes is disabled because the build will need to be refactored to work with libffi changes that use libffi sources from cpython-source-deps.

https://bugs.python.org/issue35976

zooba

A few minor tweaks. Importantly, it doesn't matter whether the ARM32 build succeeds with this PR, but only that the x86/x64 builds don't start to fail.

zooba · 2019-02-12T16:38:58Z

PCbuild/get_externals.bat

 set libraries=
 set libraries=%libraries%                                       bzip2-1.0.6
-if NOT "%IncludeSSLSrc%"=="false" set libraries=%libraries%     openssl-1.1.0j
+if NOT "%IncludeSSLSrc%"=="false" set libraries=%libraries%     openssl-1.1.1a


This change should be in the OpenSSL PR.

zooba · 2019-02-12T16:39:17Z

PCbuild/openssl.props

  </ItemDefinitionGroup>
  <PropertyGroup>
    <_DLLSuffix>-1_1</_DLLSuffix>
-    <_DLLSuffix Condition="$(Platform) == 'x64'">$(_DLLSuffix)-x64</_DLLSuffix>


This deletion should be in the OpenSSL PR.

zooba · 2019-02-12T16:40:13Z

PCbuild/pyproject.props

      <LinkTimeCodeGeneration Condition="$(Configuration) == 'Release'">UseLinkTimeCodeGeneration</LinkTimeCodeGeneration>
      <LinkTimeCodeGeneration Condition="$(SupportPGO) and $(Configuration) == 'PGInstrument'">PGInstrument</LinkTimeCodeGeneration>
      <LinkTimeCodeGeneration Condition="$(SupportPGO) and $(Configuration) == 'PGUpdate'">PGUpdate</LinkTimeCodeGeneration>
+      <TargetMachine Condition="'$(Platform)'=='ARM'">MachineARM</TargetMachine>


Move this up alongside the other TargetMachine conditions

zooba · 2019-02-12T16:40:34Z

PCbuild/pyproject.props

      <LinkTimeCodeGeneration Condition="$(SupportPGO) and $(Configuration) == 'PGInstrument'">PGInstrument</LinkTimeCodeGeneration>
      <LinkTimeCodeGeneration Condition="$(SupportPGO) and $(Configuration) == 'PGUpdate'">PGUpdate</LinkTimeCodeGeneration>
+      <TargetMachine Condition="'$(Platform)'=='ARM'">MachineARM</TargetMachine>
+      <AdditionalDependencies Condition="'$(Platform)'=='ARM'">version.lib;shlwapi.lib;ws2_32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies)</AdditionalDependencies>


I'm not convinced you need these. I see nothing in here that isn't a default.

There might be a different way to fix this, but the defaults aren't there for ARM. As far as I can see from the command line output it doesn't link to any external dependencies without this.

Here's the problem: Microsoft.Cpp.CoreWin.props only includes kernel32 and user32 for ARM because MinimalCoreWindows is true.

These files are in C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\VC\VCTargets\ on my machine

There is no way to override this because Toolset.props sets MinimalCoreWindows to true if it's empty. If it's not empty and not true in Microsoft.Cpp.CoreWin.props then you get no libs at all. If Microsoft.Cpp.CoreWin.props was changed so the second check was '$(MinimalCoreWin)'!='true' then we could override MinimalCoreWin with false.

Toolset.props include Microsoft.Cpp.Common.props which includes Microsoft.Cpp.Corewin.props and there's no way to change MinimalCoreWin in between the check in Toolset and Microsoft.Cpp.Corewinprops that I can see.

The simplest fix would be to remove version.lib,kernel32.lib,and user32.lib from this list so that they aren't repeated

Ah I see, in that case, leave them there and make them unconditional (provided nothing breaks). They get used on all platforms, and I'd rather not have it look like we have different sets.

(That said, I'm also keen to find ways to reduce this list, as that will mean we're trying to do less random stuff in the core. But that's a long term project.)

zooba · 2019-02-12T16:40:55Z

PCbuild/python.props

-    <opensslDir>$(ExternalsDir)openssl-1.1.0j\</opensslDir>
-    <opensslOutDir>$(ExternalsDir)openssl-bin-1.1.0j\$(ArchName)\</opensslOutDir>
+    <opensslDir>$(ExternalsDir)openssl-1.1.1a\</opensslDir>
+    <opensslOutDir>$(ExternalsDir)openssl-bin-1.1.1a\$(ArchName)\</opensslOutDir>


Save for the OpenSSL PR

zooba · 2019-02-12T16:43:03Z

PCbuild/python.props


    <!-- The version and platform tag to include in .pyd filenames -->
    <PydTag Condition="$(ArchName) == 'win32'">.cp$(MajorVersionNumber)$(MinorVersionNumber)-win32</PydTag>
+    <PydTag Condition="$(ArchName) == 'arm32'">.cp$(MajorVersionNumber)$(MinorVersionNumber)-win_arm</PydTag>


Looking ahead to the future, do you think "ARM == ARM32" is going to hold for the long term? Or should we explicitly put win_arm32 now? I'd prefer to be more explicit, unless there's a lot of precedent for leaving the bitness out here.

And I mean from a consumer point of view. Clearly Microsoft has decided that $(Platform)=='ARM' already, but that doesn't directly follow that we have to do the same.

I'm glad you brought this up. There was some win_arm code already in cpython, but it looks like it was there to support Windows CE. That might be another reason to use win_arm32 here, to completely disambiguate it from both arm64 and arm on Windows CE.

specifically the arm32 implementation currently relies on the following snippet in pc/pyconfig.h
It looks like support for Windows CE has been removed but there's a few comments still lingering. If Windows CE is no longer supported then the PYD_PLATFORM_TAG will also need to be edited to match. There's a doc snippet in Doc/whatsnew/3.5.rst that says the platform tag for Windows on ARM is win_arm as well.

#elif defined(_M_ARM)
#define COMPILER _Py_PASTE_VERSION("32 bit (ARM)")
#define PYD_PLATFORM_TAG "win_arm"
#else

@pfmoore @zware Any thoughts on this? I'm +0 on using win_arm32 rather than win_arm, but is there any chance people are relying on this branch in the code? I doubt it, so I think just changing it here is going to be fine, but I'd like another opinion.

paulmon · 2019-02-12T18:33:07Z

Comments should be addressed, except for the AdditionalDependencies one, because I haven't found a different way to get it to pull in any dependencies as there appears to be no defaults for ARM. Verified that it builds on x86 and x64 on my dev machine.

edit: see comment above about AdditionalDependencies. I think this is ready to go unless you still have concerns.

zooba · 2019-02-13T21:35:06Z

I think this is ready to merge.

@zware - as the person who touches the build files second-most, any thoughts?

zooba · 2019-02-13T21:36:47Z

Oh, except for the arm vs. arm32 question.

zware

LGTM.

Idle observation: it really bothers me that we have to churn every .vcxproj file to add each ProjectConfiguration; is there no way that we could factor that out?

zware · 2019-02-13T22:25:03Z

About arm vs arm32: AFAIK, there has never been an officially supported release of Python running on Windows on ARM, so I'm not concerned about any kind of backward compatibility. I tend to prefer the more explicit arm32.

zooba · 2019-02-13T23:38:35Z

Idle observation: it really bothers me that we have to churn every .vcxproj file to add each ProjectConfiguration; is there no way that we could factor that out?

Afraid not. Last time I tried both MSBuild and Visual Studio failed to handle the files properly without the items directly in there (a fairly subtle difference between parsing the unevaluated project file and the evaluated one).

@paulmon - let's update that string to arm32 wherever it occurs and then I'll merge

paulmon · 2019-02-14T00:01:28Z

I searched for 'win-arm' 'win_arm' and 'arm' and only found one other place that looks like it needs changed to arm32.

zooba · 2019-02-14T16:30:31Z

As long as we fix any others before making a stable release, it'll be fine to catch them later. But I suspect you're right and it's mostly inferred from the two places you've updated.

arm32 changes to pcbuild

918dc9e

paulmon requested review from a team, rhettinger and skrah as code owners February 12, 2019 04:06

the-knights-who-say-ni added the CLA signed label Feb 12, 2019

bedevere-bot added the awaiting review label Feb 12, 2019

add blurb for PCBuild file changes

097b0ca

zooba reviewed Feb 12, 2019

View reviewed changes

paulmon changed the title ~~bpo-35976: PCBuild changes for arm32 be separated from code changes for review~~ bpo-35976: Enable Windows projects to build with platform ARM32 Feb 12, 2019

builds on x86 and x64 work

7da2767

Paul Monson added 4 commits February 12, 2019 12:33

clean up AdditionalDependencies for ARM

5a41f42

not needed

05b3bef

remove armasm.props

bd42ab6

make AdditionalDependencies as small as possible

3dfb18c

zware approved these changes Feb 13, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 13, 2019

use win_arm32 for tag suffix

6d58613

zooba merged commit 8a1657b into python:master Feb 14, 2019

bedevere-bot removed the awaiting merge label Feb 14, 2019

paulmon deleted the arm32-pcbuild branch April 1, 2019 18:29

Uh oh!

Conversation

paulmon commented Feb 12, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmon commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Feb 13, 2019

Uh oh!

zooba commented Feb 13, 2019

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

zware commented Feb 13, 2019

Uh oh!

zooba commented Feb 13, 2019

Uh oh!

paulmon commented Feb 14, 2019

Uh oh!

zooba commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

paulmon commented Feb 12, 2019 •

edited by bedevere-bot

Loading

paulmon commented Feb 12, 2019 •

edited

Loading