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

--config-settings recognizes only the last option #11681

Closed
1 task done
ghost opened this issue Jan 1, 2023 · 22 comments · Fixed by #11853
Closed
1 task done

--config-settings recognizes only the last option #11681

ghost opened this issue Jan 1, 2023 · 22 comments · Fixed by #11853
Labels
PEP implementation Involves some PEP state: awaiting PR Feature discussed, PR is needed type: bug A confirmed bug or unintended behavior

Comments

@ghost
Copy link

ghost commented Jan 1, 2023

Description

The doc said:

The user can supply configuration settings using the --config-settings command line option (which can be supplied multiple times, in order to specify multiple settings).

If setting multiple times like this:

pip wheel --config-settings="--build-option=--cffi" --config-settings="--build-option=--avx2" -v .

Only the last option is passed to backend: {'--build-option': '--avx2'}

It seems these code let the last option override the previous options:

if dest is None:
dest = {}
setattr(parser.values, option.dest, dest)
dest[key] = val

Expected behavior

All options are passed to backend: {'--build-option': ['--cffi', '--avx2']}

Modify the code like this:

    if dest is None:
        dest = {key: []}
        setattr(parser.values, option.dest, dest)
    dest[key].append(val)

pip version

22.3.1

Python version

3.10

OS

Windows 11

How to Reproduce

  1. define a custom PEP-517 hook get_requires_for_build_wheel
  2. let the hook print the config_settings argument
    def get_requires_for_build_wheel(config_settings=None):
        print('config_settings argument:', config_settings)
        return []

Output

No response

Code of Conduct

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

ghost commented Jan 1, 2023

@pfmoore the author of the commit cdeb8f9

@ghost ghost changed the title --config-settings bug --config-settings recognizes only recognizes the last option Jan 2, 2023
@ghost ghost changed the title --config-settings recognizes only recognizes the last option --config-settings recognizes only the last option Jan 2, 2023
@ghost
Copy link
Author

ghost commented Jan 7, 2023

This command solves the problem.

pip wheel --config-settings="--build-option=--cffi --avx2" -v .

@ghost ghost closed this as completed Jan 7, 2023
@layday
Copy link
Member

layday commented Jan 7, 2023

I think there's a genuine bug here. Does your workaround work for all backends or does it just happen to work for setuptools because it's splitting (shlex.split) command-line arguments on its side? IOW, how does a command-line user construct a list of config setting values using pip?

@ghost
Copy link
Author

ghost commented Jan 7, 2023

I think there's a genuine bug here.

Yes, the behavior is different from the document, reopen the issue.

My workaround works for setuptools backend.

@ghost ghost reopened this Jan 7, 2023
@pfmoore
Copy link
Member

pfmoore commented Jan 7, 2023

IOW, how does a command-line user construct a list of config setting values using pip?

Note the following in the config-settings documentation:

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

Specifically, there's no requirement on frontends to provide a mechanism for users to specify anything other than a string value.

The pip example in the PEP is just an example of how pip might implement --config-settings, but it's not actually what we did. I'm not against the idea of allowing something like what is suggested here, but we need to be cautious. It's not pip's responsibility to define the semantics of config_settings, that's for backends, so what should happen is for backend maintainers to agree on what should be allowed as valid config settings, beyond the minimal "string key, string value" case defined by PEP 517. Once there's an agreed "extended standard", then pip can implement it.

For example, what if a user wants to pass a 1-element list to a backend? How would they do that? We can't mandate that backends have to treat a string value the same as a 1-element list containing that value. When I implemented the feature, I toyed with the idea of allowing JSON-encoded non-string values somehow. But supplying such values on the command line rapidly turns into a quoting nightmare, so I backed off and stuck with the minimum required by the standard.

Setuptools currently accepts lists by splitting. Maybe all backends should do that? I don't know, and it's not really my choice. It would be a lot simpler for frontends if that was the decision, of course.

Having said all of the above, I'm not necessarily against someone trying to implement something more complex. But if you do, be aware that any such behaviour will be very much pip-specific, and you can't assume that other frontends (such as build, or conda, or whatever) will behave the same, or indeed will go beyond the string value approach covered by the PEP which pip currently uses.

@layday
Copy link
Member

layday commented Jan 7, 2023

My chief concern is that the homonymous option in build does behave differently. Somebody coming from pip to build, or from build to pip, as I assume has happened here, will find the discrepancy between the two tools surprising. I don't have a preference, but I think we shouldn't diverge without good reason. That could very well mean that build adopts pip's model for parsing config settings.

@pfmoore
Copy link
Member

pfmoore commented Jan 7, 2023

Good point. IMO, sticking with what's standardised is the correct approach here, which probably means string values only for now. If more complex values are important, then I think the standard needs updating to define the minimal set of types frontends should support. For instance, the build API is a Python API, so there's no reason a backend couldn't choose to interpret strings and numbers differently.

@rgommers
Copy link

rgommers commented Jan 7, 2023

We ran also into this issue in meson-python (xref mesonbuild/meson-python#235) and scipy (xref scipy/scipy#17736 (comment)). cibuildwheel ran into it too (xref pypa/cibuildwheel#1227 (comment)).

pypa/build allows using the same key more than once, and merges the result into a list of strings. It would be very useful for pip and pypa/build to have the same behavior here, and what pypa/build does seems reasonable, and I think also matches the intent of PEP 517 (given the example linked above does match).

Somebody coming from pip to build, or from build to pip, as I assume has happened here, will find the discrepancy between the two tools surprising.

+1 - also timely given the "unification of interfaces would be really nice" discussion happening on Discourse.

If more complex values are important, then I think the standard needs updating to define the minimal set of types frontends should support

I don't think there is anything in PEP 517 about this case either way, aside from the example which explicitly shows that "key1=value1" ... "key1=value2" gets merged into "key1" = ["value1", "value2"] when calling the build backend hook. So I'd suggest following that example. If you want a clarification in writing @pfmoore, where should it go? Writing a whole new PEP just to say "if keys are duplicate, merge them" seems a bit much.

@pfmoore
Copy link
Member

pfmoore commented Jan 7, 2023

Agreed, I'd be happy with a simple PR to change the spec. Unfortunately, it looks like no-one has migrated PEP 517 to https://packaging.python.org/en/latest/specifications/ yet, so it would have to be a PR changing the PEP itself (which is less than ideal, but what we've done in the past so I'm not bothered by it).

A quick post on discourse, asking if any backend would have a problem with frontends treating duplicated keys as a list of values, should be sufficient to confirm consensus. It might also be worth pointing out that this would mean that backends will never receive a single-element list for an option - instead, that would be passed as a non-list string argument. That's the thing I'm most concerned about (a backend saying "this option is always a list", and not expecting this quirk).

To be clear, I'm not expecting any arguments here, I just want to make sure we don't suddenly start sending unexpected config values to backends.

@layday
Copy link
Member

layday commented Jan 7, 2023

What do we want to change in the spec? If it's to provide a schema for config settings, I think that'd be a positive change. But if it's to define how command-line options in frontends should behave, that seems a little excessive. It's not unthinkable that another, unaffiliated frontend might wish to read config settings from JSON, like Paul's described above. Personally, I think it's sufficient for pip and build to decide on a common strategy. If we're unsure whether build's or pip's approach is better, or if we believe there's room for improvement, we can continue on Discourse.

@rgommers
Copy link

rgommers commented Jan 7, 2023

What do we want to change in the spec?

I think this (part in italics is added): "Build frontends SHOULD provide some mechanism for users to specify arbitrary string-key/string-value pairs to be placed in this dictionary. In case a user provides duplicate string-key's, build frontends SHOULD combine the corresponding string-value's into a list of string-values. For example ..."

Personally, I think it's sufficient for pip and build to decide on a common strategy.

Either way works for me. It seems like a minimal clarification.

@pfmoore
Copy link
Member

pfmoore commented Jan 7, 2023

The one thing that still bugs me is that backends will have to special-case a string value as equivalent to a 1-element list containing that value. I assume setuptools does that, as otherwise the proposed new behaviour wouldn't work at all (and from what I understand it does work in build). But it seems like a weird condition to impose on backends, that they have to allow this.

Given that using a space-separated list of values --config-settings="--build-option=--cffi --avx2" works for setuptools, because the specification of the --build-option` config setting is that it gets split on spaces, do we actually need this change? Are there any backends that require a config option to be supplied as a list? And if so, how do they expect a single-item list to be supplied?

In reality, of course, hardly anyone supports config options, and the setuptools --build-option case is a holdover from the legacy setup.py command line form, so there really isn't any good example of the "right" way to do things here...

@rgommers
Copy link

rgommers commented Jan 7, 2023

But it seems like a weird condition to impose on backends, that they have to allow this.

Backends do not have to allow this. If they have no use for it, and hence not implement it, then there is also no valid reason for users to provide duplicate keys to the build frontend.

Are there any backends that require a config option to be supplied as a list? And if so, how do they expect a single-item list to be supplied?

Meson-python for one: mesonbuild/meson-python#235. A single-item list works fine with what both pip and build do today.

It's possible I think to have a --config-settings=key="a value", so the backend can't be sure about splitting on spaces. Passing multiple --config-settings seems more unambiguous. And is already done when keys are not the same, probably for the same reason.

@pfmoore
Copy link
Member

pfmoore commented Jan 7, 2023

So meson-python also translates singe strings to 1-item lists here. Cool.

I think the next step is probably for someone to propose a PR to the spec, and a PR for pip.

@eli-schwartz
Copy link
Contributor

Meson (not the PEP 517 build backend) definitely supports receiving config values that contain embedded spaces. In fact, it also supports receiving array types like this: meson setup builddir/ -Dsimpleopt=simpleval "-Doptname=['val1', 'val2', 'val3,has,commas and spaces']" but if you don't have "unusual" values with commas in them, you can just define it as meson setup builddir/ -Dsimpleopt=simpleval -Doptname=val1,val2,val3.

In short, people can put weird things into an option, and having some form of reliably transcribing that is probably useful.

One method is of course --config-settings=json-blob. Another method is having --config-settings=shlex-parsed-string. But they're both quoting hell, just to different degrees, so, I agree -- it's probably best to get out of that game as rapidly as possible and just accumulate opaque values to the right side of an equals sign.

@uranusjr
Copy link
Member

uranusjr commented Jan 8, 2023

If we’re to change the spec, wouldn’t a simpler solution to make config_settings a list of key-value tuples so backends can decide how to parse them?

@rgommers
Copy link

rgommers commented Jan 8, 2023

@uranusjr that does seem like a nicer design, but it runs the risk of backwards compat issues for some backends I think. If they don't support list of string input, they need to be updated first and then pip would only be able to make that change much later. If it's str | list[str, ...], then there's nothing that backends need to do. Do you think it's worth pushing for that larger change?

@ghost
Copy link
Author

ghost commented Jan 9, 2023

IMO we can adopt the above discussion:
1, Only pass the string after the first equal sign to backend. #11681 (comment)
2, Use this form to deal with multiple specifying: str | list[str, ...]. #11681 (comment)

Another question, does the key should be stripped?
If there is a space before the first equal sign, setuptools doesn't recognize this key as --build-option.

pip wheel --config-settings="--build-option = --cffi" -v .

@uranusjr
Copy link
Member

uranusjr commented Jan 9, 2023

If it's str | list[str, ...], then there's nothing that backends need to do

If a user is relying on the current behaviour (a later assignment overwrites the previous value), this would still make the backend crash in a weird way since the backend is only expecting to receive str values. If we want to eliminate this, some sort of flags would be needed in the backend anyway for compatibility. Unless we don’t feel this is worth supporting (by telling users that overwriting values was an accident and considered a bug).

@ghost
Copy link
Author

ghost commented Jan 9, 2023

If a user is relying on the current behaviour (a later assignment overwrites the previous value)

This behavior is different from doc, can be seem as a bug.

since the backend is only expecting to receive str values

Backends probably allow the examples in PEP-517.

@pradyunsg
Copy link
Member

Unless we don’t feel this is worth supporting (by telling users that overwriting values was an accident and considered a bug)

I don't think it's worth supporting, FWIW.

@rgommers
Copy link

A quick post on discourse, asking if any backend would have a problem with frontends treating duplicated keys as a list of values, should be sufficient to confirm consensus.

Done in https://discuss.python.org/t/amending-pep-517-with-a-sentence-on-handling-duplicate-config-settings-keys/23222

@pradyunsg pradyunsg added state: awaiting PR Feature discussed, PR is needed and removed S: needs triage Issues/PRs that need to be triaged labels Feb 1, 2023
@pradyunsg pradyunsg added the PEP implementation Involves some PEP label Feb 1, 2023
FFY00 added a commit to FFY00/pip that referenced this issue Mar 17, 2023
Signed-off-by: Filipe Laíns <lains@riseup.net>
pfmoore added a commit that referenced this issue Mar 17, 2023
kai687 pushed a commit to kai687/sphinxawesome-theme that referenced this issue Apr 16, 2023
Bumps [pip](https://github.com/pypa/pip) from 23.0.1 to 23.1.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's
changelog</a>.</em></p>
<blockquote>
<h1>23.1 (2023-04-15)</h1>
<h2>Deprecations and Removals</h2>
<ul>
<li>Remove support for the deprecated <code>--install-options</code>.
(<code>[#11358](pypa/pip#11358)
&lt;https://github.com/pypa/pip/issues/11358&gt;</code>_)</li>
<li><code>--no-binary</code> does not imply <code>setup.py
install</code> anymore. Instead a wheel will be
built locally and installed.
(<code>[#11451](pypa/pip#11451)
&lt;https://github.com/pypa/pip/issues/11451&gt;</code>_)</li>
<li><code>--no-binary</code> does not disable the cache of locally built
wheels anymore. It only
means &quot;don't download wheels&quot;.
(<code>[#11453](pypa/pip#11453)
&lt;https://github.com/pypa/pip/issues/11453&gt;</code>_)</li>
<li>Deprecate <code>--build-option</code> and
<code>--global-option</code>. Users are invited to switch to
<code>--config-settings</code>.
(<code>[#11859](pypa/pip#11859)
&lt;https://github.com/pypa/pip/issues/11859&gt;</code>_)</li>
<li>Using <code>--config-settings</code> with projects that don't have a
<code>pyproject.toml</code> now print
a deprecation warning. In the future the presence of config settings
will automatically
enable the default build backend for legacy projects and pass the
setttings to it.
(<code>[#11915](pypa/pip#11915)
&lt;https://github.com/pypa/pip/issues/11915&gt;</code>_)</li>
<li>Remove <code>setup.py install</code> fallback when building a wheel
failed for projects without
<code>pyproject.toml</code>.
(<code>[#8368](pypa/pip#8368)
&lt;https://github.com/pypa/pip/issues/8368&gt;</code>_)</li>
<li>When the <code>wheel</code> package is not installed, pip now uses
the default build backend
instead of <code>setup.py install</code> for project without
<code>pyproject.toml</code>.
(<code>[#8559](pypa/pip#8559)
&lt;https://github.com/pypa/pip/issues/8559&gt;</code>_)</li>
</ul>
<h2>Features</h2>
<ul>
<li>Specify egg-link location in assertion message when it does not
match installed location to provide better error message for debugging.
(<code>[#10476](pypa/pip#10476)
&lt;https://github.com/pypa/pip/issues/10476&gt;</code>_)</li>
<li>Present conflict information during installation after each choice
that is rejected (pass <code>-vv</code> to <code>pip install</code> to
show it) (<code>[#10937](pypa/pip#10937)
&lt;https://github.com/pypa/pip/issues/10937&gt;</code>_)</li>
<li>Display dependency chain on each Collecting/Processing log line.
(<code>[#11169](pypa/pip#11169)
&lt;https://github.com/pypa/pip/issues/11169&gt;</code>_)</li>
<li>Support a per-requirement <code>--config-settings</code> option in
requirements files.
(<code>[#11325](pypa/pip#11325)
&lt;https://github.com/pypa/pip/issues/11325&gt;</code>_)</li>
<li>The <code>--config-settings</code>/<code>-C</code> option now
supports using the same key multiple
times. When the same key is specified multiple times, all values are
passed to
the build backend as a list, as opposed to the previous behavior, where
pip would
only pass the last value if the same key was used multiple times.
(<code>[#11681](pypa/pip#11681)
&lt;https://github.com/pypa/pip/issues/11681&gt;</code>_)</li>
<li>Add <code>-C</code> as a short version of the
<code>--config-settings</code> option.
(<code>[#11786](pypa/pip#11786)
&lt;https://github.com/pypa/pip/issues/11786&gt;</code>_)</li>
<li>Reduce the number of resolver rounds, since backjumping makes the
resolver more efficient in finding solutions. This also makes
pathological cases fail quicker.
(<code>[#11908](pypa/pip#11908)
&lt;https://github.com/pypa/pip/issues/11908&gt;</code>_)</li>
<li>Warn if <code>--hash</code> is used on a line without requirement in
a requirements file.
(<code>[#11935](pypa/pip#11935)
&lt;https://github.com/pypa/pip/issues/11935&gt;</code>_)</li>
<li>Stop propagating CLI <code>--config-settings</code> to the build
dependencies. They already did
not propagate to requirements provided in requirement files. To pass the
same config
settings to several requirements, users should provide the requirements
as CLI
arguments. (<code>[#11941](pypa/pip#11941)
&lt;https://github.com/pypa/pip/issues/11941&gt;</code>_)</li>
<li>Support wheel cache when using <code>--require-hashes</code>.
(<code>[#5037](pypa/pip#5037)
&lt;https://github.com/pypa/pip/issues/5037&gt;</code>_)</li>
<li>Add <code>--keyring-provider</code> flag. See the Authentication
page in the documentation for more info.
(<code>[#8719](pypa/pip#8719)
&lt;https://github.com/pypa/pip/issues/8719&gt;</code>_)</li>
<li>In the case of virtual environments, configuration files are now
also included from the base installation.
(<code>[#9752](pypa/pip#9752)
&lt;https://github.com/pypa/pip/issues/9752&gt;</code>_)</li>
</ul>
<h2>Bug Fixes</h2>
<ul>
<li>Fix grammar by changing &quot;A new release of pip available:&quot;
to &quot;A new release of pip is available:&quot; in the notice used for
indicating that.
(<code>[#11529](pypa/pip#11529)
&lt;https://github.com/pypa/pip/issues/11529&gt;</code>_)</li>
<li>Normalize paths before checking if installed scripts are on PATH.
(<code>[#11719](pypa/pip#11719)
&lt;https://github.com/pypa/pip/issues/11719&gt;</code>_)</li>
<li>Correct the way to decide if keyring is available.
(<code>[#11774](pypa/pip#11774)
&lt;https://github.com/pypa/pip/issues/11774&gt;</code>_)</li>
<li>More consistent resolution backtracking by removing legacy hack
related to setuptools resolution
(<code>[#11837](pypa/pip#11837)
&lt;https://github.com/pypa/pip/issues/11837&gt;</code>_)</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pypa/pip/commit/6424ac4600265490462015c2fc7f9a402dba9ed8"><code>6424ac4</code></a>
Bump for release</li>
<li><a
href="https://github.com/pypa/pip/commit/868338f9f79b58eff34dafb168aed65480d080d5"><code>868338f</code></a>
Update AUTHORS.txt</li>
<li><a
href="https://github.com/pypa/pip/commit/4f3a4f72697299da1a412cf10c919a989e0692f5"><code>4f3a4f7</code></a>
Merge pull request <a
href="https://github.com/pypa/pip/issues/11919">#11919</a> from
sbidoul/deprecate-legacy-ignore-config-setting...</li>
<li><a
href="https://github.com/pypa/pip/commit/dbf4e6842c9603792f6d3944a5c9cec17bd0a92a"><code>dbf4e68</code></a>
Merge pull request <a
href="https://github.com/pypa/pip/issues/11897">#11897</a> from
sbidoul/cache-hash-checking-sbi</li>
<li><a
href="https://github.com/pypa/pip/commit/efe2d27451d50b165df78093bf5885da713fbdf8"><code>efe2d27</code></a>
Further refactor is_wheel_from_cache</li>
<li><a
href="https://github.com/pypa/pip/commit/4beca6b4c9c510b19dbb6180e962425b89e8c839"><code>4beca6b</code></a>
Improve test</li>
<li><a
href="https://github.com/pypa/pip/commit/bd746e3136e5e1be2374a079bac66071dd967a8c"><code>bd746e3</code></a>
Introduce ireq.cached_wheel_source_link</li>
<li><a
href="https://github.com/pypa/pip/commit/caafe6e87d4f2998a77b194297e1c204cf6e10c2"><code>caafe6e</code></a>
Add a couple of asserts</li>
<li><a
href="https://github.com/pypa/pip/commit/a6ef6485be9512f18121298b058797c578f65d45"><code>a6ef648</code></a>
Rename original_link_is_in_wheel_cache to is_wheel_from_cache</li>
<li><a
href="https://github.com/pypa/pip/commit/ff8c8e38887880ad81ffd7cfc6a8373213c087b7"><code>ff8c8e3</code></a>
Cosmetics</li>
<li>Additional commits viewable in <a
href="https://github.com/pypa/pip/compare/23.0.1...23.1">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=23.0.1&new-version=23.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PEP implementation Involves some PEP state: awaiting PR Feature discussed, PR is needed type: bug A confirmed bug or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants