Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Winbuild rewrite #4495

Merged
merged 26 commits into from
May 25, 2020
Merged

Winbuild rewrite #4495

merged 26 commits into from
May 25, 2020

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Mar 29, 2020

As promised in #4084 (comment).

A complete rewrite of winbuild scripts, based on the current test-windows.yml scripts, to unify Windows CI scripts and simplify local development. Instead of a single script to compile all dependencies, there is a separate script for each dependency. While this is slightly slower (due to vcvarsall.bat), I have found this to be very helpful when adding / changing dependencies. The larger factor slowing down AppVeyor is support for more dependencies. It might be a good idea to remove a few more AppVeyor jobs here.

Another difference is that this version uses format strings instead of ifs to fill in build parameters, which is IMO easier to read. Additionally, all commands are wrapped in functions to avoid repeating parameters for similar commands.

This PR uses vswhere to automatically find VS using code based on distutils, it should therefore be fairly reliable and easy to use for other contributors, but it only supports VS versions starting with 2017. I have tested it on:

  • GitHub Actions with VS 2019 Enterprise (windows-2019 image),
  • AppVeyor with VS 2017 Community (VS 2017 image),
  • Windows ltsc2019 in Docker with VS 2019 Build Tools (mcr.microsoft.com/dotnet/framework/sdk:4.8-windowsservercore-ltsc2019 image),
  • Windows 10 Home (1903) with VS 2017 Community (clean and minimal install),
  • Windows 10 Pro with VS 2019 Community.

This PR switches AppVeyor to the Windows Server 2016 image (Windows 8 10 (1607)) due to VS 2017 availability. This means Windows 7 8.1 (Windows Server 2012 R2) support will no longer be automatically tested. I don't think this is a big issue, as both systems Windows 8.1 is now EOL; if you disagree, I can try to add VS 2015 support.
Edit: I had the Windows version numbers wrong.

Note that since VS 2015, all MSVC versions are supposed to be ABI compatible, so any such VS version can be used to compile Python 3.5+ packages.

While build.rst claims Python 3.6+ is required to create valid build scripts, this only affects build_dep_all.cmd, which is not used on GHA, so Python 3.5 is fine there.

Please let me know if you would like me to change anything.

@hugovk

This comment has been minimized.

@hugovk

This comment has been minimized.

@nulano

This comment has been minimized.

@hugovk

This comment has been minimized.

@hugovk
Copy link
Member

hugovk commented Mar 29, 2020

Thanks for this! Couple of quick replies:

The larger factor slowing down AppVeyor is support for more dependencies. It might be a good idea to remove a few more AppVeyor jobs here.

Agreed, AppVeyor is the build bottleneck. Approx end-to-end running times:

  • AppVeyor: 33 minutes for 6 jobs
  • GitHub Actions: 12m for 33 jobs
  • Travis CI: 8m for 9 jobs

What would you suggest removing?

This PR switches AppVeyor to the Windows Server 2016 image (Windows 8) due to VS 2017 availability. This means Windows 7 (Windows Server 2012) support will no longer be automatically tested. I don't think this is a big issue, as both systems are now EOL; if you disagree, I can try to add VS 2015 support.

I think that's fine: "In general, we aim to support all current versions of Linux, macOS, and Windows".

https://pillow.readthedocs.io/en/stable/installation.html#platform-support

@nulano
Copy link
Contributor Author

nulano commented Mar 29, 2020

What would you suggest removing?

I would suggest removing Python38-x64 and Python35, and keeping Python38 and Python35-x64. This way the newest and oldest Python, and both architectures are being tested on a different system. This should get AV back to pre-PR speed.

