-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
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.
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? |
I mean that I chose the prefix
This API was added in #7171 and released in Pillow 10.0.0: https://pillow.readthedocs.io/en/stable/installation.html#build-options Also note that there are three options for some features: To be clear, I would consider the flags added to
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). |
That confused me because the popular build frontends (pypa/build and pip) have |
Yeah, that is why I chose |
I've renamed the (private) parameter to |
docs/installation.rst
Outdated
* 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 |
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.
Why not use the same naming pattern for settings and env vars?
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.
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 . |
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 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.
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 |
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 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.
Co-authored-by: Andrew Murray <[email protected]>
#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 (typicallybdist_wheel
) with abuild_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#L434It seems that this command does not reuse the result of the added
build_ext
step, and instead triggersbuild_ext
a second time. For some reason, the secondbuild_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 inpil_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
tosetup.py
exits with an error)?Perhaps this should wait until after the release given that it changessetup.py
.