Enable arm64 for MSVC on Windows #5811

gaborkertesz-linaro · 2021-11-04T10:36:51Z

This patch enabled win-arm64 as a new platform for Windows.
Platform query and documentation is updated accordingly.

gaborkertesz-linaro · 2021-11-04T11:05:12Z

win-arm64_pytests.txt
win-arm64_selftests.txt

nulano · 2021-11-04T11:39:22Z

Could you please post the full Pytest output? In particular, I would be interested in seeing the skips and the list of loaded dependencies (at the start of the output).

gaborkertesz-linaro · 2021-11-04T14:42:25Z

x64_pytests_full.txt
win-arm64_pytests_full.txt

I attach the x64 results as a reference also as it looks that there 3 common failing tests.

kmilos · 2021-11-05T10:25:22Z

winbuild/build.rst

@@ -42,8 +42,8 @@ behaviour of ``build_prepare.py``:
  If ``PYTHON`` is unset, the version of Python used to run
  ``build_prepare.py`` will be used. If only ``PYTHON`` is set,
  ``EXECUTABLE`` defaults to ``python.exe``.
-* ``ARCHITECTURE`` is used to select a ``x86`` or ``x64`` build. By default,
-  uses same architecture as the version of Python used to run ``build_prepare.py``.
+* ``ARCHITECTURE`` is used to select a ``x86``, ``x64`` or ``x86_arm64`` (for win-arm64) build.


x86_arm64 seems very odd to see... why not simply arm64?

I agree it's strange.
It's coming from vcvarsall.bat (attached) and the fact the win-arm64 MSVC compiler is basically a cross-compiler, not native arm64. The host is working in x86 emulation mode.
vcvarsall.bat.txt

Ah, got it. Thanks.

