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

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

Merged
merged 5 commits into from
Jun 24, 2023

Conversation

radarhere
Copy link
Member

Resolves #7167

https://github.com/python-pillow/Pillow/actions/runs/4999985216/jobs/8956875899#step:7:23

DEPRECATION: --build-option and --global-option are deprecated. pip 23.3 will enforce this behaviour change. A possible replacement is to use --config-settings. Discussion can be found at pypa/pip#11859

--config-settings is described at https://peps.python.org/pep-0517/#config-settings

So I think our build options should change from

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

to

python3 -m pip install --upgrade Pillow -C enable=[feature]

However, setuptools does not currently pass the config settings through to the backend - pypa/setuptools#2491

Inspired by https://setuptools.pypa.io/en/latest/build_meta.html#dynamic-build-dependencies-and-other-build-meta-tweaks, my suggestion is to instead have a custom backend that translates the config-settings into a form that setuptools will understand. This should work until setuptools resolve their issue.

For pyproject.toml, I used the fallback setting for the require value - https://pip.pypa.io/en/stable/reference/build-system/pyproject-toml/#fallback-behaviour

_custom_build/backend.py Show resolved Hide resolved
_custom_build/backend.py Outdated Show resolved Hide resolved
@nulano
Copy link
Contributor

nulano commented Jun 3, 2023

Would it be clearer to use freetype=enable instead of enable=freetype?

@radarhere
Copy link
Member Author

Ok, sure, I've swapped the order.

@hugovk
Copy link
Member

hugovk commented Jun 14, 2023

Would it be clearer to use freetype=enable instead of enable=freetype?

I don't mind too much either way, but will note we're flipping the order from the current: --disable-zlib to -C zlib=disable.

Trying to think of precedent, pip install --help has:

  --use-feature <feature>     Enable new functionality, that may be backward incompatible.
  --use-deprecated <feature>  Enable deprecated functionality, that will be removed in the future.

Which is more like the existing, but is a bit different. Are there any more direct comparisons?

@hugovk hugovk added this to the 10.0.0 milestone Jun 23, 2023
@radarhere
Copy link
Member Author

I wasn't able to find any comparisons.

Just for my personal thoughts, I think freetype=enable is better than enable=freetype because

  • enable=freetype might make people think there should only be one enable argument, which wouldn't have been the plan
  • it more clearly implies that you shouldn't try enabling and disabling freetype simultaneously

@hugovk
Copy link
Member

hugovk commented Jun 24, 2023

Fair enough, thank you!

@hugovk hugovk merged commit c482634 into python-pillow:main Jun 24, 2023
@radarhere radarhere deleted the build branch June 24, 2023 09:43
janEbert added a commit to janEbert/apex that referenced this pull request Jul 6, 2023
This implements a custom build backend, inspired by [the solution used
by Pillow](python-pillow/Pillow#7171).

With this change, argument flags now need to be specified as key-value
pairs instead of key-only as `--config-settings`. Every argument can
either be left unspecified or have a value that is either "enabled" or
"disabled".

For example:

```
--config-settings --global-option=--cpp_ext
--global-option --cpp_ext
```

becomes

```
--config-settings cpp_ext=enabled
```

The setuptools version requirement of 40.8.0 was also adopted from
Pillow. It is the version that introduced the `build_meta:__legacy__`
backend.
janEbert added a commit to janEbert/apex that referenced this pull request Jul 6, 2023
This implements a custom build backend, inspired by [the solution used
by Pillow](python-pillow/Pillow#7171).

The setuptools version requirement of 40.8.0 was also adopted from
Pillow. It is the version that introduced the `build_meta:__legacy__`
backend.
tobiasah added a commit to tobiasah/pycapnp that referenced this pull request Oct 2, 2023
This implements a custom build backend, inspired by the
[solution used by Pillow.](python-pillow/Pillow#7171)

install-option is deprecated and was removed with pip 23.1. The
comonly accepted solution seems to be to define a custom build
backend for now
pypa/setuptools#2491 (comment)

This commit changes the usage from `--install-option` to `--config-settings`.
tobiasah added a commit to tobiasah/pycapnp that referenced this pull request Oct 2, 2023
This implements a custom build backend, inspired by the
[solution used by Pillow.](python-pillow/Pillow#7171)

install-option is deprecated and was removed with pip 23.1. The
comonly accepted solution seems to be to define a custom build
backend for now
pypa/setuptools#2491 (comment)

This commit changes the usage from `--install-option` to `--config-settings`.
haata pushed a commit to capnproto/pycapnp that referenced this pull request Oct 3, 2023
This implements a custom build backend, inspired by the
[solution used by Pillow.](python-pillow/Pillow#7171)

install-option is deprecated and was removed with pip 23.1. The
comonly accepted solution seems to be to define a custom build
backend for now
pypa/setuptools#2491 (comment)

This commit changes the usage from `--install-option` to `--config-settings`.
@@ -0,0 +1,4 @@
[build-system]
requires = ["setuptools >= 40.8.0", "wheel"]

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.

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!

sys.argv = sys.argv[:1] + ["build_ext"] + flags + sys.argv[1:]
return super().run_setup(setup_script)

def build_wheel(

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

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).

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.

@@ -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 .

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.

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.

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.

--global-option is deprecated
4 participants