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

Fix editable installation with custom build backend and configuration options #7658

Merged
merged 9 commits into from
Mar 30, 2024

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Dec 30, 2023

#7171 added a custom build backend to translate -C feature=disable style flags to the old --disable-feature style flags by prepending the build command (typically bdist_wheel) with a build_ext --disable-feature command which builds the extension modules before being added to a wheel.

It was later noted that this doesn't support editable installs: #7171 (comment)
However, the obvious implementation does not work on Windows: #7171 (comment)

Investigating, I found that editable installs use a private setuptools command editable_wheel: https://github.com/pypa/setuptools/blob/0e2229d7e7d50e4c5f363cb8435a78b03a957dfc/setuptools/build_meta.py#L434
It seems that this command does not reuse the result of the added build_ext step, and instead triggers build_ext a second time. For some reason, the second build_ext does not get initialized properly on Windows and fails to set some of the compiler flags (it does work properly if it is only invoked once, i.e. if no -C feature=disable style flags were provided).

Therefore, in this PR I instead pass the configuration flags before the build command and store them before calling setup(...), then use them to set the defaults in pil_build_ext.initialize_options. There has been some discussion in pypa/setuptools#3896 about how this could be done within setuptools itself, but there does not appear to be a solution yet.

Two issues to consider:

  • Could the option -C conflict with some setuptools options (currently, passing -C to setup.py exits with an error)?
  • Perhaps this should wait until after the release given that it changes setup.py.

@webknjaz
Copy link

  • Could the option -C conflict with some setuptools options (currently, passing -C to setup.py exits with an error)?

One way of solving this could be prefixing the options. But also, you could declare that any underlying setuptools options aren't officially supported unless documented. Then, you'd be in control of how the build is recommended to be invoked. After all, most of the builds are to be made by project's own CI while most of the rest are done by the downstreams. Most of the end-users should avoid using sdist. And the contributors would need to follow the contrib docs.

I've been experimenting with having dedicated changelog sections for the downstream packagers and things that are contributor-facing: https://yarl.aio-libs.org/en/latest/changes/. Maybe, implementing a similar approach could benefit Pillow too.

translate -C feature=disable style flags to the old --disable-feature style

I came to a conclusion in other projects that having true/false values might communicate the intent better, especially since this is a new API. Think about this option?

@nulano
Copy link
Contributor Author

nulano commented Dec 31, 2023

One way of solving this could be prefixing the options. But also, you could declare that any underlying setuptools options aren't officially supported unless documented.

I mean that I chose the prefix -C for passing the configuration from backend.py to setup.py, but it is not clear whether this might be broken by a future setuptools version (i.e. setuptools adding its own -C option at the start of the build command in build_meta). I also considered the prefix --pillow-private-config (or something similar), but wanted to wait for opinions from others since I don't expect this to make it into the 10.2.0 release on Tuesday.

I came to a conclusion in other projects that having true/false values might communicate the intent better, especially since this is a new API. Think about this option?

This API was added in #7171 and released in Pillow 10.0.0: https://pillow.readthedocs.io/en/stable/installation.html#build-options
Perhaps this could be changed, but at least I don't see the benefit in this case given that it has already been included in a release.

Also note that there are three options for some features: disable, enable, vendor.

To be clear, I would consider the flags added to setup.py by this PR to be private and not intended for direct use; installing (and building) with pip is what is recommended.

I've been experimenting with having dedicated changelog sections for the downstream packagers and things that are contributor-facing: https://yarl.aio-libs.org/en/latest/changes/. Maybe, implementing a similar approach could benefit Pillow too.

That looks like a good idea, but again, this PR is not intended to change anything, only to fix editable installation (which has already been useful for me while writing #7659).

@webknjaz
Copy link

That confused me because the popular build frontends (pypa/build and pip) have --config-setting (or -C for short) specifically for supplying arbitrary config settings to the backends.

@nulano
Copy link
Contributor Author

nulano commented Dec 31, 2023

That confused me because the popular build frontends (pypa/build and pip) have --config-setting (or -C for short) specifically for supplying arbitrary config settings to the backends.

Yeah, that is why I chose -C for passing those settings from the custom backend to setuptools-based setup.py, but I do see how it can be confusing.

@nulano nulano marked this pull request as draft December 31, 2023 16:43
@nulano
Copy link
Contributor Author

nulano commented Jan 2, 2024

I've renamed the (private) parameter to --pillow-configuration to avoid confusion or potential conflicts with future versions of setuptools.

@nulano nulano marked this pull request as ready for review January 2, 2024 14:40
* Environment variable: ``MAX_CONCURRENCY=n``. Pillow can use
multiprocessing to build the extension. Setting ``MAX_CONCURRENCY``
sets the number of CPUs to use, or can disable parallel building by
* Config setting: ``-C parallel=n``. Can also be given
Copy link

Choose a reason for hiding this comment

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

Why not use the same naming pattern for settings and env vars?

Copy link
Contributor Author

@nulano nulano Jan 6, 2024

Choose a reason for hiding this comment

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

MAX_CONCURRENCY was added in #721 (almost 10 years ago).

--parallel is a distutils/setuptools flag added in Python 3.5; Pillow support added in 64bce1a (over 5 years ago).

So I think it makes sense to keep those names for consistency.

You can also install Pillow in `editable mode`_::

winbuild\build\build_env.cmd
python.exe -m pip install -v -C raqm=vendor -C fribidi=vendor -e .
Copy link

Choose a reason for hiding this comment

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

I think that for pip install, the config settings should go after the user-requested package. I know that for some options pip attached them to the package install requests that they follow. I think it might do the same for PEP 517 config settings as pip install may have many install requests in the same command. As in pip install pkg-X --config-setting=some=thing pkg-Y --config-setting=some=other-thing. Needs checking, though.

But I still recommend having package settings coupled with the package name/spec this way.

Suggested change
python.exe -m pip install -v -C raqm=vendor -C fribidi=vendor -e .
python.exe -m pip install -v -e . -C raqm=vendor -C fribidi=vendor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find any recommendations about the order in the pip documentation, so I'll take your word for it. Testing locally, I find that either order works the same with a single package. I've created #7686 for your suggestion.

winbuild/build.rst Outdated Show resolved Hide resolved
winbuild/build.rst Outdated Show resolved Hide resolved
@hugovk hugovk merged commit 0b1d0c2 into python-pillow:main Mar 30, 2024
85 checks passed
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