Test Windows with GitHub Actions

[nulano]

For #3606

Adds Windows tests to GitHub Actions.

Existing scripts winbuild\build_dep.py and winbuild\build.py expect MSVS2015, but GitHub Actions only comes with MSVS2017 (or MSVS2019). It does come with a backwards-compatibility component, but it is incompatible with the existing scripts. For now this uses build scripts hardcoded in test-windows.yml, I would like to update the winbuild scripts once I have all the dependencies working.

Dependency builds are split into separate steps to make logs easier to read. If kept, this would require another change in winbuild\build_dep.py.

Available dependencies:

• libjpeg
• libjpeg-turbo
• zlib
• webp
• libtiff
• freetype
• lcms2
• openjpeg
• ghostscript
• libimagequant
• raqm

Strikethrough indicates potential licensing issues for binary distribution.

TODO

• Only Python 3.6.8 for now. TODO add 3.5.4 and 3.7.4
• Only x86 for now. TODO add x64
• Add PyPy3
• Make sure dependencies are built for Release
• Fix all LNK4098 warnings
• Update winbuild\build_dep.py and winbuild\build.py (in future PR)
• Upload coverage. Depends on Test Linux and macOS with GitHub Actions #4081 for token.
• Build wheels

[nulano]

This now matches AppVeyor Python 3.x jobs; I don't think it's possible to compile for Python 2.7 on GitHub Actions. Note that AppVeyor does try to compile lcms2, but it errors out: https://ci.appveyor.com/project/Python-pillow/pillow/builds/27627690/job/ual03214w7mvbx4q?fullLog=true#L1214

---

PyPy3 x86 should be easy to add, but I don't know if there is a way to add it to the matrix without also adding PyPy3 x64, which doesn't exist.

Duplicating the job would work, but that would lead to a lot of duplicate code. I think the current test-windows.yml scripts should be merged with winbuild\build_dep.py, but I like the way dependency builds are split in the logs.

I therefore propose changing winbuild\build_dep.py to generate separate scripts for each dependency and a single script which runs them all (e.g. for AppVeyor).

I would also like to change winbuild\build.py such that it only generates build scripts, but doesn't run them (similarly to my build_dep.py proposal), as the current way leads to messy logs.

Before I mess with winbuild, I would like to check whether this could break something else (such as Windows releases?). What do you think?

[hugovk]

Great work!

The 6 * Python 3 jobs take ~28 minutes on AppVeyor (in serial), and ~7 minutes on Actions (all in parallel)!

Don't worry about 2.7, we're dropping it soon.

---

Perhaps exclude can be used to test PyPy3 x86 and exclude PyPy3 x64, something like this?

runs-on: ${{ matrix.os }}
strategy:
  matrix:
    os: [macOS-10.14, windows-2016, ubuntu-18.04]
    node: [4, 6, 8, 10]
    exclude:
      # excludes node 4 on macOS
      - os: macos-10.14
        node: 4

Example from https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobsjob_idstrategymatrix

---

I'm generally fine with refactoring winbuild. We're not using AppVeyor to release anything, a lack of dependencies was the main blocker for that: #553 (comment). So right now, it won't break any Windows releases.

It would be a great longer term goal to build Windows wheels via CI, to remove the one-person bus factor.

And if we do so, I'd favour doing so with Actions so we can retire or at least minimise AppVeyor.

[nulano]

Perhaps exclude can be used to test PyPy3 x86 and exclude PyPy3 x64, something like this?
[...]
Example from https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobsjob_idstrategymatrix

Done on my branch, thank you!

I have 21 hung jobs I can't cancel ("Failed to cancel check suite.") due to #4074, once they time out and I can test it, I will push it here and mark Ready for review.

---

I'm generally fine with refactoring winbuild. We're not using AppVeyor to release anything, a lack of dependencies was the main blocker for that: #553 (comment). So right now, it won't break any Windows releases.

It would be a great longer term goal to build Windows wheels via CI, to remove the one-person bus factor.

#4029 (comment):

Those files are produced by my build script, which simply runs setup.py with bdist_wheel, bdist_egg, and bdist_wininst arguments for all supported CPython versions.

I remembered this as build.py... I'll go ahead and refactor winbuild once the rest of this PR is done.

---

And if we do so, I'd favour doing so with Actions so we can retire or at least minimise AppVeyor.

Agreed.

[hugovk]

I have 21 hung jobs I can't cancel due to #4074, once they time out and I can test it, I will push it here and mark Ready for review.

