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

GHA: Add Windows in Docker #4461

Closed
wants to merge 5 commits into from
Closed

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Mar 2, 2020

Adds tests for (and includes) #4372

This PR adds a Windows run in a Windows Docker instance.

The build script is just test-windows.yml with updated paths. The Dockerfile is based on the guide from Microsoft. helper.is_win32_docker is based on this StackOverflow answer.

This is mostly intended as a proof of concept; I imagine it would be better to add the Docker image to python-pillow/docker-images, as it takes almost 20 minutes to build the image. While the Image does have to be build on Windows 10 Pro (or Windows Server 2019), if no Pillow members have access to such a system, it is possible to build and push the image from GHA itself.

If such a prebuilt image is used, I think it would be a good idea to add the optional dependencies currently not being tested on Windows, such as numpy, as that wouldn't affect build times, or add extra dependencies on CI.

There is no coverage yet.

@umarcor
Copy link

umarcor commented Nov 3, 2020

@nulano, I find this proof of concept very interesting. However, I wonder what's the use case for using Windows containers, given that those work on Windows hosts only, while Linux containers work on Linux, Windows or macOS.

@nulano
Copy link
Contributor Author

nulano commented Nov 3, 2020

@nulano, I find this proof of concept very interesting. However, I wonder what's the use case for using Windows containers, given that those work on Windows hosts only, while Linux containers work on Linux, Windows or macOS.

When I first started working on Pillow, it was very difficult to compile on Windows due to hardcoded paths. For that reason, I found it easier to work in Docker, as I could more easily update the paths to something predictable. Since my rewrite of the Windows scripts #4495 has been merged, it is no longer as useful for local development anymore (see the difference in fc48553).

While running in Docker I found a bug that could only be reproduced when running on Windows in headless mode, and I think this is the only way of testing that on Github Actions. Another advantage of this approach was that, if updated to use a prebuilt container image, it was easier to set up effective caching for the Windows builds; this is no longer needed since #4701.

@umarcor
Copy link

umarcor commented Nov 4, 2020

Thanks a lot for that very detailed insight!

@nulano
Copy link
Contributor Author

nulano commented Oct 28, 2022

This is not very useful anymore as explained in my previous comment, closing.

@nulano nulano closed this Oct 28, 2022
@nulano
Copy link
Contributor Author

nulano commented Jan 1, 2023

@umarcor You might be interested in looking at #6847, which also adds testing in Docker. Rather than installing Visual Studio to compile the wheel (as in this PR), that PR uses a precompiled wheel and tests it on a fresh Windows install to make sure that all required dependencies are correctly packaged (unlike in e.g. #6701). An alternative to using Docker is to use Windows Sandbox as suggested previously: #5573 (comment)

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

Successfully merging this pull request may close these issues.

3 participants