I think MSYS2 can be moved to GHA once it is preinstalled (actions/runner-images#632).

I'm not sure about PyPy. Since it is installed manually on AV, it can be updated as soon as a new version is released, unlike GHA where it takes at least a week (they mention in multiple issues that this is their standard rollout period). But it does take almost 8 min...

@hugovk
Copy link
Member

hugovk commented Mar 29, 2020

What would you suggest removing?

I would suggest removing Python38-x64 and Python35, and keeping Python38 and Python35-x64. This way the newest and oldest Python, and both architectures are being tested on a different system. This should get AV back to pre-PR speed.

I think MSYS2 can be moved to GHA once it is preinstalled (actions/virtual-environments#632).

👍

I'm not sure about PyPy. Since it is installed manually on AV, it can be updated as soon as a new version is released, unlike GHA where it takes at least a week (they mention in multiple issues that this is their standard rollout period). But it does take almost 8 min...

We rarely need to have PyPy available as soon as it's released, so following the standard rollout should be fine. A week or a few shouldn't be much of a problem. And we're basically following standard rollouts for all the other CIs/versions.

@python-pillow python-pillow deleted a comment from codecov bot Mar 30, 2020
@nulano
Copy link
Contributor Author

nulano commented Mar 30, 2020

I have merged with master, removed Python38, Python35-x64, and PyPy3 from AppVeyor, and updated install.rst. AppVeyor now takes ~16 min.

@python-pillow python-pillow deleted a comment from codecov bot Mar 30, 2020
@nulano nulano mentioned this pull request Apr 11, 2020
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint fixes

winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
@nulano
Copy link
Contributor Author

nulano commented Apr 12, 2020

It seems @radarhere's rebase has removed some changes from merge commits. For example, #4342 changed test-windows.yml, so I resolved the conflict by updating build_prepare.py accordingly. The rebase removed this merge commit, and thus also this change. (Effectively reverting #4342 in this branch.) This is also what caused the lint failure. I'll compare my local copy with the rebase to restore these changes in a separate commit.

On that note, does anyone know how to do a merge like that properly to avoid this in the future? Or should I ask on StackOverflow (a brief google didn't find anything)?

@hugovk
Copy link
Member

hugovk commented Apr 14, 2020

@cgohlke Hi, please could you have a look at this PR? At least to check your it's okay with your workflow? Thank you!

@cgohlke
Copy link
Contributor

cgohlke commented Apr 14, 2020

I don't see anything in this PR that could interfere with my build system. I don't rely on winbuild or build instructions.

@@ -292,9 +292,7 @@ or from within the uncompressed source directory::
Building on Windows
^^^^^^^^^^^^^^^^^^^

We don't recommend trying to build on Windows. It is a maze of twisty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear - so you think that this PR improves the situation enough that it is now viable for users to compile from source by themselves?

Copy link
Contributor Author

@nulano nulano Apr 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While installing Visual Studio is not exactly easy, once that is done, compiling is as difficult as running the script. It is by no means a maze anymore. I did not have to do any extra configuration on my new laptop whatsoever.

That said, when writing it I was thinking more about developers (contributing PRs) than users. AFAIK most Windows users aren't used to compiling code at all, so perhaps it would be best to write it as "We recommend you use prebuilt wheels from PyPI. If you wish to compile Pillow manually, you can use the build scripts and notes ..."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@radarhere any comments on the current wording? Okay to merge?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording is fine, just wanted to check that we knew that we were stepping up our level of commitment to building from source on Windows.

@nulano
Copy link
Contributor Author

nulano commented May 9, 2020

Thanks to v2 of actions/upload-artifact, I have enabled wheel building for PyPy (it was disabled due to .egg filename conflict).

I also updated the windows instructions as follows:

We recommend you use prebuilt wheels from PyPI. If you wish to compile Pillow manually, you can use the build scripts in the winbuild directory used for CI testing and development. These scripts require Visual Studio 2017 or newer and NASM.

@hugovk
Copy link
Member

hugovk commented May 24, 2020

@nulano Please could you resolve the conflicts? Then let's get this merged. Thanks!

…d-rewrite

# Conflicts:
#	.github/workflows/test-windows.yml
#	winbuild/config.py
#       winbuild/build_prepare.py
@nulano
Copy link
Contributor Author

nulano commented May 25, 2020

Merged with master; Travis failed because Python 3.5 on s390x was cancelled for some reason.

@hugovk
Copy link
Member

hugovk commented May 25, 2020

Thank you! It was hanging so I cancelled it, and have just restarted it. I'll give it a chance to finish, but it's not related to this PR so we can merge it without it too.

@hugovk hugovk merged commit 3cc030e into python-pillow:master May 25, 2020
@hugovk
Copy link
Member

hugovk commented May 25, 2020

✅ Thanks again for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants