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.

Sign up for GitHub

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

Use --config-settings instead of deprecated --global-option #7171

Merged
merged 5 commits into from
Jun 24, 2023
Merged

Use --config-settings instead of deprecated --global-option #7171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f734d2
Use --config-settings instead of deprecated --global-option
radarhere May 20, 2023
18da2d0
Removed inplace target
radarhere May 20, 2023
8780772
Merge branch 'main' into build
radarhere Jun 4, 2023
2d0b13b
Swapped config key and value
radarhere Jun 4, 2023
38d6386
Do not import internal class
radarhere Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test-mingw.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ jobs:
pushd depends && ./install_extra_test_images.sh && popd

- name: Build Pillow
run: SETUPTOOLS_USE_DISTUTILS="stdlib" CFLAGS="-coverage" python3 -m pip install --global-option="build_ext" .
run: SETUPTOOLS_USE_DISTUTILS="stdlib" CFLAGS="-coverage" python3 -m pip install .
Copy link

Choose a reason for hiding this comment

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

I got to the conclusion that it's also nice to configure coverage through the config setting. Here's an example of the interface I ended up with: https://yarl.aio-libs.org/en/latest/changes/#released-versions

Copy link
Member Author

Choose a reason for hiding this comment

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

We're already providing make install-coverage. I don't see the need to provide yet another way of enabling coverage.

Copy link

Choose a reason for hiding this comment

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

Yeah, I see. Just thought it's less integrated into the Python interfaces, but into tooling of external ecosystems.


- name: Test Pillow
run: |
Expand Down
1 change: 1 addition & 0 deletions MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ graft src
graft depends
graft winbuild
graft docs
graft _custom_build

# build/src control detritus
exclude .appveyor.yml
Expand Down
9 changes: 2 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,26 +46,21 @@ help:
@echo " docserve run an HTTP server on the docs directory"
@echo " html make HTML docs"
@echo " htmlview open the index page built by the html target in your browser"
@echo " inplace make inplace extension"
@echo " install make and install"
@echo " install-coverage make and install with C coverage"
@echo " lint run the lint checks"
@echo " lint-fix run Black and isort to (mostly) fix lint issues"
@echo " release-test run code and package tests before release"
@echo " test run tests on installed Pillow"

.PHONY: inplace
inplace: clean
python3 -m pip install -e --global-option="build_ext" --global-option="--inplace" .

.PHONY: install
install:
python3 -m pip -v install .
python3 selftest.py

.PHONY: install-coverage
install-coverage:
CFLAGS="-coverage -Werror=implicit-function-declaration" python3 -m pip -v install --global-option="build_ext" .
CFLAGS="-coverage -Werror=implicit-function-declaration" python3 -m pip -v install .
python3 selftest.py

.PHONY: debug
Expand All @@ -74,7 +69,7 @@ debug:
# for our stuff, kills optimization, and redirects to dev null so we
# see any build failures.
make clean > /dev/null
CFLAGS='-g -O0' python3 -m pip -v install --global-option="build_ext" . > /dev/null
CFLAGS='-g -O0' python3 -m pip -v install . > /dev/null

.PHONY: release-test
release-test:
Expand Down
56 changes: 56 additions & 0 deletions _custom_build/backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import sys

from setuptools.build_meta import * # noqa: F401, F403
hugovk marked this conversation as resolved.
Show resolved Hide resolved
from setuptools.build_meta import build_wheel

backend_class = build_wheel.__self__.__class__


class _CustomBuildMetaBackend(backend_class):
def run_setup(self, setup_script="setup.py"):
if self.config_settings:

def config_has(key, value):
settings = self.config_settings.get(key)
if settings:
if not isinstance(settings, list):
settings = [settings]
return value in settings

flags = []
for dependency in (
"zlib",
"jpeg",
"tiff",
"freetype",
"raqm",
"lcms",
"webp",
"webpmux",
"jpeg2000",
"imagequant",
"xcb",
):
if config_has(dependency, "enable"):
flags.append("--enable-" + dependency)
elif config_has(dependency, "disable"):
flags.append("--disable-" + dependency)
for dependency in ("raqm", "fribidi"):
if config_has(dependency, "vendor"):
flags.append("--vendor-" + dependency)
if self.config_settings.get("platform-guessing") == "disable":
flags.append("--disable-platform-guessing")
if self.config_settings.get("debug") == "true":
flags.append("--debug")
if flags:
sys.argv = sys.argv[:1] + ["build_ext"] + flags + sys.argv[1:]
return super().run_setup(setup_script)

