pypa / build

A simple, correct Python build frontend
https://build.pypa.io
MIT License
740 stars 121 forks source link

_build_in_isolated_env fails to pass in config_settings to builder.get_dependencies #264

Closed e2thenegpii closed 1 year ago

e2thenegpii commented 3 years ago

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_forbuild* 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.

layday commented 3 years ago

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

e2thenegpii commented 3 years ago

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.

layday commented 3 years ago

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

ghost commented 1 year ago

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

henryiii commented 1 year ago

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?

layday commented 1 year ago

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.

henryiii commented 1 year ago

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?

layday commented 1 year ago

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

FFY00 commented 1 year ago

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

henryiii commented 1 year ago

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)

abravalheri commented 1 year ago

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.

FFY00 commented 1 year ago

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

abravalheri commented 1 year ago

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.

FFY00 commented 1 year ago

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.

layday commented 1 year ago

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.

FFY00 commented 1 year ago

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

henryiii commented 1 year ago

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.

layday commented 1 year ago

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.

henryiii commented 1 year ago

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.

pfmoore commented 1 year ago

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.

layday commented 1 year ago

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.

jameshilliard commented 1 year ago

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?

henryiii commented 1 year ago

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.

layday commented 1 year ago

setuptools will error if:

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
jameshilliard commented 1 year ago

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

pfmoore commented 1 year ago

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.

henryiii commented 1 year ago

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.

jameshilliard commented 1 year ago

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
henryiii commented 1 year ago

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.

layday commented 1 year ago

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
henryiii commented 1 year ago

@jameshilliard Yes, I tested both of your suggestions, they both work.

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

You can retry this yourself (a little messy since this wan't designed to do this, but it happened to be easy to whip up a recipe quickly since I work on it):

docker run --rm -it alpine
apk add python3 git
python3 -m ensurepip
python3 -m pip install pipx build wheel setuptools_scm
pipx install copier
pipx inject copier copier-templates-extensions "pydantic<2"
export "PATH=/root/.local/bin:$PATH"
export SETUPTOOLS_SCM_PRETEND_VERSION=1.0.0
copier copy gh:scientific-python/cookie test_setuptools --UNSAFE --defaults --data=project_name=test_setuptools --data=org=org --data=backend=setuptools --data=full_name="My Name" --data=email=me@email.com --data=license=BSD
cd test_setuptools

And, to answer my own question, this absolutely is correct in pip (meaning it blows up as it should given setuptool's current behavior):

$ python -m pip wheel --no-build-isolation -C--build-option=--tag-build -C--build-option=1 .
... fails
error: option --tag-build not recognized
$ python -m pip wheel --no-build-isolation -C--build-option=egg_info -C--build-option=--tag-build=1 .
... passes

So fixing this will bring us inline with pip, and there's a workaround for people stuck. I'm in favor of #627 as is.

pfmoore commented 1 year ago

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.

Agreed. I'd oppose adding anything like that to pip without a PEP supporting it, if only because a PEP would have to establish that the need is significant enough to justify the complexity.

I'm confused, is this bug present in pip?

I haven't seen any reports of this (or any other) bug with our config-settings implementation. That says to me that either it's not an issue in practice, or no-one is using the feature so they haven't encountered it yet. I'm also somewhat confused, as the bug is being presented here as significant and widespread, and yet we've had no evidence of it with pip.

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.

To me, that's an argument that setuptools is going to be under a lot more pressure from their users to improve their approach to using config settings. I don't view it as pressure on other tools to implement workarounds for the fact that setuptools hasn't addressed the issue yet. Yes, that's rather a black and white viewpoint, but we all have more work than we have people to do it, and we have to prioritise.

I assume that new build tools will work in a way that fits with the existing model (even if only by the simple expedient of hooks ignoring all settings except the ones that apply to them). So unless the KEY=VALUE model has demonstrable flaws (which absolutely would need a new PEP to address) I see this as largely an issue for older tools like setuptools with backward compatibility concerns making progress harder.

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.

That sounds likely. I wonder if there's a need here for a new build backend that is essentially a setuptools wrapper that does nothing more than accept config settings, translate them and forward the hook calls onto setuptools. I don't know how practical that is, but it's certainly a potential avenue for projects needing to manage the transition.

layday commented 1 year ago

@jameshilliard Yes, I tested both of your suggestions, they both work.

I wasn't aware you could chain commands like that. What does python setup.py sdist --abc egg_info --def even mean? Which one is going to run first? Does the output of egg_info completely invalidate get_requires?

jameshilliard commented 1 year ago

Yes, I tested both of your suggestions, they both work.

Yeah, it appeared to work for me when I tested, I just wasn't sure if I had missed some issue here, some of our packages use this trick with build_ext in my pep517 setuptools migration patch and were doing something similar for our setup.py based builds.

I wasn't aware you could chain commands like that. What does python setup.py sdist --abc egg_info --def even mean? Which one is going to run first? Does the output of egg_info completely invalidate get_requires?

I'm not entirely sure, it may be undocumented, I think how it works is that you can pass command specific args to any setup.py command and they get applied only to the requested stage, for example we pass a build_ext option to our setup.py build and install stages currently for psycopg2.

layday commented 1 year ago

So then you'll be calling egg_info over and over again (once for each get_requires, build_sdist and build_wheel), without knowing exactly what it's gonna do. And the get_requires invocation is gonna have a reduplicated egg_info.

If it works, it works... I guess.

henryiii commented 1 year ago

Fixing this in setuptools is better. But this workaround is already required to use pip, so let’s fix the bug and let setuptools work on better support. (You can also add a custom backend to process config settings if it’s your package).

build declares it is a “correct” builder. This is just a build-only bug that hurts most build backends (including setuptools), and has a workaround that’s already being used for pip, since pip doesn’t have the same bug.

jameshilliard commented 1 year ago

So then you'll be calling egg_info over and over again (once for each get_requires, build_sdist and build_wheel), without knowing exactly what it's gonna do. And the get_requires invocation is gonna have a reduplicated egg_info.

I'm assuming setuptools does filtering based on the egg_info argument before using the appended config options internally. I think when doing this with a setup.py based build we sometimes needed to pass the build_ext args to both build and install invocations as otherwise the install stage would rerun the build_ext stage without the desired args when doing an install as build_ext internally is a dependency of the build and install stages within setuptools or something like that.

So passing the egg_info/build_ext or whatever argument to all the hooks appears to be what we need to get the correct behavior in any case.

If it works, it works... I guess.

Yeah, I was a bit confused why the invalid egg_info commands were even an issue since I had assumed this trick was the expected way to pass command specific args to setuptools.

Fixing this in setuptools is better.

Does anything even need to be fixed in setuptools, aside from maybe the documentation?

layday commented 1 year ago

Addressed by #627.

gentlegiantJGC commented 1 year ago

I am a little lost here. Our build script needs a boolean input to enable some behaviour in the bdist_wheel stage in certain contexts.

In build 0.x I had a custom bdist_wheel class and passed an argument to the setup.py through build with the following command.

python -m build -C--build-option=--find-libs=True

In build 1.0 this now errors when arguments are parsed in _in_process.py

Could someone point me to some documentation or an example on how to achieve this in the new version.

henryiii commented 1 year ago

This is a bug in setuptools. Ideally it should be fixed there. See / upvote https://github.com/pypa/setuptools/issues/3896.

There are two options. Ons is you can try using -C--build-option=bdist_wheel -C--build-option=--find-libs=True. That might or might not work, it seems like bdist_wheel might not work with this hack like egg_info did. The other way, the correct way, is to do one of these:

export DIST_EXTRA_CONFIG=...  # e.g. /tmp/build-opts.cfg
echo -e "[bdist_wheel]\nfind-libs=True" > $DIST_EXTRA_CONFIG
python -m build

Or,

echo -e "\n[bdist_wheel]\nfind-libs=True" >> setup.cfg
python -m build

(Please note that the first one assumes you are not already setting $DIST_EXTRA_CONFIG - and cibuildwheel cross compiling to Windows ARM does set it)

gentlegiantJGC commented 1 year ago

I tried this -C--build-option=bdist_wheel -C--build-option=--find-libs=True It does work but it makes bdist_wheel run multiple times.

henryiii commented 1 year ago

Okay, great (and yes it does do that, so a pretty bad workaround).

I think another workaround at the package level could be adding the custom options to the other commands (just egg_info, I think?), 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 - see https://setuptools.pypa.io/en/latest/build_meta.html#dynamic-build-dependencies-and-other-build-meta-tweaks. 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.

That would look like this:

# _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"]
gentlegiantJGC commented 1 year ago

I have just tried using an environment variable and that seems to work and sidesteps this mess. I thought I tried this before without luck.

jameshilliard commented 9 months ago

I'm not entirely sure, it may be undocumented, I think how it works is that you can pass command specific args to any setup.py command and they get applied only to the requested stage, for example we pass a build_ext option to our setup.py build and install stages currently for psycopg2.

Looks like setuptools accidentially broken this in #4079, I have a PR reverting that in #4218