fix: wrap string config setting values in list before iterating#10804
Merged
radoering merged 2 commits intoMar 31, 2026
Conversation
Reviewer's GuideEnsures build backend error reporting correctly formats pip --config-settings flags when config setting values are plain strings, and adds a regression test to cover this behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_execute_operation, consider iterating overconfig_settings.items()(e.g.for setting, values in config_settings.items():) instead of indexing the dict twice to simplify the loop and avoid repeated lookups. - When normalizing
values, you currently special-case onlystr; if other scalar types are ever passed (e.g. bytes), you may want to either handle them similarly or validate and raise early, so you don’t silently iterate over unexpected types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_execute_operation`, consider iterating over `config_settings.items()` (e.g. `for setting, values in config_settings.items():`) instead of indexing the dict twice to simplify the loop and avoid repeated lookups.
- When normalizing `values`, you currently special-case only `str`; if other scalar types are ever passed (e.g. bytes), you may want to either handle them similarly or validate and raise early, so you don’t silently iterate over unexpected types.
## Individual Comments
### Comment 1
<location path="tests/installation/test_executor.py" line_range="1575" />
<code_context>
assert io.fetch_output() == expected_output
+def test_build_backend_error_includes_config_settings_in_pip_command(
+ mocker: MockerFixture,
+ config: Config,
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for non-string/multiple config-setting values to ensure behavior for list inputs and multiple keys is preserved.
To better exercise `build-config-settings`, consider also covering:
- A setting whose value is already a list (e.g. `{"simple-project": {"CC": ["gcc", "clang"]}}`), asserting we get two `--config-settings` flags.
- Multiple keys for the same package (e.g. `{"CC": "gcc", "CFLAGS": "-O2"}`), asserting all appear in the pip command.
You can add a separate test for these or parametrize this one over string vs list vs multiple-key configurations to guard against regressions in value handling.
Suggested implementation:
```python
@pytest.mark.parametrize(
"config_settings,expected_flags",
[
# A single string value for a single key
(
{"simple-project": {"CC": "gcc"}},
["--config-settings", "simple-project:CC=gcc"],
),
# A list value for a single key should expand to multiple flags
(
{"simple-project": {"CC": ["gcc", "clang"]}},
[
"--config-settings",
"simple-project:CC=gcc",
"--config-settings",
"simple-project:CC=clang",
],
),
# Multiple keys for the same package should all appear
(
{"simple-project": {"CC": "gcc", "CFLAGS": "-O2"}},
[
"--config-settings",
"simple-project:CC=gcc",
"--config-settings",
"simple-project:CFLAGS=-O2",
],
),
],
)
def test_build_backend_error_includes_config_settings_in_pip_command(
mocker: MockerFixture,
config: Config,
pool: RepositoryPool,
io: BufferedIO,
env: MockEnv,
fixture_dir: FixtureDirGetter,
config_settings: dict[str, dict[str, object]],
expected_flags: list[str],
) -> None:
"""Config setting values (strings, lists, multiple keys) must be expanded to full flags."""
error = BuildBackendException(
Exception("build failed"), description="build failed"
)
mocker.patch.object(ProjectBuilder, "build", side_effect=error)
# Configure build-config-settings for this parameterized case
# The concrete way to attach this to `config` should match how the executor
# currently reads build configuration in this test file.
config.set_build_config_settings(config_settings) # type: ignore[attr-defined]
```
To fully implement the requested coverage, adjust the rest of `test_build_backend_error_includes_config_settings_in_pip_command` (the part not shown in the snippet) to:
1. Use the parameterized `config_settings` instead of any hard-coded build-config-settings:
- If you are currently doing something like:
`config.set_build_config_settings({"simple-project": {"CC": "gcc"}})`
replace that with the already-added call or adapt it to your API.
- If `Config` does not expose `set_build_config_settings`, use the correct mechanism already used in this file to inject build-config-settings (e.g. `config.merge()`, direct attribute assignment, or a dedicated helper). In that case, update the line I added to that mechanism instead of `set_build_config_settings`.
2. Assert that all `expected_flags` appear in the pip command constructed from the error:
- Wherever you currently assert on the pip command (typically via `io.fetch_output()` or `io.fetch_error()` or by inspecting a `cmd` list), extend the assertion to verify:
- Each `expected_flags[i]` appears in order.
- The number of `--config-settings` occurrences matches the length of `expected_flags` // 2 (for flag/value pairs).
- For example, if you have a `cmd` list:
`cmd = <whatever builds the pip command>`
add an assertion such as:
`assert expected_flags == [arg for arg in cmd if arg in expected_flags]`
or, if you capture a string:
`output = io.fetch_output()`
`for flag in expected_flags: assert flag in output`
3. Ensure `pytest` is imported in this file if it is not already:
- At the top of `tests/installation/test_executor.py`, add:
`import pytest`
- If there is already a `pytest` import, no further changes are needed.
These adjustments will give the test real coverage of:
- A single string-valued config-setting.
- A list-valued config-setting expanding to multiple `--config-settings` flags.
- Multiple keys for the same package, all present in the pip command.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Documentation Updates 1 document(s) were updated by changes in this PR: CHANGELOGView Changes@@ -11,6 +11,7 @@
- Fix an issue where HTTP Basic Authentication credentials could be corrupted during request preparation, causing authentication failures with long tokens ([#10748](https://github.com/python-poetry/poetry/pull/10748)).
- Fix an issue where `poetry publish --no-interaction --build` requested user interaction ([#10769](https://github.com/python-poetry/poetry/pull/10769)).
- Fix an issue where `poetry init` and `poetry new` created a deprecated `project.license` format ([#10787](https://github.com/python-poetry/poetry/pull/10787)).
+- Fix an issue where string config setting values in `installer.build-config-settings` were iterated character-by-character instead of as whole values, producing garbled `--config-settings` flags in the pip fallback error message command ([#10804](https://github.com/python-poetry/poetry/pull/10804)).
### Docs
How did I do? Any feedback? |
dimbleby
force-pushed
the
fix/executor-string-config
branch
from
5a2713e to
eca8626
Compare
When a config setting value is a plain str (e.g. "CC": "gcc"), iterating it directly yields one character per iteration, producing garbled --config-settings flags in the error message pip command. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
radoering
force-pushed
the
fix/executor-string-config
branch
from
eca8626 to
e6bd472
Compare
radoering
approved these changes
Mar 31, 2026
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a config setting value is a plain str (e.g. "CC": "gcc"), iterating it directly yields one character per iteration, producing garbled --config-settings flags in the error message pip command.
Pull Request Check List
Resolves: #issue-number-here
Summary by Sourcery
Ensure build backend error reporting correctly formats build config settings in generated pip commands.
Bug Fixes:
Tests: