bpo-35920: Windows 10 ARM32 platform support #11774

paulmon · 2019-02-06T19:40:01Z

At a minimum this code needs to be refactored to take into account removing libffi_msvc and moving to the current libffi implementation based on #3806.

https://bugs.python.org/issue35920

the-knights-who-say-ni · 2019-02-06T19:40:05Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

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

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

Modules/_decimal/libmpdec/bits.h

PCbuild/_decimal.vcxproj

zooba · 2019-02-06T21:37:55Z

Lib/distutils/util.py

@@ -92,6 +94,23 @@ def get_platform ():

 # get_platform ()

+def get_target_platform ():


If this needs to replace most uses of get_platform except when detecting whether we are cross-compiling, can we just update get_platform (and avoid all the renames above) and provide a new API for generating the tag?

On second reading, we probably aren't properly handling the --plat-name option in build_ext properly already, and that would be the better way to specify that "arm" is the target. It can select a default based on the environment variable, but it should also work when passed -p win-arm and going via the vcvarsall.bat path.

get_target_platform returns the target platform when cross-compiling.
get_platform returns the host platform.
ARM32 binaries can only be cross-compiled using the MSVC toolset.
Also when a module like numpy overrides the build files --target-plat doesn't get always passed through in the attributes like it should

According to the docs, it already returns the target platform on macOS, as well as already being wrong for Windows ("non-POSIX platforms … return sys.platform"), so I think changing it on Windows to also return the distutils target is fine.

I'll start a post on python-dev to clarify and see what people think.

I updated the PR to use get_platform and get_host_platform. Testing is still needed

Lib/platform.py

zooba · 2019-02-06T21:38:59Z

Lib/platform.py

+        return True
+    return False
+
+def win32_editionId(editionId=''):


This will need documentation (Doc/library/platform.rst)

Lib/platform.py

PCbuild/_contextvars.vcxproj

PCbuild/_freeze_importlib.vcxproj

PCbuild/pyproject.props

PCbuild/pythoncore.vcxproj

Tools/winiot/sync_win_iot.py

zooba · 2019-02-06T21:59:29Z

To help make the reviews simpler, I'm inclined to say it's okay to merge most of the .vcxproj and .vcxproj.filters changes immediately. Specifically, those that:

just add the platform/configuration values for ARM32
add conditions for ARM32 platform that don't impact Win32/x64 platforms
update the .filters files to match what's actually in there

Obviously the impactful ctypes and OpenSSL changes should not be included yet, and I mentioned a few things that I would like to see reverted, but most of the project changes are mechanical and otherwise unused unless explicitly enabled through build.bat -p ARM32 (which already works as that parameter just passes whatever value through).

@zware @skrah any thoughts?

skrah · 2019-02-06T22:13:47Z

On Wed, Feb 06, 2019 at 01:47:14PM -0800, Paul Monson wrote: > @@ -145,7 +145,7 @@ mpd_bsf(mpd_size_t a) } /* END ASM */ -#elif defined(MASM) +#elif defined(MASM) || defined (_M_ARM) #include <intrin.h> That makes sense I think. Do you consider armasm.exe to be the same as masm.exe?

GitHub is confusing, do you mean my comment makes sense or the diff makes sense? If it's the former, please ignore the following: Libmpdec (of which I am the author) uses -DASM or /DMASM mainly to enable the optimizations in typearith.h. It is only used in this place because intrinsics and ASM should be switched on/off together. Only enabling on this one intrinsic is pointless since it is only moderately time critical and completely eclipsed by the inline functions in typearith.h, which are very much time critical. Moreover, I want to see *which* build is enabled on the buildbots in the command line and for my own testing in libmpdec. So I don't want macros from headers enabling or disabling features. I don't understand the question about armasm.exe, which has nothing to do with the section in the diff. masm.exe is used to compile vcdiv64.asm. Any changes to libmpdec should go to upstream first. I'm pretty sure that the ANSI build is sufficient for ARM devices.

paulmon · 2019-02-06T22:34:40Z

