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

Specify build backend via pyproject.toml #7190

Closed
wants to merge 12 commits into from

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented May 27, 2023

For #6941.

#7188 added setuptools to the CI installation for Python 3.12, because 3.12 dropped setuptools:

This is actually a bit bigger problem than the CI because users could theoretically have a system that does not have setuptools installed.


With pyproject.toml, we can specify the "build backend". In our case, this is setuptools. (The "build frontend" is what reads this file and does the install, often pip.)

So when installing, the frontend will install the required backend, so we don't need to worry whether users have already installed it. We can also specify a minimum version. I just went with the current latest, and it also seems we can remove the extra PILLOW_VERSION double quotes hack we needed for Windows on Python 3.8.

It's also possible to move more config from setup.cfg to pyproject.toml, but I ran into some problems when moving [options] and [options.extras_require] over, so let's leave those for later. We can also move the tools config, but that can also wait to keep the diff smaller here and the PR focused.


About winbuild/build_prepare.py: it had a direct invocation of setup.py which is a no-go because it assumes setuptools is already installed. So we need pip install instead (reminder of https://blog.ganssle.io/tag/setuptools.html).

I went with --global-option for now, to follow the current approach elsewhere, but that will need changing as part of #7167.

I did hit one blocker though: test-windows.yml builds a wheel, it calls:

winbuild\\build\\build_pillow.cmd --disable-imagequant bdist_wheel

Before, that was handled by this:

        r'"{python_dir}\{python_exe}" setup.py build_ext --vendor-raqm --vendor-fribidi %*',  # noqa: E501

Which wrote out a command that was called like:

2023-04-28T14:37:47.1413330Z D:\a\Pillow\Pillow>"C:\hostedtoolcache\windows\Python\3.8.10\x86\python.exe" setup.py build_ext --vendor-raqm --vendor-fribidi --disable-imagequant bdist_wheel 

Now, those two args --disable-imagequant bdist_wheel each need wrapping as --global-option="--disable-imagequant" --global-option="bdist_wheel"

But this PR does:

        r'"{python_dir}\{python_exe}" -m pip install --upgrade pip',
        r'"{python_dir}\{python_exe}" -m pip install . '
        r'--global-option="--vendor-raqm" '
        r'--global-option="--vendor-fribidi" '
        r'--global-option="%*"',

And those two args were both passed into %* meaning they both were wrapped in a single global-option which doesn't work, so it doesn't build the wheel:

2023-05-27T14:56:57.1800310Z D:\a\Pillow\Pillow>"C:\hostedtoolcache\windows\Python\3.8.10\x86\python.exe" -m pip install . --global-option="--vendor-raqm" --global-option="--vendor-fribidi" --global-option="--disable-imagequant bdist_wheel" 

Any ideas?

Maybe replace the build_pillow.cmd call entirely, and use something like https://pypi.org/project/build/ to call python -m build --wheel?

@aclark4life
Copy link
Member

@hugovk Is there any other reasonable option besides jumping ship to build? Even if you could fix the wheel build with build_pillow.cmd the entire build_prepare.py is hard to follow like setup.py … not sure how much of that can be cleaned up now, but that's an attractive goal at least.

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

Maybe replace the build_pillow.cmd call entirely, and use something like https://pypi.org/project/build/ to call python -m build --wheel?

Does this build a wheel from the previously compiled Pillow without invoking the compiler?
If so, that sounds like a great solution!
(I expect the --disable-imagequant flag will no longer be needed when building the wheel if that is the case.)

Edit to clarify: The first build_pillow.cmd call when building Pillow is needed to make sure the correct compiler is used. The second one is needed because setuptools requires passing the same parameters to avoid rebuilding Pillow before making a wheel. If the second call can be avoided in some way, it would greatly simplify building wheels from an already built and tested build. However, I can't quite understand how build works in the time I have available at the moment, so it is possible I am misunderstanding it.

Note that I have a ton of work this week so I cannot look into this in detail until next week.

winbuild/build_prepare.py Outdated Show resolved Hide resolved
winbuild/build_prepare.py Outdated Show resolved Hide resolved
.github/workflows/test-windows.yml Show resolved Hide resolved
@hugovk
Copy link
Member Author

hugovk commented May 28, 2023

@hugovk Is there any other reasonable option besides jumping ship to build? Even if you could fix the wheel build with build_pillow.cmd the entire build_prepare.py is hard to follow like setup.py … not sure how much of that can be cleaned up now, but that's an attractive goal at least.

Switching to build might be the simpler choice - creating wheels from existing binaries that have already been built using build_prepare.py, rather than calling it a second time.

Edit: actually build does build again (the clue is in the name!), but in isolation:

This will build the package in an isolated environment, generating a source-distribution and wheel in the directory dist/.

Otherwise, we can keep the current build_prepare.py system, it's a matter of getting the existing parameters passed through properly, shouldn't be too hard. I don't have Windows so it's a bit slow to test via CI pushes :)

@hugovk
Copy link
Member Author

hugovk commented May 28, 2023

Maybe replace the build_pillow.cmd call entirely, and use something like pypi.org/project/build to call python -m build --wheel?

Does this build a wheel from the previously compiled Pillow without invoking the compiler? If so, that sounds like a great solution! (I expect the --disable-imagequant flag will no longer be needed when building the wheel if that is the case.)

Edit to clarify: The first build_pillow.cmd call when building Pillow is needed to make sure the correct compiler is used. The second one is needed because setuptools requires passing the same parameters to avoid rebuilding Pillow before making a wheel. If the second call can be avoided in some way, it would greatly simplify building wheels from an already built and tested build. However, I can't quite understand how build works in the time I have available at the moment, so it is possible I am misunderstanding it.

No, it actually does rebuild (hence the name!) but does so in an isolated environment. It's a "build frontend" so it can do a full build, like pip.

We might want to replace build_pillow.cmd with build at some point, but I'm less clear on the full Windows build process so it's out of scope here and we should probably to stick with build_pillow.cmd so it can do the correct compiler selection, and fix passing the parameters.

Note that I have a ton of work this week so I cannot look into this in detail until next week.

Thanks! We're not in a rush for this. The July release will have beta wheels for Python 3.12, it would be nice this in there to get feedback before releasing with full support for 3.12 in the October release. Otherwise, it can likely wait until October.

hugovk and others added 2 commits May 28, 2023 21:14
@hugovk hugovk marked this pull request as ready for review May 29, 2023 11:46
@radarhere
Copy link
Member

We're not in a rush for this. The July release will have beta wheels for Python 3.12, it would be nice this in there to get feedback before releasing with full support for 3.12 in the October release. Otherwise, it can likely wait until October.

The essential part of this - adding pyproject.toml - is part of #7171, which we do plan to release in July.

So I would think this should also be part of that release?

Wrap arguments before passing in documentation
@nulano
Copy link
Contributor

nulano commented Jun 24, 2023

With #7171 this fails to pass the --global-option values to setup.py.
I've created #7230 to use the -C raqm=vendor format instead.

@hugovk hugovk closed this in #7230 Jun 30, 2023
@hugovk hugovk deleted the add-pyproject.toml branch July 5, 2023 20:26
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.

None yet

4 participants