Skip to content

Conversation

freakboy3742
Copy link
Contributor

@freakboy3742 freakboy3742 commented Jun 21, 2025

Changes proposed in this pull request:

  • Upgrades cibuildwheel to 3.0
  • Adds a cibuildwheel configuration to produce arm64-iphonesimulator, arm64-iphoneos and x86_64-iphonesimulator wheels
  • Adds changes to the wheel dependency build scripts to support building for iOS

There are two notable/controversial features of this patch.

Firstly, it moves the "check" tests to a separate checks directory. This is necessary because iOS has to use a single python -m pytest invocation to run the test suite, and it isn't possible (or, at least, I can't work out how to get it to work) to run all the test_* tests in the Tests folder and the Tests/check_wheel.py test with a single invocation, because the two targets overlap.

Secondly, it provides patches for brotli and libwebp. These projects are both managed by Google, and contributing code upstream is complicated because of their CLA process. Until those patches have been contributed, applied, and a release made, these patches are required to build on iOS.

@radarhere
Copy link
Member

These projects are both managed by Google, and contributing code upstream is complicated because of their CLA process.

But this is something that you're planning to do, yes? I'm not personally opposed to patches, but it would be ideal if they were temporary, and I just want to know that the ball is rolling in that direction.

Although, #8743 (comment)

I'm not a huge fan of including patch files in repos, I have memories of diffing patch files after an upstream has shifted, which wasn't much fun

@freakboy3742
Copy link
Contributor Author

These projects are both managed by Google, and contributing code upstream is complicated because of their CLA process.

But this is something that you're planning to do, yes? I'm not personally opposed to patches, but it would be ideal if they were temporary, and I just want to know that the ball is rolling in that direction.

That's my intention, yes. The current problem is that I need my employer to sign off on me signing Google's CLA, and that process is proving complicated. They don't have any problem with me contributing code to Open Source projects; but they're wary of signing a big contributor agreement with Google.

Although, #8743 (comment)

I'm not a huge fan of including patch files in repos, I have memories of diffing patch files after an upstream has shifted, which wasn't much fun

You won't get any argument from me on this one. If I could see another option, I'd take it. The only thing I'd say in defence of these particular patches is that they're relatively small modifications to the build system of the respective projects; and neither project releases updates very often (Brotli's last two releases were in August 2023 and August 2020; libwebp in Dec 2024 and Apr 2024)

freakboy3742 and others added 2 commits June 21, 2025 17:58
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere
Copy link
Member

At some point in working on this, you may start scratching your head and wonder where these intermittent TIFF errors are coming from. This is a problem in Pillow main, and #9002 is a possible soluton.

@freakboy3742
Copy link
Contributor Author

Thanks for the heads up on that one. I'm also trying to work out why the iOS builds that work fine locally are breaking in CI... 🤔

@radarhere
Copy link
Member

Upgrades cibuildwheel to 3.0

As an aside, that isn't done in this PR. It is was already done in #9010

@freakboy3742
Copy link
Contributor Author

The CI failures at this point seem to fall into two categories:

  1. Coverage failures that... don't reflect the changes that have been made - they're picking up coverage gaps in test suite skip definitions... but I can't see any #pragma: no cover definitions on existing test skips. I'm not sure what (if anything) I can/should do for those.
  2. The TIFF memory leak issue (cf Revert "Fix memory leak in TiffEncode" #8992 / Fix libtiff cleanup #9002).

The iOS wheels, however, are building and passing tests.

Add some minor simplifications and removes som changes that aren't strictly necessary.
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@hugovk hugovk added the Build label Jun 23, 2025
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.

Thank you!

@radarhere
Copy link
Member

That's my intention, yes. The current problem is that I need my employer to sign off on me signing Google's CLA, and that process is proving complicated. They don't have any problem with me contributing code to Open Source projects; but they're wary of signing a big contributor agreement with Google.

Would it be helpful if I created a PR with your patch at https://github.com/google/brotli? I would just need you to comment with an explanation and answer any questions.

freakboy3742 and others added 2 commits June 27, 2025 22:15
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Remove pyproject.toml from test-sources, plus tweaks to script syntax and release note.
@freakboy3742
Copy link
Contributor Author

Looks like everything is back to passing, except for the stray coverage failure which is a false positive, AFAICT.

@radarhere
Copy link
Member

In our Windows wheels in main at the moment, we only test NumPy on AMD64

if ("$venv" -like "*\cibw-run-*-win_amd64\*") {
& $venv\Scripts\$python -m pip install numpy
}

This PR installs it on Windows x86. However, NumPy is not detected, and a warning is raised - https://github.com/python-pillow/Pillow/actions/runs/15938772420/job/44963636446?pr=9030#step:7:14090

  Tests\test_numpy.py:16
    C:\pillow\Tests\test_numpy.py:16: PytestDeprecationWarning: 
    Module 'numpy' was found, but when imported by pytest it raised:
        ImportError('Error importing numpy: you should not try to import numpy from\n        its source directory; please exit the numpy source tree, and relaunch\n        your python interpreter from there.')
    In pytest 9.1 this warning will become an error by default.
    You can fix the underlying problem, or alternatively overwrite this behavior and silence this warning by passing exc_type=ImportError explicitly.
    See https://docs.pytest.org/en/stable/deprecations.html#pytest-importorskip-default-behavior-regarding-importerror
      numpy = pytest.importorskip("numpy", reason="NumPy not installed")

@freakboy3742
Copy link
Contributor Author

With the win32 fix (thanks @radarhere) we're back to a green board - even coverage is happy.

@radarhere radarhere merged commit da10ed1 into python-pillow:main Jun 30, 2025
75 checks passed
@aclark4life
Copy link
Member

Woo hoo!! Thanks @freakboy3742 , @radarhere , all! Excited to see this.

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.

4 participants