Since this is an internal value only used by Pillow, there is nothing preventing the use of arm64. In fact, while vcvarsall uses x86_arm64, MSBuild uses ARM64 (according to this PR, I haven't checked myself). The x64 value already disagrees with vcvarsall's x86_amd64. So I would also vote for either arm64 or ARM64. Of the two, I would probably prefer ARM64 to match platform.machine(), but either is fine.

Good point, updated!

hugovk · 2021-11-07T07:05:10Z

Is this testable on GitHub Actions?

gaborkertesz-linaro · 2021-11-08T09:46:20Z

Not yet unfortunately.

gaborkertesz-linaro · 2021-11-10T13:39:07Z

I've investigated the following test: test_image_access.py::TestEmbeddable::test_embeddable

It's only enabled local Windows run: https://github.com/python-pillow/Pillow/blob/main/Tests/test_image_access.py#L399
https://github.com/python-pillow/Pillow/runs/4152656413?check_suite_focus=true#step:25:2227
However it fails for me on x64 and win-arm64 as well.

I tend to think this test should be removed or fix for all platforms as it should be a platform agnostic test as embedding into C/C++ code should be universal.

I see several issues:

First error: LINK : fatal error LNK1104: cannot open file 'python310.lib'

Library directory list doesn't include the Python libraries: https://github.com/python-pillow/Pillow/blob/main/Tests/test_image_access.py#L437
I was able to fix that by adding the missing libraries in test_image_access.py.

Next error: "python310.dll not found"

I was able to fix that by adding the required directory to the path in test_image_access.py also.

Next error: "ModuleNotFoundError: No module named 'encodings'"

I run Python in venv and the compiled C code set Python home to the venv's home directory, but changing that to the Python executable's home directory fixed that in: https://github.com/python-pillow/Pillow/blob/main/Tests/test_image_access.py#L413

Next error: https://github.com/python-pillow/Pillow/blob/main/Tests/test_image_access.py#L416 returns with error, which results AssertionError: assert 3221225477 == 0

nulano · 2021-11-10T13:57:39Z

Next error: https://github.com/python-pillow/Pillow/blob/main/Tests/test_image_access.py#L416 returns with error, which results AssertionError: assert 3221225477 == 0

For reference, 3221225477 == 0xC0000005, i.e. access violation. From MSDN:

Specifically, a C0000005 error is an access violation error caused by a buffer overrun.

gaborkertesz-linaro · 2021-11-10T14:05:28Z

Thanks!

However my main question is:
Would you welcome if I investigate this issue further? I'm hesitating a bit as this test is disabled by most cases. If you would prefer to see this to be fixed, I'll investigate it further.

nulano · 2021-11-10T15:12:22Z

Would you welcome if I investigate this issue further?

As far as I'm concerned, I wasn't around when the test was added, and I haven't seen it pass on Windows in the past 2 years. I've also not looked into it further myself.

winbuild/build.rst

gaborkertesz-linaro · 2021-11-10T19:38:06Z

Would you welcome if I investigate this issue further?

As far as I'm concerned, I wasn't around when the test was added, and I haven't seen it pass on Windows in the past 2 years. I've also not looked into it further myself.

Wouldn't it be a right time to remove this test to avoid further confusion?

gaborkertesz-linaro · 2021-11-16T10:06:19Z

@radarhere @nulano @hugovk Sorry for the reminder, but can I do anything to close this PR?

radarhere · 2021-12-28T21:14:48Z

winbuild/build_prepare.py

-        # commit: Merge branch 'master' into msvc (matches 2.16.0 tag)
-        "url": "https://github.com/ImageOptim/libimagequant/archive/f41ee301ff3a407b16991af3dbe03910919bbdc3.zip",  # noqa: E501
-        "filename": "libimagequant-f41ee301ff3a407b16991af3dbe03910919bbdc3.zip",
-        "dir": "libimagequant-f41ee301ff3a407b16991af3dbe03910919bbdc3",


Why switch to the master branch here? As the comment says, previously this used the msvc branch, so the 2.17.0 equivalent would be ImageOptim/libimagequant@e4c1334

I've updated to the 2.17.0 version of the msvc branch in #5916

nulano · 2021-12-29T00:27:30Z

winbuild/build.rst

-* ``ARCHITECTURE`` is used to select a ``x86`` or ``x64`` build. By default,
-  uses same architecture as the version of Python used to run ``build_prepare.py``.
+* ``ARCHITECTURE`` is used to select a ``x86``, ``x64`` or ``ARM64``build.
+  By default, uses same architecture as the version of Python used to run ``build_prepare.py``.
  is used.


This change seems to have disappered in a force-push. Is there any reason not to include it anymore?

Suggested change

is used.

is used. Note that the ``ARM64`` build is not fully supported and some dependencies may fail to build. Pull requests are welcome.

Sorry I didn't mention that LittleCMS is the last dependency and VS2019 update is merged, just not released yet:
mm2/Little-CMS#288
Patch to use VS2019 project: gaborkertesz-linaro@0af6042

So if either VS2019 project is accepted or VS2017 will be updated for ARM64 as well and LCMS2 is released, all external dependencies will be enabled.
Probably this pull request can be blocked until updated LCMS2 is released.

lcms2 2.13 has now been released.

Thanks for the heads up, I'm going to rebase my changes and run the tests!

gaborkertesz-linaro · 2022-01-31T15:43:49Z

01_build_prepare.log
02_build_dep_all.log
03_build_pillow_install.log
04_selftest.log
05_Tests.log

nulano · 2022-01-31T17:29:17Z

Could you please re-run the tests with C:\kg\python_test\github_packages\Pillow\winbuild\build\bin added to PATH? It seems fribidi.dll wasn't found, likely because of this. Looks mostly OK otherwise. The commands should probably be something like:

PATH %PATH%;C:\kg\python_test\github_packages\Pillow\winbuild\build\bin
"C:\kg\python_test\venv_310_new\Scripts\python.exe" -m pytest -v Tests

This patch enables ARM64 as a new platform for Windows. Platform query and documentation is updated accordingly.

In order to enable win-arm64, VS2019 should be used, while other platforms should work with newer version as well. Tested on x64-win10.

gaborkertesz-linaro · 2022-02-01T10:08:02Z

05_Tests2.log
Tests are repeated with Pillow\winbuild\build\bin added to the path.

nulano

Thank you, looks mostly good.

The first three failures seem to be a difference in floating point rounding:

FAILED Tests/test_color_lut.py::TestTransformColorLut3D::test_3_to_4_channels
    At index 4 diff: 0.04000000000000001 != 0.04
FAILED Tests/test_color_lut.py::TestTransformColorLut3D::test_4_to_4_channels
    At index 4 diff: 0.04000000000000001 != 0.04
FAILED Tests/test_color_lut.py::TestTransformColorLut3D::test_with_normals_3_channels
    At index 12 diff: 0.15999999999999992 != 0.16000000000000003

The next two could be caused by a similar difference, being a small margin above the epsilon (but hard to be sure without visually inspecting the images):

FAILED Tests/test_file_wmf.py::test_load_raw - AssertionError:  average pixel value difference 2.0479 > epsilon 2.0000
FAILED Tests/test_file_wmf.py::test_load_set_dpi - AssertionError:  average pixel value difference 2.6547 > epsilon 2.1000

The next failed test I have never seen pass on Windows:

FAILED Tests/test_image_access.py::TestEmbeddable::test_embeddable - distutil...

And the last one could be due to how the tests are being run. Are you running the tests directly in a desktop session, or are you using some sort of terminal without a desktop, e.g. SSH? Either way this seems like a separate issue unrelated to ARM.

FAILED Tests/test_imagegrab.py::TestImageGrab::test_grab - OSError: screen grab failed

gaborkertesz-linaro · 2022-02-01T13:13:40Z

And the last one could be due to how the tests are being run. Are you running the tests directly in a desktop session, or are you using some sort of terminal without a desktop, e.g. SSH? Either way this seems like a separate issue unrelated to ARM.

I connect to my local PC via remote desktop and run the tests in Windows Terminal, so I'd expect this shouldn't prevent screengrab. Probably as a follow-up taks this could be investigated.

gaborkertesz-linaro · 2022-02-02T09:01:30Z

Thanks for the thorough and persistent support!
If anybody ever interested about Pillow and dependencies porting to WoA, I've created a confluence page summary:
https://linaro.atlassian.net/wiki/spaces/WOAR/pages/28654632982/A+tale+of+a+dependency+chain+Pillow+and+the+crew

nulano · 2022-02-02T11:02:15Z

If anybody ever interested about Pillow and dependencies porting to WoA, I've created a confluence page summary: https://linaro.atlassian.net/wiki/spaces/WOAR/pages/28654632982/A+tale+of+a+dependency+chain+Pillow+and+the+crew

A small clarification: Pillow doesn't use libpng, HarfBuzz, FriBiDi directly. These are only listed in build_prepare because they are used by the other libraries.

Specifically, libpng is used by FreeType to support color fonts in CBDT/SBIX format, and HarfBuzz/FriBiDi are dependencies of Raqm. Raqm used to be built directly, but as of Pillow 8.2.0 a modified version is included in the sources and is now the default on Windows. For details see: https://pillow.readthedocs.io/en/stable/installation.html#external-libraries

gaborkertesz-linaro · 2022-02-02T13:04:16Z

Thanks, I'll fix these issues!
Why libpng is needed to be imported directly by Pillow, while FreeType imports for itself anyway?

nulano · 2022-02-02T13:25:47Z

Why libpng is needed to be imported directly by Pillow, while FreeType imports for itself anyway?

I don't understand your question. Doing a grep for libpng in Pillow only shows build_prepare.py and test-windows.yml for me, the latter containing a comment that libpng is only used by FreeType.

Are you asking why build_prepare generates a script to compile the indirect dependencies? The VS2010 build of FreeType doesn't build libpng directly, libpng must already be present and compiled somewhere, so build_prepare takes care of that.

gaborkertesz-linaro · 2022-02-02T13:50:43Z

Thanks, yes, that was my concern that FreeTypes includes libpng dependency in its source and I'd expect that ideally it should take care of build dependencies:
https://gitlab.freedesktop.org/freetype/freetype/-/blob/master/subprojects/libpng.wrap
However I see the released package doesn't include the subprojects.

gaborkertesz-linaro force-pushed the winarm64 branch from 39ec132 to 3c24aa3 Compare November 4, 2021 11:27

radarhere added the Windows label Nov 4, 2021

kmilos reviewed Nov 5, 2021

View reviewed changes

gaborkertesz-linaro force-pushed the winarm64 branch from 3c24aa3 to 69c888a Compare November 8, 2021 10:17

nulano reviewed Nov 10, 2021

View reviewed changes

winbuild/build.rst Show resolved Hide resolved

gaborkertesz-linaro force-pushed the winarm64 branch from b7428b1 to 7687186 Compare December 7, 2021 15:46

hugovk approved these changes Dec 28, 2021

View reviewed changes

radarhere reviewed Dec 28, 2021

View reviewed changes

nulano reviewed Dec 29, 2021

View reviewed changes

radarhere mentioned this pull request Dec 30, 2021

Updated winbuild libimagequant to 2.17.0 #5916

Merged

gaborkertesz-linaro force-pushed the winarm64 branch from 7687186 to def67cf Compare January 3, 2022 14:14

nulano mentioned this pull request Jan 6, 2022

Add missing ImageModes #5935

Merged

gaborkertesz-linaro force-pushed the winarm64 branch from def67cf to 3da0c59 Compare January 31, 2022 15:42

gaborkertesz-linaro and others added 2 commits February 1, 2022 09:18

Windows: Enable ARM64 for MSVC

f22d356

This patch enables ARM64 as a new platform for Windows. Platform query and documentation is updated accordingly.

lcms2: Update to VS2019

22a1f6d

In order to enable win-arm64, VS2019 should be used, while other platforms should work with newer version as well. Tested on x64-win10.

gaborkertesz-linaro force-pushed the winarm64 branch from 3da0c59 to 22a1f6d Compare February 1, 2022 10:07

nulano approved these changes Feb 1, 2022

View reviewed changes

radarhere merged commit c74313a into python-pillow:main Feb 2, 2022

radarhere changed the title ~~Windows: Enable win-arm64 for MSVC~~ Enable arm64 for MSVC on Windows Feb 2, 2022

This was referenced Oct 25, 2022

Failed to build Windows ARM64 dependencies #6679

Closed

Add xfail mark to TestEmbeddable #6703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable arm64 for MSVC on Windows #5811

Enable arm64 for MSVC on Windows #5811

gaborkertesz-linaro commented Nov 4, 2021

gaborkertesz-linaro commented Nov 4, 2021

nulano commented Nov 4, 2021

gaborkertesz-linaro commented Nov 4, 2021

kmilos Nov 5, 2021

gaborkertesz-linaro Nov 5, 2021

kmilos Nov 5, 2021

nulano Nov 6, 2021

gaborkertesz-linaro Nov 8, 2021

hugovk commented Nov 7, 2021

gaborkertesz-linaro commented Nov 8, 2021 •

edited by radarhere

gaborkertesz-linaro commented Nov 10, 2021 •

edited

nulano commented Nov 10, 2021

gaborkertesz-linaro commented Nov 10, 2021 •

edited by radarhere

nulano commented Nov 10, 2021

gaborkertesz-linaro commented Nov 10, 2021

gaborkertesz-linaro commented Nov 16, 2021

radarhere Dec 28, 2021

radarhere Dec 30, 2021

nulano Dec 29, 2021 •

edited

gaborkertesz-linaro Jan 3, 2022 •

edited

radarhere Jan 29, 2022

gaborkertesz-linaro Jan 31, 2022

gaborkertesz-linaro commented Jan 31, 2022

nulano commented Jan 31, 2022

gaborkertesz-linaro commented Feb 1, 2022

nulano left a comment •

edited

gaborkertesz-linaro commented Feb 1, 2022

gaborkertesz-linaro commented Feb 2, 2022

nulano commented Feb 2, 2022 •

edited

gaborkertesz-linaro commented Feb 2, 2022 •

edited by radarhere

nulano commented Feb 2, 2022

gaborkertesz-linaro commented Feb 2, 2022

	is used.
	is used. Note that the ``ARM64`` build is not fully supported and some dependencies may fail to build. Pull requests are welcome.

Enable arm64 for MSVC on Windows #5811

Enable arm64 for MSVC on Windows #5811

Conversation

gaborkertesz-linaro commented Nov 4, 2021

gaborkertesz-linaro commented Nov 4, 2021

nulano commented Nov 4, 2021

gaborkertesz-linaro commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugovk commented Nov 7, 2021

gaborkertesz-linaro commented Nov 8, 2021 • edited by radarhere

gaborkertesz-linaro commented Nov 10, 2021 • edited

nulano commented Nov 10, 2021

gaborkertesz-linaro commented Nov 10, 2021 • edited by radarhere

nulano commented Nov 10, 2021

gaborkertesz-linaro commented Nov 10, 2021

gaborkertesz-linaro commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulano Dec 29, 2021 • edited

Choose a reason for hiding this comment

gaborkertesz-linaro Jan 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborkertesz-linaro commented Jan 31, 2022

nulano commented Jan 31, 2022

gaborkertesz-linaro commented Feb 1, 2022

nulano left a comment • edited

Choose a reason for hiding this comment

gaborkertesz-linaro commented Feb 1, 2022

gaborkertesz-linaro commented Feb 2, 2022

nulano commented Feb 2, 2022 • edited

gaborkertesz-linaro commented Feb 2, 2022 • edited by radarhere

nulano commented Feb 2, 2022

gaborkertesz-linaro commented Feb 2, 2022

gaborkertesz-linaro commented Nov 8, 2021 •

edited by radarhere

gaborkertesz-linaro commented Nov 10, 2021 •

edited

gaborkertesz-linaro commented Nov 10, 2021 •

edited by radarhere

nulano Dec 29, 2021 •

edited

gaborkertesz-linaro Jan 3, 2022 •

edited

nulano left a comment •

edited

nulano commented Feb 2, 2022 •

edited

gaborkertesz-linaro commented Feb 2, 2022 •

edited by radarhere