-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Build Windows wheels using cibuildwheel #7580
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
Conversation
| if shutil.which("djpeg"): | ||
| try: | ||
| subprocess.check_call(["djpeg", "-version"]) | ||
| return True | ||
| except subprocess.CalledProcessError: | ||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
djpeg.exe and cjpeg.exe require VC++ redistributable packages, but the whole point of testing in Docker is to not install those. Running these during CPython tests therefore crashes with a message about missing DLLs. PyPy tests do require VC++, so they are still tested there.
.github/workflows/wheels.yml
Outdated
| docker run --rm | ||
| -v {project}:C:\pillow | ||
| -v C:\cibw:C:\cibw | ||
| -v %CD%\..\venv-test:%CD%\..\venv-test | ||
| -e CI -e GITHUB_ACTIONS | ||
| mcr.microsoft.com/windows/servercore:ltsc2022 | ||
| powershell C:\pillow\.github\workflows\wheels-test.ps1 %CD%\..\venv-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cibuildwheel downloads Python to CIBW_CACHE_PATH (set to C:\cibw above) and sets up a venv for testing at %temp%\cibw-run-<random>\<python-tag>\venv-test. It then runs the test command in the directory %temp%\cibw-run-<random>\<python-tag>\test_cwd.
The CIBW_CACHE_PATH and venv-test directories are bind-mounted here so that the venv can be used from the Docker container. The only way I could find to refer to the venv path was %CD%\..\venv-test; it might be worth opening an issue in cibuildwheel for a better solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could try it!
hugovk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! A quick review.
c10446f to
f55f7ce
Compare
Co-authored-by: Hugo van Kemenade <[email protected]>
f55f7ce to
6fe42bd
Compare
Tests/check_wheel.py
Outdated
| expected_modules = {"pil", "tkinter", "freetype2", "littlecms2", "webp"} | ||
|
|
||
| # tkinter is not available in cibuildwheel installed CPython on Windows | ||
| if not importlib.util.find_spec("tkinter"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect there's a reason, but why is it not simpler to test for tkinter by attempting to import it normally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint error, https://github.com/nulano/Pillow/actions/runs/6998411214/job/19036420674#step:7:21:
Tests/check_wheel.py:10:16: F401 `tkinter` imported but unused; consider using `importlib.util.find_spec` to test for availability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, assert tkinter should silence the warning. We've done that before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I remember seeing that before, but figured the linter suggestion would work as well.
I've pushed a commit to try: import tkinter instead for consistency with other optional imports (the only one I could find with a simple grep was in Image.preinit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the others were replaced with pytest.importorskip (#4436) which doesn't fit here.
|
|
||
| - name: Upload fribidi.dll | ||
| if: "matrix.arch != 'ARM64'" | ||
| uses: actions/upload-artifact@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this was only run for one Python version.
Pillow/.github/workflows/test-windows.yml
Lines 246 to 248 in 63bec07
| - name: Upload fribidi.dll | |
| if: "github.event_name != 'pull_request' && matrix.python-version == 3.11" | |
| uses: actions/upload-artifact@v3 |
Any particular reason it's now being run for every version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now in the wheels.yml workflow which only runs a single job for all Python versions of a single architecture.
Previously it was in test-windows.yml which has a separate job for each Python version.
For #7390:
test-windows.ymlsolibimagequantcan be included there,fribidiartifact to the wheels workflow so that both x86 and x64 versions are provided,CIBW_CONFIG_SETTINGStopyproject.tomland addedbuild-verbosity = 1so that build warnings are visible,wheels-test.shtoTests\check_wheel.pyso that it can also be used on Windows,wheels.yml, usingwinbuild\build_prepare.pyto build dependencies same as before, building withcibuildwheel, and testing in Docker similarly to Check available features in Windows wheels #6847.I've included x86 also for #7443 (comment); there have been several issues opened about installing on win32. Perhaps it is not yet the time to stop providing these.
I've tested the arm64 wheels on an M2 MacBook in a Windows ARM VM using this batch script:
Although the VM did have VC++ redistributable DLLs installed, I checked the list of imported dlls by the pyd files match between arm64 and amd64 with dumpbin.