@skrah I was agreeing that your comment made sense.
MSVC uses armasm.exe to build ARM assembler files. There is no masm.exe for ARM32 so checking MASM when building for ARM32 wasn't seem obvious to me, but I understand now.
Thanks for the additional explanation.

skrah · 2019-02-06T23:24:33Z

@zooba I also think anything under PCBuild that is conditional on ARM32 is fine to merge. The files do get a bit in the way when reviewing.

skrah · 2019-02-06T23:31:52Z

@paulmon Yes, I should have used /DMASM_AND_INTRINSICS; I agree that the use of MASM is not really obvious, it really just sounds nicer and shortens the command line.

nascheme · 2019-02-07T00:25:08Z

I didn't look at the mountains of XML-like stuff in PCBuild, looks like gobblygook to me. However, I briefly looked over the rest and it looks relatively sensible to me.

One style note, I generally don't like portability code like this:

@unittest.skipUnless(sys.platform == 'win32', 'these tests require Windows')
SKIP_MESSAGE = (None if sys.platform == 'win32' and not platform.win32_is_iot() else
                "These tests require Windows x86 or x64.  Windows IoT Core and nanoserver are not supported")

@unittest.skipUnless(SKIP_MESSAGE is None, SKIP_MESSAGE)
class BDistMSITestCase(support.TempdirManager,
                       support.LoggingSilencer,
                       unittest.TestCase):

You are testing for specific versions or platform identifiers, etc. It is better to test for "capabilities". I don't know where it would make sense to put the code (perhaps not in platform.py), but I think it would be clearer to define individual boolean flags for these things. So, the unit test code can do:

@unittest.skipUnless(platform_flags.have_win32_msi)
...

Where have_win32_msi is defined (probably not the correct name), you have have a substantial comment string that explains exactly what that flag means.

Maybe that's not too clear. Let me try to explain in another way. Say you have code throughout Python that works differently if the platform supports case-insensitive filenames. Rather than open code the test (e.g. "if plaform-x or platform-y or platform-z"), you define a global variable in one spot (e.g "have_case_insenstive_files = plaform-x or ..."). Then all the different places you need to check that, you just check that global.

nascheme · 2019-02-07T00:30:45Z

If the boolean flags for indicating if the platform supports some feature are only used by tests, it looks like Lib/test/support/__init__py is the place for that. We have skip_unless_symlink(), for example. Most of these ARM32 platform skips could be done that way I think.

paulmon · 2019-02-12T02:45:50Z

I'm working on a seperate PR for the PCBuild directory changes next so that we can separate the mechanical build changes from the code changes.

paulmon · 2019-02-14T18:42:27Z

This PR has been rebased on the merged PCBuild changes

paulmon · 2019-02-14T18:45:51Z

I am currently working on refactoring the changes to ctypes for ARM to match #11797

zooba · 2019-02-14T19:39:56Z

Lib/distutils/sysconfig.py

-    g['EXT_SUFFIX'] = _imp.extension_suffixes()[0]
+    # if cross-compiling replace hardcoded platform-specific EXT_SUFFIX
+    # with an EXT_SUFFIX that matches the target platform
+    if get_platform() == get_target_platform():


If we change the definition of get_platform as per my post to python-dev, then to handle this I'd add a get_host_platform which is only used for this check.

Lib/distutils/util.py

PC/layout/main.py

PC/layout/support/options.py

paulmon · 2019-04-13T00:01:01Z

Rebased on master and cleaned up.
Most of the changes from the original PR have been refactored into seperate PRs.
These remaining changes should be refactored into mergable PRs, the current state of this PR is not mergable.

Lib/distutils/sysconfig.py

Lib/distutils/util.py

Lib/platform.py

Lib/test/test_codecs.py

Lib/test/test_mimetypes.py

Doc/library/platform.rst

zooba · 2019-04-22T18:00:18Z

Also, please try to avoid force pushing changes - it's much easier to review new commits. We'll squash merge at the end, so the history will disappear anyway.

