build icon indicating copy to clipboard operation
build copied to clipboard

_build_in_isolated_env fails to pass in config_settings to builder.get_dependencies

Open e2thenegpii opened this issue 5 years ago • 3 comments

https://github.com/pypa/build/blob/2c1da830f214580e8e4b94e4c88345963fa32d2e/src/build/main.py#L58

When using python -m build. The config_settings dictionary is always None in the get_requires_for_build_* callbacks regardless of the options passed on the command line.

The fix is to pass in config_settings dictionary to get_dependencies on the call on the referenced line.

I will likely have the time to add a pull request in a few days.

e2thenegpii avatar Mar 23 '21 03:03 e2thenegpii

This is an oversight but a fortunate one, because fixing it would break passing config settings to setuptools' sdists and wheels :(

layday avatar Mar 23 '21 13:03 layday

Would you mind elaborating how it would break setuptools? Did you run a test suite to come to that conclusion? Looking at the setuptools PEP517 backend at https://github.com/pypa/setuptools/blob/88b8b7807ec3064fec713da83fc0c7860b982536/setuptools/build_meta.py#L152 it seems setuptools is going to ignore any entry that doesn't have the key of "--global-option"

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Tuesday, March 23, 2021 9:24 AM, layday @.***> wrote:

This is an oversight but a fortunatee one, because fixing it would break passing config settings to setuptools' sdists and wheels :(

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

e2thenegpii avatar Mar 23 '21 21:03 e2thenegpii

setuptools only accepts --global-option. This is passed to every hook invoked from build, so that an option that is valid for e.g. build_wheel (python setup.py bdist_wheel) is invalid for get_requires_for_build_wheel (python setup.py egg_info).

layday avatar Mar 23 '21 22:03 layday

setuptools v64.0.0 (11 Aug 2022) added --build-option. Release note: https://setuptools.pypa.io/en/latest/history.html#v64-0-0 The PR added --build-option:https://github.com/pypa/setuptools/pull/3380

ghost avatar Jan 02 '23 04:01 ghost

That's great to hear! For other build backends (scikit-build-core, meson-python, etc), this bug will likely be problematic, and would be good to fix. For example, in scikit-build-core, every config option is settable via config settings, and some of them do affect the dependency step, like setting the min version of CMake.

Think it's time to fix this bug and note how to use it with modern setuptools?

henryiii avatar Jan 03 '23 13:01 henryiii

I don't think it makes a lot of difference that setuptool accepts --build-option - global option just means the option comes before the command and build option comes after it, e.g. python setup.py --global-opts-here build_sdist --build-opts-here. The issue where setuptools will pass all global/build opts to all commands remains.

layday avatar Jan 03 '23 14:01 layday

Oh, okay. Is there a path forward for setuptools to fix this? (like egg_info ignoring options it doesn't support, or setuptool's get_requires_for_build_wheel stripping options it doesn't support) It really is a build bug that I'd like to see eventually fixed. Maybe we could provide an opt in/out for the bugged behavior that sdtputools is relying on?

henryiii avatar Jan 03 '23 16:01 henryiii

I don't think there's been any progress since https://github.com/pypa/setuptools/issues/2491#issuecomment-803684203.

layday avatar Jan 03 '23 19:01 layday

(sparked by the discussion in #627)

At this point, I am starting to think it's better to just do this change so that we do not delay it further, as from conversations with other people, and my personal involvement in developing build backends, this seems like the correct path forward. This is already causing issues with other backends, and I do not want to be forced to provide a more complicated CLI, so changing the behavior to what is most desirable in general is something that as soon as we do it, the better.

To handle cases where this might break things, we'd have to provide a mechanism to temporarily switch to old behavior. Regarding setuptools, given that no development happened there, and so that this change does not get delayed any further, I'd special case it and turn the behavior on by default.

About the mechanism that should be used for this, I think an environment variable would probably be the best, and in order to avoid requiring it to be set for each call, I'd probably make it a list of backends.


Additionally, pinging @abravalheri to see if we can get some more movement on the setuptools side.

TLDR: We are currently only passing --config-settings to the build_* hooks, which is an undesirable general behavior and something we overlooked. We want to pass --config-settings to all hooks, but setuptools cannot handle that, which ended up stalling this issue.

FFY00 avatar Jun 27 '23 13:06 FFY00

I'd probably make it a list of backends.

We don't know what backend is being used, unless you mean just doing a textual match against the setting of build-backend (which I'm still not very fond of). It's perfectly valid to wrap a build backend (it's explicitly allowed/recommended for some cases, and covered in PEP 517, setuptool's docs, and scikit-build's docs), and that shouldn't suddenly cause different behavior due to some special logic looking at the build-backend value.

