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

Setuptools does not pass config_settings through backend #2491

Open
di opened this issue Dec 17, 2020 · 43 comments
Open

Setuptools does not pass config_settings through backend #2491

di opened this issue Dec 17, 2020 · 43 comments

Comments

@di
Copy link
Member

di commented Dec 17, 2020

The build project now has a --config-setting flag to pass options to the backend:

$ python -m build --help
usage: python -m build [-h] [--version] [--sdist] [--wheel] [--outdir dir] [--skip-dependencies]
                       [--no-isolation] [--config-setting CONFIG_SETTING]
                       [srcdir]

- - - >8 - - -

  --config-setting CONFIG_SETTING, -C CONFIG_SETTING
                        pass option to the backend

I wanted to use this to set wheel's --plat-name argument like so:

$ python -m build --wheel --config-setting plat-name=foo

I found that this worked as expected until the settings got to setuptool's _build_with_temp_dir function:

def _build_with_temp_dir(self, setup_command, result_extension,
result_directory, config_settings):
config_settings = self._fix_config(config_settings)
result_directory = os.path.abspath(result_directory)
# Build in a temporary directory, then copy to the target.
os.makedirs(result_directory, exist_ok=True)
with tempfile.TemporaryDirectory(dir=result_directory) as tmp_dist_dir:
sys.argv = (sys.argv[:1] + setup_command +
['--dist-dir', tmp_dist_dir] +
config_settings["--global-option"])

In this example, _build_with_temp_dir gets called with config_settings={'plat-name': 'foo'}, which then gets 'fixed' into config_settings={'plat-name': 'foo', '--global-option': []}, and then setuptools ignores everything in config_settings and only considers --global-option.

It almost seems like --global-option itself could be used for this, but unfortunately it doesn't work as it's value is passed to lots of things that don't accept the arguments I'm hoping to pass:

$ python -m build --wheel --config-setting="--global-option=--plat-name" --config-setting="--global-option=foo" -n
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 --plat-name not recognized
...

I created a commit that "fixes" this for me as a proof of concept: fc95b3b

Possibly related issues:

@jaraco
Copy link
Member

jaraco commented Dec 21, 2020

I could have used something like this too for Setuptools itself to disable post-release tags for #2500 whereas currently it's using this hack. I suspect Setuptools needs to devise a way to map "config settings" to tweak knobs at different scopes in the build process (global, per-command, ...).

@abitrolly
Copy link

@di why not put this into PR?

@di
Copy link
Member Author

di commented Jan 15, 2021

@abitrolly Because I'm not sure it's right. The means by which a frontend passes these arguments to the backend is not well defined.

@abitrolly
Copy link

@di what is missing here https://www.python.org/dev/peps/pep-0517/#config-settings ?

abitrolly added a commit to abitrolly/peps that referenced this issue Jan 16, 2021
Because it is hard to implement in current form

pypa/setuptools#2491 (comment)
@di
Copy link
Member Author

di commented Jan 16, 2021

@abitrolly All these phrases from that section indicate that this is not well defined:

Build backends MAY assign any semantics they like to this dictionary.

Build frontends SHOULD provide some mechanism for users to specify arbitrary string-key/string-value pairs to be placed in this dictionary.

Build frontends MAY also provide arbitrary other mechanisms for users to place entries in this dictionary

@abitrolly
Copy link

@di I proposed some clarifications in python/peps#1766 Not sure where it should go from there.

Normalizing parameters to be passed as "option": "value" without dashes seems to be a good compromise. Because interface to build system is defined as callback functions and not as CLI API, keeping dashes would mean that backend should somehow pass the names through argparse.

@FFY00
Copy link
Member

FFY00 commented Jan 16, 2021

FYI I am also in favor of removing the dashes, I don't think they are needed in PEP 517.

@layday
Copy link
Member

layday commented Mar 18, 2021

Incidentally, global options in setuptools seem to mean something different than they do in pip where they are placed in front of the setuptools command. Here, they are appended to the command. pip users might find the discrepancy surprising.

I'm not sure why arguments to sdist and bdist_wheel should not be funneled through config_settings verbatim - why is --global-option needed at all?

@jaraco
Copy link
Member

jaraco commented Mar 21, 2021

I'm sure the reason why the PEP is non-specific about the format is because it would be difficult for a protocol like that to be specific without being over-constrained for a particular use-case.

I'm not sure why arguments to sdist and bdist_wheel should not be funneled through config_settings verbatim - why is --global-option needed at all?

I imagine that the sdist and bdist_wheel commands have settings that are unique to the command. I also expect there could be settings that are applied globally, independent of any command.

Regardless, what I'd like to see happen is for someone to examine the code and determine all the command-line parameters setuptools currently solicits that would be relevant to a build backend... and determine at what scopes those settings are relevant (globally, per command, other?). From there, we can create a scheme, something simple, maybe namespaced, to accept those parameters through config_settings.

@abitrolly
Copy link

@layday if you want to push this further, I guess the right way it to do what @brettcannon said in python/peps#1766 (comment)

Please discuss any proposed changes at https://discuss.python.org/c/packaging/14 first.

@layday
Copy link
Member

layday commented Dec 29, 2021

I don't know how changing lists to comma-separated value strings in the PEP is going to help here.

@abitrolly
Copy link

@layday because PEP doesn't specify how to pass lists, nobody wants to write this.

@dixonwhitmire
Copy link

Hi @abitrolly so just to clarify for myself and possibly for others following this issue, is the next step here to discuss potential changes in https://discuss.python.org/c/packaging/14 so that the PEP is updated with well defined statements?
Thank you!

@abitrolly
Copy link

@dixonwhitmire yes, that's correct.

@PolyacovYury
Copy link

Just encountered this.
My use-case was to pass --quiet from python -m build call.

First there is an issue with build itself: passing -C--global-option=--quiet once causes an error, since config_settings['--global-option'] becomes a string, and setuptools expects a list. Maybe change _fix_config() to something like:

    def _fix_config(self, config_settings):
        config_settings = config_settings or {}
        config_settings.setdefault('--global-option', [])
        if not isinstance(config_settings['--global-option'], list):
            config_settings['--global-option'] = [config_settings['--global-option']]
        return config_settings

Second issue is that setuptools does NOT conform to https://www.python.org/dev/peps/pep-0517/#config-settings.
--global-option should be inserted in front of the setup_command, and --build-option should go to the end of the argv.
This way, invoking python -m build -C--global-option=--quiet will produce the expected result.

Although, even better would be to adjust build itself, so that instead of -C it has -G and -B (for --global-option and --build-option respectively), but that would be out of scope of this discussion.

@abravalheri
Copy link
Contributor

Hi @PolyacovYury , I think the first part of the problem you described was solved in
#2876, wasn't it?

Regarding the second part:

--global-option should be inserted in front of the setup_command

It is not immediately clear to me that the spec mandates that --global-option should come before setup_command... Is there any specific part of the document or is this something you concluded is necessary for --quiet to work? (I wonder it this order, while working for --quiet might break other options that specific to the sdist or bdist_wheel subcommands).

@layday
Copy link
Member

layday commented Mar 3, 2022

I commented on this upthread:

Incidentally, global options in setuptools seem to mean something different than they do in pip where they are placed in front of the setuptools command. Here, they are appended to the command. pip users might find the discrepancy surprising.


It is not immediately clear to me that the spec mandates that --global-option should come before setup_command [...]

This is all setuptools-specific stuff, the spec has no notion of global options nor does it expect that options will be passed to a CLI.

@abravalheri
Copy link
Contributor

Hi @sbidoul, yes there is still the challenge of what to do with egg_info and sdist.
Right now --build-option will be passed to all commands. If any of the commands do not accept certain flags/options, they will raise an exception.

@sbidoul, @layday, @henryiii do you believe that the best we can do right now is to only pass --build-option to bdist_wheel and ignoring it in any other command?

I don't have experience with these extra parameters, but if you think that this is the best way forward we can craft a PR that does that...

