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

get_requires_for_build_wheel hook is invoked without passing config_settings #12273

Open
1 task done
rgommers opened this issue Sep 11, 2023 · 3 comments
Open
1 task done
Labels
S: needs triage Issues/PRs that need to be triaged type: bug A confirmed bug or unintended behavior

Comments

@rgommers
Copy link

Description

The exact invocation is here:

return backend.get_requires_for_build_wheel()

A few lines below that there's an invocation of get_requires_for_build_editable with the same problem.

PEP 517 clearly states that config_settings must be passes to all hooks, so not doing so looks like a bug to me.

Context: I noticed because I had an optional system dependency in the numpy build and the config-settings flag to disable usage of that flag wasn't doing anything. On inspection of my CI build log, the flag wasn't being passed in, which led me to search the pip code base for all calls to the get_requires_for_build_wheel hook. The missing argument in the call in sdist.py indeed explains the failure I was seeing.

More context: meson-python has a dependency on Linux on patchelf in some cases, and to determine whether installing patchelf from PyPI was necessary, a build configure step could be triggered (see these lines of code). That situation should be rare, which probably explains why no one noticed the missing config_settings argument. That code is now changed, so the problem no longer affects meson-python users.

Expected behavior

All calls to get_requires_for_build_wheel and get_requires_for_build_editable always pass in config_settings.

pip version

23.2.1

Python version

3.11

OS

Linux

How to Reproduce

The actual reproducer was inside a dev build of numpy, which is nontrivial to reproduce. However, I don't think it's necessary to reproduce that particular failure - from the line of code I identified above and PEP 517, it seems clear that this is a bug and that passing in config_settings will fix the bug.

If a reproducer is needed for testing, it'd be better to write a standalone minimal reproducer.

Output

No response

Code of Conduct

@rgommers rgommers added S: needs triage Issues/PRs that need to be triaged type: bug A confirmed bug or unintended behavior labels Sep 11, 2023
@rgommers
Copy link
Author

Ah, while I did search the Pip issue tracker and didn't find anything, I only looked at pypa/build after opening this issue. There is more discussion on the same topic there: pypa/build#264. The tl;dr of that seems to be:

@henryiii
Copy link
Contributor

henryiii commented Sep 14, 2023

From pypa/build#264 (comment) I thought this bug wan't present in pip.

The bug is setuptools injects the contents of config_settings to every command, and unless a --build-option is supported by every command that gets run, it breaks down. -C--build-option=custom_option=True will break unless custom_option is supported by every command, not just bdist_wheel. This bug is hidden by previously build (and pip it seems) passing an empty config_settings to get_requires_for_build_wheel, which calls egg_info.

Fixing it has caused some issues (also see pypa/build#264) for build users due to setuptool's bug, pypa/setuptools#3896, though I'm hoping someone will actually just go and fix the bug now that more people are able to see it. There are a couple of user-facing workarounds, so AFAIK those affected have been able to use the workarounds.

If this bug is present in pip (which does make sense since setuptools doesn't handle custom wheel related options when this is handled correctly according to PEP 517), then maybe a short wait to see if it gets fixed in setuptools now that build exposes it by behaving correctly would be best. It would be nice if the reply to the bug reports could just be "update setuptools" instead of echo -e "\n[bdist_wheel]\ncustom_option=True" >> setup.cfg or setting $DIST_EXTRA_CFG.

I think an author could possibly add these custom options to the other commands (just egg_info?), which would cause them to no longer error, and would be a workaround no worse than a lot of other setuptools workarounds in setup.py's for binary builds. Another author workaround would be to wrap the PEP 517 build hooks and remove config_settings from the get requires hooks. That's literally what build used to do and pip still does. IMO, a quick fix for setuptools would be for it to start ignoring config_settings it it's get requires build hooks.

It is very import to fix soonish, though, as all other backends expect to be able to see all the options for all the hooks. Scikit-build-core, for example, has several that could affect the requirement list. It is clearly a part of the PEP that config_settings is passed to these hooks, and other backends that are designed around the PEP expect it.

PS: There are also author-level workarounds. You can add do-nothing handling to all the commands that see these config-settings. You can also follow https://setuptools.pypa.io/en/latest/build_meta.html#dynamic-build-dependencies-and-other-build-meta-tweaks (also the recommended way to customize according to the PEP) and add the following wrapper:

# _custom_build/backend.py
from setuptools import build_meta as _orig
from setuptools.build_meta import *


def get_requires_for_build_wheel(config_settings=None):
    return _orig.get_requires_for_build_wheel()


def get_requires_for_build_sdist(config_settings=None):
    return _orig.get_requires_for_build_sdist()


def get_requires_for_build_editable(config_settings=None):
    return _orig. get_requires_for_build_editable()
[build-system]
requires = ["setuptools"]
build-backend = "backend"
backend-path = ["_custom_build"]

This literally does what pip is currently doing and build used to do.

@sbidoul
Copy link
Member

sbidoul commented Sep 17, 2023

@rgommers normally config settings should be injected in these methods via the ConfiguredBuildBackendHookCaller class. So it's normal they are not apparent at the call site you point out in the OP. Not sure where the bug could lie, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: needs triage Issues/PRs that need to be triaged type: bug A confirmed bug or unintended behavior
Projects
None yet
Development

No branches or pull requests

4 participants
@rgommers @sbidoul @henryiii and others