paulmon · 2019-04-22T20:06:29Z

Sorry, I'll try to avoid force-pushing.

It was getting hard to merge these changes with the other arm changes for testing purposes because of the long list of commits here. There were a lot of conflicts because the other commits were lifted out of this one.

zooba · 2019-04-22T23:26:10Z

Lib/distutils/util.py

-
-    if targetPlatformFromEnvironment != None and targetPlatformFromEnvironment in TARGET_TO_PLAT:
-        targetPlatform = TARGET_TO_PLAT[targetPlatformFromEnvironment]
+    if os.name == 'nt':


Let's only define TARGET_TO_PLAT for Windows as well. Even though it's static, it still gets constructed on each call and it doesn't have any meaning on other OS's.

zooba · 2019-04-22T23:29:26Z

Force pushing to reset when you've done a big refactor or rebase is fine, since re-reviewing from the start is probably necessary. But for fix-ups we get a "view changes since last review" button when you just add new commits, which makes things much easier on our side.

If there isn't already a NEWS file for bpo-35920, please add one, and hopefully the macOS failure is transient and the rebuild will succeed. If so, I think this is ready to merge.

paulmon · 2019-04-23T00:14:24Z

Lib/distutils/util.py

@@ -38,7 +38,7 @@ def get_host_platform():
    if os.name == 'nt':
        if 'amd64' in sys.version.lower():
            return 'win-amd64'
-        if 'arm32' in sys.version.lower():
+        if 'arm' in sys.version.lower():


sys.version() returns ARM not arm32

sys.version
'3.8.0a3+ (heads/test:5145ef5d65, Apr 19 2019, 14:58:01) [MSC v.1916 32 bit (ARM)]'

zooba · 2019-04-23T22:10:20Z

@paulmon It looks like the macOS build issue is a real issue - circular dependency within distutils

Traceback (most recent call last):
  File "./setup.py", line 14, in <module>
    from distutils.command.build_ext import build_ext
  File "/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/command/build_ext.py", line 11, in <module>
    from distutils.core import Command
  File "/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/core.py", line 16, in <module>
    from distutils.dist import Distribution
  File "/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/dist.py", line 19, in <module>
    from distutils.util import check_environ, strtobool, rfc822_escape
  File "/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/util.py", line 14, in <module>
    from distutils.spawn import spawn
  File "/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/spawn.py", line 84, in <module>
    from distutils import sysconfig
  File "/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/sysconfig.py", line 18, in <module>
    from .util import get_platform, get_host_platform
ImportError: cannot import name 'get_platform' from 'distutils.util' (/Users/vsts/agent/2.150.0/work/1/s/Lib/distutils/util.py)

Probably best to move the from distutils import sysconfig line in spawn.py into the _spawn_posix function right before it's used.

paulmon · 2019-04-24T00:45:09Z

That line is spawn.py hasn't changed in a long time.

I think the problem is sysconfig.py line 18. I can't see a reason for the "from .util import get_platform, get_host_platform" to be there, it looks like it might an incomplete refactoring.

I've removed it and I'm doing some testing to make sure the standard tests still pass on PC and ARM.

zooba · 2019-04-24T17:58:57Z

I think the problem is sysconfig.py line 18. I can't see a reason for the "from .util import get_platform, get_host_platform" to be there, it looks like it might an incomplete refactoring.

I'd worry that get_platform may be being used as distutils.sysconfig.get_platform, especially since the original sysconfig module has get_platform as well. Changing spawn.py will have a smaller impact on users.

paulmon · 2019-04-24T18:30:10Z

Is the Azure pipelines PR build missing now? I think that's where the MacOS failure was showing up.

zooba · 2019-04-24T18:51:55Z

Hmm... guess the webhook got lost? Close/reopen is normally the best way. I'll do it.

paulmon · 2019-04-24T20:08:24Z

Waited an hour and tried again. Stills seems like the webhook is missing

zooba · 2019-04-24T22:17:42Z

I started a build manually at https://dev.azure.com/python/cpython/_build/results?buildId=41266

Other PRs seem to be working, so not sure what's going on here.

paulmon · 2019-04-24T23:21:55Z

Thanks. It looks like all of the builds passed now, including macOS.

zooba · 2019-04-25T18:36:53Z

Indeed. Congrats!

paulmon requested review from rhettinger, skrah and a team as code owners February 6, 2019 19:40

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

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

skrah reviewed Feb 6, 2019

View reviewed changes

Modules/_decimal/libmpdec/bits.h Outdated Show resolved Hide resolved

skrah reviewed Feb 6, 2019

View reviewed changes

PCbuild/_decimal.vcxproj Outdated Show resolved Hide resolved

zooba reviewed Feb 6, 2019

View reviewed changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 12, 2019

paulmon force-pushed the win-arm32-proof-of-concept branch from d4bcb80 to 278b786 Compare February 14, 2019 18:39

zooba reviewed Feb 14, 2019

View reviewed changes

Lib/distutils/util.py Outdated Show resolved Hide resolved

zooba reviewed Feb 20, 2019

View reviewed changes

PC/layout/main.py Outdated Show resolved Hide resolved

PC/layout/support/options.py Outdated Show resolved Hide resolved

paulmon force-pushed the win-arm32-proof-of-concept branch 2 times, most recently from 766fd54 to 6daef76 Compare February 27, 2019 22:43

paulmon force-pushed the win-arm32-proof-of-concept branch from 6f0d72b to 2c2d75e Compare April 12, 2019 23:58

zooba reviewed Apr 22, 2019

View reviewed changes

zooba mentioned this pull request Apr 22, 2019

Fixes platform.win32_ver on non-Windows platforms #12912

Merged

Paul Monson added 2 commits April 22, 2019 12:20

Address code review feedback

77d776e

fixes platform.win32_edition on non-Windows platforms

cec2b28

zooba reviewed Apr 22, 2019

View reviewed changes

paulmon changed the title ~~bpo-35920: CPython for Windows ARM32 proof of concept~~ bpo-35920: Windows 10 ARM32 platform support Apr 22, 2019

add news, fix get_platform()

fc21d47

paulmon commented Apr 23, 2019

View reviewed changes

fix macos test failure

b4c3a74

break circular dependency

d48686d

zooba closed this Apr 24, 2019

zooba reopened this Apr 24, 2019

paulmon closed this Apr 24, 2019

paulmon reopened this Apr 24, 2019

zooba merged commit 62dfd7d into python:master Apr 25, 2019

bedevere-bot removed the awaiting review label Apr 25, 2019

zooba mentioned this pull request May 11, 2023

Windows 10 ARM32 platform support #80101

Closed

		@@ -92,6 +94,23 @@ def get_platform ():

		# get_platform ()

		def get_target_platform ():

Uh oh!

bpo-35920: Windows 10 ARM32 platform support #11774

bpo-35920: Windows 10 ARM32 platform support #11774

Uh oh!

Conversation

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

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 6, 2019

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zooba commented Feb 6, 2019

Uh oh!

skrah commented Feb 6, 2019 via email

Uh oh!

paulmon commented Feb 6, 2019

Uh oh!

skrah commented Feb 6, 2019

Uh oh!

skrah commented Feb 6, 2019

Uh oh!

nascheme commented Feb 7, 2019

Uh oh!

nascheme commented Feb 7, 2019

Uh oh!

paulmon commented Feb 12, 2019

Uh oh!

paulmon commented Feb 14, 2019

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmon commented Apr 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zooba commented Apr 22, 2019

Uh oh!

paulmon commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

paulmon commented Feb 6, 2019 •

edited by bedevere-bot

Loading

paulmon commented Feb 14, 2019 •

edited

Loading

zooba commented Apr 22, 2019 •

edited by bedevere-bot

Loading

paulmon Apr 23, 2019 •

edited

Loading

paulmon commented Apr 24, 2019 •

edited

Loading