(I am considering that --global-option and --build-option are stop gap solutions and will never be 100% perfect, but that can be implemented immediately... indeed we need a better plan, as it is been discussed in #3896, but that may take more time to design.)

tobiasah added a commit to tobiasah/pycapnp that referenced this issue 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`.
@sbidoul
Copy link
Member

sbidoul commented Oct 2, 2023

@sbidoul, @layday, @henryiii do you believe that the best we can do right now is to only pass --build-option to bdist_wheel and ignoring it in any other command?

I don't have experience with these parameters either, but what I can tell is that pip only passes its --build-option to the bdist_wheel command. So yes, I think what you propose is a good stop gap until a broader solution is designed.

@layday
Copy link
Member

layday commented Oct 2, 2023 via email

tobiasah added a commit to tobiasah/pycapnp that referenced this issue 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`.
@sbidoul
Copy link
Member

sbidoul commented Oct 2, 2023

In my understanding the --global-option and --build-option config settings have been designed to be direct PEP 517 equivalents to the pip --global-option and --build-option flags.

When pip invokes setup.py directly, it passes --global-option values to all commands.
--build-option have only ever been passed by pip to setup.py bdist_wheel (and only when used in the pip wheel command).

So from that point of view it makes sense for the setuptools backend to only pass the --build-option config settings to bdist_wheel command, no? The --global-option settings would continue to be passed to all commands.

With that little change we'd have simple and clear transition path for users when pip drops the legacy setup.py invocations.

@layday
Copy link
Member

layday commented Oct 2, 2023

I don't know why the PEP 517 options in setuptools were modeled on pip, but the main audience here isn't people transitioning from legacy pip installs to isolated pip installs - it's people transitioning from invoking setup.py directly, to build. A --build-option which only works with bdist_wheel is not an adequate replacement.

@sbidoul
Copy link
Member

sbidoul commented Oct 2, 2023

A --build-option which only works with bdist_wheel is not an adequate replacement.

I don't disagree a better mechanism for setuptools config settings is needed (and I personally have no idea what it should be - I'll leave that to setuptools experts), but that should not prevent a short term fix for the --build-option setting, which is clearly broken as it is today, since it crashes the backend.