IMO BUILD_NO_CONFIG_SETTINGS_FOR_REQUIRES=1 or whatever would probably be enough while setuptools works on this; I wonder if we could even provide a nice error message if this exact issue is detected? (Also, older setuptools, especially setuptools 59, will be around for quite a while, so it's not going away in the next version of build or two - but not making it a flag sounds fine to me)

henryiii avatar Jun 27 '23 14:06 henryiii

Additionally, pinging abravalheri to see if we can get some more movement on the setuptools side.

Hi @FFY00, I did some preliminary work on the setuptools.build_meta regarding filtering config_settings per hook. There are 2 main problems there, one is relatively easy to fix (the lack of a list of which parameters we want to expose going forward and which parameters will be considered legacy and supported only as best effort -- personally I don't want to take an unilateral decision about that), the other problem is more profound: the intertwined nature of egg-info and how the design of PEP 517 is not really backward compatible with it...

July and possibly August are going to be a busy months for me, so I cannot promise I will be able to work on this. If anyone in the community would like to give it a go that would also be nice.

abravalheri avatar Jun 30 '23 15:06 abravalheri

@abravalheri thanks for the update, and for having a look at this! Are you planning to also include this in setuptools.build_meta:__legacy__? If not, would it be possible to issue a warning on unknown/unexpected options instead of erroring out?

FFY00 avatar Jun 30 '23 15:06 FFY00

The __legacy__ backend is a subclass of the regular backend and most of the times it also get the updates for free.

I don't think these updates would be incompatible.

abravalheri avatar Jun 30 '23 15:06 abravalheri

We don't know what backend is being used, unless you mean just doing a textual match against the setting of build-backend (which I'm still not very fond of).

Yes.

It's perfectly valid to wrap a build backend (it's explicitly allowed/recommended for some cases, and covered in PEP 517, setuptool's docs, and scikit-build's docs), and that shouldn't suddenly cause different behavior due to some special logic looking at the build-backend value.

It is, but I don't know how to detect such cases.

The main thing to consider here is the UX, IMO. A backend being a setuptools wrapper isn't that relevant for the final user, it's something they may be aware, if the backend advertises it, but often aren't. Many backends also don't even advertise they are a wrapper of setuptools, so the user very few users will know that. Because of this, even though I understand your opinion regarding the different behavior, I don't think it outweighs the cons of the alternatives.

IMO BUILD_NO_CONFIG_SETTINGS_FOR_REQUIRES=1 or whatever would probably be enough while setuptools works on this;

That would create immense pain for downstream packagers, for eg, because it'd require them to update every single package recipe that uses setuptools to set that environment variable (for reference, there are ~1.8k packages on the arch repo alone that use setuptools as the backend...).

I wonder if we could even provide a nice error message if this exact issue is detected?

Definitely. This can even be done for setuptools wrappers if we decide to go with an approach similar to what I proposed. We detect that the backend is likely a setuptools wrapper and give them the envvar ready to be set (eg. PYPA_BUILD_NO_CONFIG_FOR_REQUIRES=(...),my_custom_backend).

Going back to the expectations discussion above, detecting that the backend may be, or is likely to be, a setuptools wrapper and proving a helpful message, like you mention, should mostly mitigate the issue, I think.

FFY00 avatar Jun 30 '23 16:06 FFY00

https://github.com/pypa/build/pull/632 mirrors the programmatic API by allowing users to pass config settings to each hook separately, which I think is an elegant, permanent way out of this impasse. I can think of very few downsides - discoverability is one (we are entering the realm of mini-DSLs) and the pressure it might put on pip to introduce something similar.

layday avatar Jul 01 '23 09:07 layday

Thanks for working on that @layday!

#632 mirrors the programmatic API by allowing users to pass config settings to each hook separately, which I think is an elegant, permanent way out of this impasse. I can think of very few downsides - discoverability is one (we are entering the realm of mini-DSLs) and the pressure it might put on pip to introduce something similar.

Yup, I think the main downside is how it complicates things. In the long term I think it's in our -- and the user's -- best interest to avoid requiring this kind of mechanisms.

Personally, I think trying having some sort of heuristic to detect setuptools and setuptools-wrappers is good enough, and I think it's an acceptable tradeoff to not put out this (mis)feature.

It's probably worth to ping the pip folks, to see what they think. @pradyunsg @uranusjr @pfmoore

FFY00 avatar Jul 06 '23 16:07 FFY00

I am still against any attempt to try to detect what backend is being used. It should be possible for someone to convert a build backend into a local build backend without hidden tool-specific surprises. For example, turning build-backend="setuptools.build_meta" into build-backend = "backend" backend-path = ["_custom_build"] and wrapping it (as described in both PEP 517 and the setuptools docs) shouldn't suddenly cause some distro's packaging to start failing because they were specifying PYPA_BUILD_NO_CONFIG_FOR_REQUIRES="setuptools.build_meta" and now that's different.

This is a breakage that would be hard to track down when it happens, and isn't clearly anyone's fault other than build's for trying to read the build-backend and assume things about it in the first place.

Matching against the packages in requires might be a bit better (honestly, might be a lot better), but it's still wrong, since someone might include setuptools because they need pkg_utils / pkg_resources, or they might be using a backend that wraps setuptools and lists it as an dependency (I assume setuptools-rust does this). If we did go with way, I think this is how I'd want to do it; anything listed in PYPA_BUILD_NO_CONFIG_FOR_REQUIRES would match against the package names in requires.

Personally, I think having an opt-in environment variable (BUILD_NO_CONFIG_SETTINGS_FOR_REQUIRES=1) that can then become a noop in the future would be helpful in a transition period, and could be removed easily in the future - once most installed versions of setuptools handle getting requires correctly, build's behavior would change and that environment variable would do nothing. It's easier to reason about than something trying to match against a build backend, and it's a lot less likely to be applied to all packages by distribution build scripts than something that is supposed to be applied to all packages (but will break in surprising ways for a few of them). Most of the time, you shouldn't hit this, so hopefully only a few packages will need to enable whatever workaround we provide. Most setuptools packages are not using --config-setting(s)/-C yet.

Also, whatever we do needs to be mirrored in Pip. We should not come up with something different then what Pip will have to do. Ideally we use the same envvar, or at least use the same structure.

henryiii avatar Jul 06 '23 16:07 henryiii

Personally, I think trying having some sort of heuristic to detect setuptools and setuptools-wrappers is good enough, and I think it's an acceptable tradeoff to not put out this (mis)feature.

That does mean that setuptools users will be unable to pass config settings to get_requires just as they are now, and the same holds for the proposed env var. Very few other backends utilise config settings - the PR that reignited the discussion cites a package which uses setuptools, and would be excluded by such a heuristic. The vast majority of issues filed against build about config settings are just that - people trying to pass an option to one setuptools command which causes another command to error out. The folks who migrate from setup.py are (vaguely) aware of the correspondence between setuptools commands and hooks, but simply find themselves unable to pass settings to the appropriate command/hook.

Ideally, we'd consign config settings to the annals of history, but while they exist and while they remain useful, enabling power users to do power user-y things with them isn't exactly what I'd categorise as a misfeature. Being able to do something clumsily is - perhaps - better than not being able to do it at all. Hook prefixes provide an answer for "how do I do X with popular backend Z" - backends which don't carry setuptools' baggage won't have to concern themselves with hook prefixes nor will their users.

layday avatar Jul 06 '23 16:07 layday

Very few other backends utilize config settings

This is heavily used in scikit-build-core and meson-python. I'd expect maturin might too - compiled backends are really likely to need configurable settings. Though Scikit-build-core does allow any config setting to be set via environment variable too[^1], so it can be worked around for now (since config-settings support is rather flimsy currently). But the "most correct" way to configure something dynamic feels like config-settings. (Also, meson-python hates environment variables).

Ideally, we'd consign config settings to the annals of history

Config settings aren't used much yet because binary build backends are just now being developed, and supporting them everywhere has taken time (cibuildwheel added support not that long ago), and setuptools makes them very hard to implement and very ugly (IMO) to use (see #638 Edit: pointed at wrong issue originally). The best way to use them with setuptools might actually be to add a local backend! I don't think that means config-settings weren't a good idea, it just means that they've been hard to use and really hard to rely on historically, especially in a setuptools dominated world.

Hook prefixes provide an answer for "how do I do X with popular backend Z" - backends

The biggest issue with hook prefixes is they really feel like they need a PEP - Pip needs to support the same thing we do; any builder really should support them. It also means that : isn't a valid inside a config settings key - I would have thought it's currently supported. And the main reason it's need at the moment seems to be due to a historical setuptools problem - I'd love at least one non-"fix a setuptools bug" reason to add something like that.

[^1]: Scikit-build-core's config system allows pyproject.toml, config-settings, and environment variables to be used for any setting.

henryiii avatar Jul 06 '23 16:07 henryiii

It's probably worth to ping the pip folks, to see what they think. @pradyunsg @uranusjr @pfmoore

I haven't yet had time to read through this whole discussion (I'll do so another day when I have some free time) but I'm a pretty strong -1 on special-casing any backend. Pip certainly won't do that unless I get overridden by the other maintainers.

What pip does right now is accept key=value config settings on the command line, and pass them to the build backend whenever we call a hook. That is, in my view, in line with what the PEP says on the matter. It's certainly true that the PEP didn't go into much detail on the whole issue of config_settings, and there was very little input from backends on what they would like the interface to look like, but unless someone writes a follow-up PEP, this is what we have got to work with.

As I say, I'll try to look at the full thread here sometime in the next few days, but I hope these comments aren't too far off the mark of what you wanted in the meantime.

pfmoore avatar Jul 06 '23 16:07 pfmoore

This is heavily used in scikit-build-core and meson-python.

Fair enough, but perhaps owing to setuptools' relative popularity, most reported issues with config settings appear to concern setuptools. It's a fair point that binary build backends will see more use out of these than pure Python backends have.

Pip needs to support the same thing we do

I'm not entirely sure that they do. If anything, pip would probably be happy to defer user-initiated package builds to build, from what I gather. There'd definitely be pressure on pip to adopt the same scheme - I mention this above - and the fewer differences that exist between the two tools where they overlap, the better - but surely packaging tools are free to innovate to some extent. Stipulating the config setting interface was rebuffed when we last revisited config settings to clarify passing duplicate keys.

It also means that : isn't a valid inside a config settings key - I would have thought it's currently supported.

This isn't accurate FWIW. It won't split on every : - just the first one. To pass a config setting which contains a colon to all hooks, you can use a null prefix: -C:foo:bar=baz.

layday avatar Jul 06 '23 17:07 layday

This is heavily used in scikit-build-core and meson-python.

At least with meson-python it seems one can just bypass pep517 entirely and use the meson infrastructure directly if needed, I ended up having to do that with scipy due to the apparent complete lack of any cross compilation support in meson-python.

I don't think we're using anything with scikit-build-core right now but I wonder if we would need to bypass that pep517 build backend like we are with meson-python when cross compiling somehow? Invoking build systems like meson/cmake indirectly(ie without properly setting up cross compilation environment/configurations) tends to not work correctly.

Very few other backends utilise config settings - the PR that reignited the discussion cites a package which uses setuptools, and would be excluded by such a heuristic. The vast majority of issues filed against build about config settings are just that - people trying to pass an option to one setuptools command which causes another command to error out.

When converting all our setuptools packages from the legacy setup.py based build/install process to pep517 I didn't notice any packages that broke due to passing the config settings to all the hooks. Only the failure to pass config settings seemed to cause build failures. I guess we're just not using the config settings in a way that passing them to all the hooks would break or something?

jameshilliard avatar Jul 06 '23 17:07 jameshilliard

scikit-build-core supports cross compilation for macOS ARM and Pyodide. We also support it for Windows ARM with some small changes (compared to the minimal version) to your CMakeLists.txt.

I didn't notice any packages that broke due to passing the config settings to all the hooks

I think it's pretty specific, it has to be something that breaks the egg info commands.

henryiii avatar Jul 06 '23 18:07 henryiii

setuptools will error if:

  • You pass egg_info-specific options to any hook other than get_requires_for_build_*
  • You pass sdist-specific options to any hook other than than build_sdist
  • You pass bdist_wheel-specific options to any hook other than build_wheel

For example:

$ python setup.py egg_info --help | rg tag-build
  --tag-build (-b)  Specify explicit tag to add to version number
$ python -m build -n -s -C--build-option=--tag-build -C--build-option=1
* Getting build dependencies for sdist...
running egg_info
writing test_setuptools.egg-info/PKG-INFO
writing dependency_links to test_setuptools.egg-info/dependency_links.txt
writing top-level names to test_setuptools.egg-info/top_level.txt
reading manifest file 'test_setuptools.egg-info/SOURCES.txt'
writing manifest file 'test_setuptools.egg-info/SOURCES.txt'
* Building sdist...
usage: _in_process.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: _in_process.py --help [cmd1 cmd2 ...]
   or: _in_process.py --help-commands
   or: _in_process.py cmd --help

error: option --tag-build not recognized

ERROR Backend subprocess exited when trying to invoke build_sdist

This is equivalent to running:

$ python setup.py sdist --tag-build=1
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

error: option --tag-build not recognized

And with #632:

$ python -m build -n -s -Cget_requires_for_build_sdist:--build-option=--tag-build -Cget_requires_for_build_sdist:--build-option=1
[...]
Successfully built test_setuptools-1.0.0.tar.gz

layday avatar Jul 06 '23 18:07 layday

scikit-build-core supports cross compilation for macOS ARM and Pyodide.

That seems to be similar to python-meson, which is also missing generic cross compilation support.

We also support it for Windows ARM with some small changes (compared to the minimal version) to your CMakeLists.txt.

For packages which may use scikit-build-core can we bypass the scikit-build-core pep517 backend safely and just use cmake directly(we have cmake cross compilation support when used directly)?

jameshilliard avatar Jul 06 '23 21:07 jameshilliard

I've read through this issue, and checked the pip code. I believe that what I said about pip's behaviour is accurate. I have no idea why it's not caused any issues with setuptools, maybe no-one is actually using the --config-settings option...

I don't anticipate changing what pip does, even if build decides to do something like special-case setuptools. I can understand the argument for common behaviour, but short of a new PEP defining the required behaviour explicitly, I think we'll just end up debating whose application-defined behaviour should "win", and I don't think that's an argument worth having.