Can you click the "Cancel check suite" button?

https://help.github.com/en/articles/managing-a-workflow-run#canceling-a-workflow-run

[nulano]

Can you click the "Cancel check suite" button?

It gives me an error.
I reported the issue here: https://git.luolix.topmunity/t5/GitHub-Actions/Can-t-cancel-hung-job-quot-Failed-to-cancel-check-suite-quot/m-p/32673#M1216

[hugovk]

6 hour timeout, yikes.

I've pushed your gha-win-wip branch to my fork to run it here:

https://github.com/hugovk/Pillow/commit/9def3c588a1dd56f32338c628552dce77a6c10b4/checks

All green except PyPy3.

---

I saw the check for the GITHUB_ACTIONS env var here:

nulano@9def3c5#diff-2deb7e4843eb6ebdbc14446813affc8eR359

I've not tested it, but it looks like it should be GITHUB_ACTION without the S:

• https://git.luolix.topmunity/t5/GitHub-Actions/Have-the-CI-environment-variable-set-by-default/m-p/32671/highlight/true#M1215
• https://help.github.com/en/articles/virtual-environments-for-github-actions#default-environment-variables

[nulano]

6 hour timeout, yikes.

I've pushed your gha-win-wip branch to my fork to run it here:

Thanks!

---

https://github.com/hugovk/Pillow/commit/9def3c588a1dd56f32338c628552dce77a6c10b4/checks

All green except PyPy3.

debug: OperationError:
debug:  operror-type: LookupError
debug:  operror-value: cp65001 encoding is only available on Windows

Looks like yet another PyPy bug, sigh...

I'll confirm this tomorrow by skipping the failing test.

---

I've not tested it, but it looks like it should be GITHUB_ACTION without the S:

Hmm... I've run cmd /c "set" to dump all enviroment variables and there is an undocumented variable GITHUB_ACTIONS=true: https://github.com/nulano/Pillow/commit/4b4917a31c69b2a0deb143b01b5feb839a83b937/checks#step:2:25

I suppose GITHUB_ACTION will suffice until the docs are updated.

[hugovk]

GitHub staff replied:

There's two distinct values here. GITHUB_ACTION is the name of the action that's running, if any (and there may not be one). GITHUB_ACTIONS is always set to show that you're running within GitHub Actions.

https://git.luolix.topmunity/t5/GitHub-Actions/Have-the-CI-environment-variable-set-by-default/td-p/32125

Let's stick with the plural.

[nulano]

PyPy3 added, marking ready for review. The remaining tasks can be done in future PRs if this is merged in the meantime.

I added a 30min timeout (triple PyPy3 build time) to avoid what happened yesterday.

---

PyPy bug reported and test skipped: https://bitbucket.org/pypy/pypy/issues/3081/operationerror-cp65001-is-only-available

[hugovk]

Some minor things

[nulano]

Actually, all freetype tests are broken on PyPy3 with Raqm, so it's probably my fault, or a bug in PyPy... unrelated to #3978.

For example:

[hugovk]

I'd really like to get this PR merged in soon.

It'd be fine for PyPy3 to follow in a later PR, the main thing would be to replace the bulk of the AppVeyor jobs. What is important before removing AppVeyor jobs is coverage.

Having said that, there's a lot of value in having these GHA jobs merged in and running in parallel with AppVeyor, even if coverage is to follow, just because they're so fast and we'll get build feedback much quicker.

---

Coverage:

• Upload coverage. Depends on Test Linux and macOS with GitHub Actions #4081 for token.

I added a token to .codecov.yml in #4081. I'm not yet sure if the coverage uploaded to Codecov on my fork and the Pillow repo are fully consistent, but feel free to grab the same token to try out here too.

Another place it go is hardcoded in test*.yml:

      env:
        CODECOV_TOKEN: abcde12345uvwxyz

Ideally forks could still have correct coverage showing on their own Codecov page, before PRs are made. And then a PR would have correct coverage on the Pillow Codecov page.

Thanks!

[aclark4life]

@hugovk Merge-away from me pending everyone else's comfort level and everyone please allow me to echo @wiredfool thanks to @nulano !! (@nulano you should consider joining the Pillow team if you are interested cf. #4048 . We could add you to the team now as a volunteer, then if that goes well, add you to Tidelift in 2020 …)

[nulano]

Is there a reason why the external C libraries are built/linking against the Multi-threaded (/MT) runtime library instead of Multi-threaded DLL (/MD), which is used by Python/distutils?