haata pushed a commit to capnproto/pycapnp that referenced this issue 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`.
@abravalheri
Copy link
Contributor

abravalheri commented Oct 3, 2023

I don't know why the PEP 517 options in setuptools were modeled on pip, but the main audience here isn't people transitioning from legacy pip installs to isolated pip installs - it's people transitioning from invoking setup.py directly, to build. A --build-option which only works with bdist_wheel is not an adequate replacement.

Right now, I don't think we are in a good position even to "paper over the cracks". The situation with egg_info/dist_info "idempotency" (among the multiple direct/indirect calls triggered by PEP 517/PEP 660 hooks in different sub-processes) is difficult to deal with.

Solving this and related problems is one of the major steps in creating a more definitive solution1. So if the most immediate concern is to offer a solution for people migrating from direct running setup.py, I don't think is worth bothering with an intermediate stop-gap solution23 and we just go with #3896.

On the other hand, if the most immediate concern is to offer a solution for compatibility with pip legacy flags, we can easily create a PR that ignores config_settings["--build-option"] for all the PEP 517 hooks except setuptools.build_meta:build_wheel.

Footnotes

  1. I did a summary of current state and challenges here:
    https://github.com/pypa/setuptools/issues/3896#issuecomment-1656714771.

  2. Again, designing a more definitive solution is difficult and can take time. In the discussion in https://github.com/pypa/setuptools/issues/3896#issuecomment-1656714771 and https://github.com/pypa/setuptools/issues/3896#issuecomment-1708438587, I highlighted on how I would go about it, but I invite any member of the community to discuss and contribute.

  3. The best thing people looking for an alternative to python setup.py arguments can do right now is to modify setup.cfg or use the DIST_EXTRA_CONFIG env var.

@sbidoul
Copy link
Member

sbidoul commented Oct 13, 2023

On the other hand, if the most immediate concern is to offer a solution for compatibility with pip legacy flags, we can easily create a PR that ignores config_settings["--build-option"] for all the PEP 517 hooks except setuptools.build_meta:build_wheel.

This may unlock a workaround to #3237. --config-setting=--build-option=--bdist-dir=/tmp/build

Edit: hm, or not, due to the .egg-info directory.

@abravalheri
Copy link
Contributor

abravalheri commented Oct 13, 2023

I was trying to see if we have further agreement on this, but I guess I can prepare a PR for that ... (update: #4079)

Edit: hm, or not, due to the .egg-info directory.

That is the thing... .egg-info gets called implicitly by other commands, so we need a persistent way of setting the parameters between the different independent calls to multiple processes.

Also some packages will simply not work if .egg-info is not on their project root, because they depend on importlib.metadata finding it there... (like setuptools itself - see #3921 (comment) and the following comment by Jason).

abravalheri added a commit to abravalheri/setuptools that referenced this issue Oct 13, 2023
In pypa#2491 (comment)
the discussion seems to lead to the idea that it is better for now
to avoid passing any `--build-option` for commands that are not
`bdist_wheel` in `setuptools.build_meta`.
@abravalheri
Copy link
Contributor

I opened a discussion in #4083 for a more long-term approach.

abravalheri added a commit to abravalheri/setuptools that referenced this issue Nov 20, 2023
In pypa#2491 (comment)
the discussion seems to lead to the idea that it is better for now
to avoid passing any `--build-option` for commands that are not
`bdist_wheel` in `setuptools.build_meta`.
abravalheri added a commit to abravalheri/setuptools that referenced this issue Nov 20, 2023
In pypa#2491 (comment)
the discussion seems to lead to the idea that it is better for now
to avoid passing any `--build-option` for commands that are not
`bdist_wheel` in `setuptools.build_meta`.
MehdiChinoune referenced this issue in msys2/MINGW-packages Jan 6, 2024
@jameshilliard
Copy link
Contributor

that should not prevent a short term fix for the --build-option setting, which is clearly broken as it is today, since it crashes the backend.

It seems that #4079 made --build-option completely unusable for passing flags to commands like build_ext.

So if the most immediate concern is to offer a solution for people migrating from direct running setup.py, I don't think is worth bothering with an intermediate stop-gap solution23 and we just go with #3896.

This is exactly our use case which was completely broken by #4079, I've opened a PR reverting it in #4218 as #4079 seems to clearly be a regression.

@abravalheri
Copy link
Contributor

Hi @jameshilliard, I just pushed the bugfix you provided in v69.1.1, once the CI is done you can see if that works for you. Thank you very much for reporting the problem and working on the fix.

/fyi @sbidoul, the change in #4217 should not introduce problems for pip, but we never known... Please let me know if you find problems.

github-merge-queue bot pushed a commit to openvinotoolkit/openvino that referenced this issue Oct 23, 2024
… OV wheel (#27190)

### Details:
- Direct `setup.py` calls are deprecated and using `build` package
instead is recommended
([source](https://packaging.python.org/en/latest/discussions/setup-py-deprecated/))
- PR simplifies wheel building by using `python -m build` for all pip
versions
- A new build dependency has been added since `build` is not supplied
with Python
- `setuptools` (our current build backend) is not integrated well with
`config_settings`, which leads to an unfriendly looking workaround for
passing build settings (more details in
pypa/setuptools#2491)
- Migrations for other wheels will be delivered in separate PRs as they
have a different scope (for example `openvino_dev` also requires
migration from using `pkg_resources` package)

### Tickets:
 - CVS-155190
CuriousPanCake pushed a commit to CuriousPanCake/openvino that referenced this issue Nov 6, 2024
… OV wheel (openvinotoolkit#27190)

### Details:
- Direct `setup.py` calls are deprecated and using `build` package
instead is recommended
([source](https://packaging.python.org/en/latest/discussions/setup-py-deprecated/))
- PR simplifies wheel building by using `python -m build` for all pip
versions
- A new build dependency has been added since `build` is not supplied
with Python
- `setuptools` (our current build backend) is not integrated well with
`config_settings`, which leads to an unfriendly looking workaround for
passing build settings (more details in
pypa/setuptools#2491)
- Migrations for other wheels will be delivered in separate PRs as they
have a different scope (for example `openvino_dev` also requires
migration from using `pkg_resources` package)

### Tickets:
 - CVS-155190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests