bpo-35920: Windows 10 ARM32 platform support

[paulmon]

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]

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!

[zooba]

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.

[paulmon]

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

[zooba]

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.

[zooba]

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

[paulmon]

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

[zooba]

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

[zooba]

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]

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]

@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]

@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]

@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]

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]

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]

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]

This PR has been rebased on the merged PCBuild changes

[paulmon]

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

[zooba]

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.

[paulmon]

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.

[zooba]

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]

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]

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]

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]

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]

@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]

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]

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]

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

[zooba]

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

[paulmon]

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

[zooba]

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]

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

[zooba]

Indeed. Congrats!