I don't have an opinion on what build should do. You have different users, with different requirements, than pip, and this almost certainly translates to different trade-offs being appropriate.

pfmoore avatar Jul 06 '23 21:07 pfmoore

I think we are both hoping for some progress on the sysconfig proposal @FFY00 is working on. Generic cross-compilation is hard, especially if you need to support manylinux. Any further standardization of cross compilatin would always be appreciated. :)

can we bypass the scikit-build-core pep517 backend

Depends on the design of the package, though I think it's roughly on par with meson-python. Meson can't natively create wheels or packaging metadata like entry points or the version metadata FWIU, so I don't think you are actually getting a proper install if you bypass it. And you don't need to bypass it with scikit-build-core, it will respect related CMake variables and toolchain files, and you can use standard cross compiling tricks for Python. Not really elegant yet, but not horrible. In the near future (and especially if I have examples! Feel free to post some into an issue), I'll try to document (and possibly improve) the cross compiling story. My main issue in the past has been how hard it is to make a proper manylinux wheel with a modern compiler (required for most archs) without the RHEL patched compiler manylinux native images use.

But this is getting off topic here, happy to take an issue in scikit-build-core discussing cross compiling.

henryiii avatar Jul 06 '23 21:07 henryiii

error: option --tag-build not recognized

Hmm, can't you just do something like this(when used in combination with #627) to ensure the option gets passed to the right setuptools command?

python -m build -n -s -C--build-option=egg_info -C--build-option=--tag-build -C--build-option=1

or

python -m build -n -s -C--build-option=egg_info -C--build-option=--tag-build=1

jameshilliard avatar Jul 06 '23 21:07 jameshilliard

You have different users, with different requirements, than pip, and this almost certainly translates to different trade-offs being appropriate.

If build starts supporting custom prefixes for --config-settings, I think there will be pressure on pip to do the same thing eventually. That's why for that solution, it seems like a pep would be in order. I'd like to see the two tools (installer and sdist/wheel creator) be fairly close if possible. Some package managers (most today?) use pip install, not build + installer.

The environment variable workaround probably would be a lot less pressure to support. I'm confused, is this bug present in pip? To me it seems it's only in build. If that's the case, there would be no pressure at all to support a build workaround flag that reenables this bug for backward compatibility. (And if pip has this bug too, then should't it be fixed? @layday, do you know?)

maybe no-one is actually using the --config-settings?

I believe it's really hard to use with setuptools, but as new build tooling is popping up, it's going to be used a lot more. Simple builds don't need it, and historically complicated builds are still being done with setup.py <commands>, because it's really hard to use --config-settings with setuptools. I don't think there's any documentation on how to do it. And people doing it are running into bugs (like either side of this one - missing the config-settings or needing the config-settings to be missing).

Most complex packages (PyTorch, pysocpg, etc) have lots of options. I expect those options are going to be passed via config-settings eventually. Those packages are only just now beginning to try to move off of classic setup.py invocations. I expect this to increase in the Python 3.12 era.

henryiii avatar Jul 06 '23 21:07 henryiii

Hmm, can't you just do something like this(when used in combination with #627) to ensure the option gets passed to the right setuptools command?

~~No, that won't work.~~

And if pip has this bug too, then should't it be fixed? @layday, do you know?

The bug being that config settings are not being passed to get_requires? It seems not:

$ python -m pip wheel --no-deps . -C--build-option=--tag-build -C--build-option=1
Processing /[...]/build/tests/packages/test-setuptools
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: test-setuptools
  Building wheel for test-setuptools (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Building wheel for test-setuptools (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      usage: _in_process.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
         or: _in_process.py --help [cmd1 cmd2 ...]
         or: _in_process.py --help-commands
         or: _in_process.py cmd --help

      error: option --tag-build not recognized
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for test-setuptools
Failed to build test-setuptools
ERROR: Failed to build one or more wheels

layday avatar Jul 06 '23 22:07 layday