@cgohlke Look at the Build Pillow step. I noticed that distutils chooses /MT for CPython 3.x, but /MD for PyPy3. I'm not sure whether there was a change in CPython, but I guessed that there was based on this. I do remember seeing /MD in Python 2.7 builds on AppVeyor.

---

I'd really like to get this PR merged in soon.

It'd be fine for PyPy3 to follow in a later PR, the main thing would be to replace the bulk of the AppVeyor jobs. What is important before removing AppVeyor jobs is coverage.

Having said that, there's a lot of value in having these GHA jobs merged in and running in parallel with AppVeyor, even if coverage is to follow, just because they're so fast and we'll get build feedback much quicker.

I pushed Raqm with PyPy3 disabled for now. My current theory is that this is due to the /MD vs /MT difference between CPython and PyPy, but I think this can be tested later.

I would definitely make sure AppVeyor is kept functional, at least until GHA is out of beta; just reducing it to one or two jobs would probably be a good start. This is a good idea especially when you consider yesterday's 2 hour outage: actions/checkout#49

I would be more comfortable adding in the token in a separate PR, so feel free to merge now.

---

[...] (@nulano you should consider joining the Pillow team if you are interested cf. #4048 . We could add you to the team now as a volunteer, then if that goes well, add you to Tidelift in 2020 …)

I will definitely consider this, but I expect to become quite busy over the next few weeks, so I'll get back to you later once I've got my schedule figured out.

[nulano]

I noticed that distutils chooses /MT for CPython 3.x

That should not be. I would recommend not to distribute these builds. Do the Visual Studio installations include the redistributable files? Could be that distutils fails to find the vcruntime_redist files and switches to /MT.

Looking at the source of distutils on my desktop, this does seem to make sense. I am pretty sure the Visual Studio install does have the required files, both because the PyPy3 distutils finds it, and because I see the file on my desktop Visual Studio 2017 Community install (in C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Redist\MSVC\14.14.26405\x86\Microsoft.VC141.CRT\vcruntime140.dll). I'm not sure why CPython's distutils can't see it...

Also note it is the same on AppVeyor which has VS2015.

[nulano]

I found the issue. After removing this line, Pillow is built with /MD:

Pillow/.github/workflows/test-windows.yml

Line 316 in ae8fd74

set DISTUTILS_USE_SDK=1

Looking through _msvccompiler setting this value makes distutils skip looking for the redistributable files and env['py_vcruntime_redist'] is never set. Does anyone remember why this line was added? I only added it because I copied it from AppVeyor.

Edit: git blame: 1ad678a, from 6 years ago... Windows SDKs have since been changed to be a VS component, not a separate install, and SetEnv is not present on GHA (as I remember from running a search a week ago).

@cgohlke Do you see any issue with removing this line?

[aclark4life]

@nulano Probably fine pending @cgohlke's confirmation … FWIW, here's some docs about that environment variable: https://docs.python.org/3/distutils/apiref.html#module-distutils.msvccompiler

[nulano]

Everything is /MD now (except ghostscript, which is only called through the exe). PyPy3 still fails with Raqm, might be a PyPy bug.

I think the AppVeyor failure is an intermittent PyPy bug, I faintly remember a similar problem when I was working on PyPy3 for AppVeyor. Restarting it might fix it.
(EDIT: PyPy2 passed on my repo: https://ci.appveyor.com/project/nulano/pillow/builds/27807172/job/jaw1mxgbkl6mupm0)

I also ran all tests on a clean Vista VM with the 3.7x86 wheel, getting two failures: missing Visual Studio and missing PowerShell, both only needed for Tests.

[hugovk]

Are we good to merge this once the conflicts are resolved?

I would definitely make sure AppVeyor is kept functional, at least until GHA is out of beta; just reducing it to one or two jobs would probably be a good start. This is a good idea especially when you consider yesterday's 2 hour outrage: actions/checkout#49

Good point, let's keep both ticking over until at least after launch. We can use that time for any updates/tuning needed for this.

[hugovk]

Thank you very much!

[tvatter]

Quick question, I managed to follow some of your steps for a library that I develop, but only using windows-2016 and not windows-latest. Given that it's going to be deprecated tomorrow, it's kind of a bummer. Have you thought about this already?

Essentially, with windows-latest, although "C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" x86 executes without problem, I still get the dreaded error: Microsoft Visual C++ 14.0 is required. :(

[radarhere]

@tvatter see #4188

[tvatter]

Thanks! (what a mess ^^)