def build_wheel(
Copy link

Choose a reason for hiding this comment

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

@hugovk @radarhere this omits build_editable() which is almost the same as build_wheel(). You'll likely want to wrap it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've attempted to add build_editable in 290a2d1, but for some reason it is not working properly on Windows: https://github.com/nulano/Pillow/actions/runs/7308594440/job/19915362677#step:25:728

Copy link

Choose a reason for hiding this comment

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

It looks like the build script calls something different from what's called on the main branch: I see "Running command Getting requirements to build editable" vs. "Running command Getting requirements to build wheel". The former is from your log, the latter is from the main branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I added 2446fd8 to test editable installing on GHA. For some reason, it seems the include paths are not passed in properly when building editable on Windows (specifically the OpenJpeg path is found and added for part of the build, but not all of it).

Copy link

Choose a reason for hiding this comment

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

Oh, interesting, I didn't see where it's defined. It's often hard to distinguish outputs of different commands if they are squashed in the same CI step, so I tend to recommend putting each command in a separate step or using the corresponding shell's capabilities to improve such logging. I also recently saw a case where Powershell would proceed to execute commands after the previous ones fail.

Anyway, this may indicate dependency on some weird pre-existing state.

One way of exploring GHA jobs is https://github.com/marketplace/actions/debugging-with-tmate, by the way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have mentioned that it also fails for me locally.

It's the first command in that step that is failing, the second one just runs a simple test.
The path is added in setup.py in the build_extensions method here:

Pillow/setup.py

Lines 687 to 691 in 41e45b5

if best_version and _find_library_file(self, "openjp2"):
# Add the directory to the include path so we can include
# <openjpeg.h> rather than having to cope with the versioned
# include path
_add_directory(self.compiler.include_dirs, best_path, 0)

From the GHA log, it looks almost as if it first does a regular build, then a second build again skipping that method, but I'm not sure why or where to look.

Yes, I have used tmate before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created #7658 for this.

self, wheel_directory, config_settings=None, metadata_directory=None
):
self.config_settings = config_settings
return super().build_wheel(wheel_directory, config_settings, metadata_directory)


build_wheel = _CustomBuildMetaBackend().build_wheel
24 changes: 12 additions & 12 deletions docs/installation.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,40 +380,40 @@ Build Options
using a setting of 1. By default, it uses 4 CPUs, or if 4 are not
available, as many as are present.

* Build flags: ``--disable-zlib``, ``--disable-jpeg``,
``--disable-tiff``, ``--disable-freetype``, ``--disable-raqm``,
``--disable-lcms``, ``--disable-webp``, ``--disable-webpmux``,
``--disable-jpeg2000``, ``--disable-imagequant``, ``--disable-xcb``.
* Config settings: ``-C zlib=disable``, ``-C jpeg=disable``,
``-C tiff=disable``, ``-C freetype=disable``, ``-C raqm=disable``,
``-C lcms=disable``, ``-C webp=disable``, ``-C webpmux=disable``,
``-C jpeg2000=disable``, ``-C imagequant=disable``, ``-C xcb=disable``.
Disable building the corresponding feature even if the development
libraries are present on the building machine.

* Build flags: ``--enable-zlib``, ``--enable-jpeg``,
``--enable-tiff``, ``--enable-freetype``, ``--enable-raqm``,
``--enable-lcms``, ``--enable-webp``, ``--enable-webpmux``,
``--enable-jpeg2000``, ``--enable-imagequant``, ``--enable-xcb``.
* Config settings: ``-C zlib=enable``, ``-C jpeg=enable``,
``-C tiff=enable``, ``-C freetype=enable``, ``-C raqm=enable``,
``-C lcms=enable``, ``-C webp=enable``, ``-C webpmux=enable``,
``-C jpeg2000=enable``, ``-C imagequant=enable``, ``-C xcb=enable``.
Require that the corresponding feature is built. The build will raise
an exception if the libraries are not found. Webpmux (WebP metadata)
relies on WebP support. Tcl and Tk also must be used together.

* Build flags: ``--vendor-raqm``, ``--vendor-fribidi``.
* Config settings: ``-C raqm=vendor``, ``-C fribidi=vendor``.
These flags are used to compile a modified version of libraqm and
a shim that dynamically loads libfribidi at runtime. These are
used to compile the standard Pillow wheels. Compiling libraqm requires
a C99-compliant compiler.

* Build flag: ``--disable-platform-guessing``. Skips all of the
* Build flag: ``-C platform-guessing=disable``. Skips all of the
platform dependent guessing of include and library directories for
automated build systems that configure the proper paths in the
environment variables (e.g. Buildroot).

* Build flag: ``--debug``. Adds a debugging flag to the include and
* Build flag: ``-C debug=true``. Adds a debugging flag to the include and
library search process to dump all paths searched for and found to
stdout.


Sample usage::

python3 -m pip install --upgrade Pillow --global-option="build_ext" --global-option="--enable-[feature]"
python3 -m pip install --upgrade Pillow -C [feature]=enable

Platform Support
----------------
Expand Down
4 changes: 4 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[build-system]
requires = ["setuptools >= 40.8.0", "wheel"]
Copy link

Choose a reason for hiding this comment

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

@radarhere @hugovk wheel shouldn't actually be depended on: pypa/setuptools#3056.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has since been removed in #7248.

Copy link

Choose a reason for hiding this comment

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

Ah, I should've checked. Thanks!

build-backend = "backend"
backend-path = ["_custom_build"]
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ extras =
tests
commands =
make clean
{envpython} -m pip install --global-option="build_ext" --global-option="--inplace" .
{envpython} -m pip install .
{envpython} selftest.py
{envpython} -m pytest -W always {posargs}
allowlist_externals =
Expand Down