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

fix(config): When merging, replace rather than combining specific configuration keys #15066

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Jan 14, 2025

In general, Cargo merges lists in configuration by concatenating them. However, sometimes the lists don't make sense for merging, such as a program and its arguments. We had the UnmergedStringList type that handled this case for merging environment variables, but it did not work for multiple configs.

  • Removes the UnmergedStringList type, which only worked for preventing merging of environment variables with configuration.
  • Adds a new function is_nonmergable_list which hard-codes which configuration keys contain lists that should not be merged.

Fixes #14906

@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2025
@epage
Copy link
Contributor

epage commented Jan 15, 2025

When we talked about this in the Cargo team meeting (2025-01-07), we were ok with this so long as we felt there was a proper way of fixing this later. One idea to do that was to delay merging until access. We're unsure how difficult that would be to implement.

@rustbot

This comment has been minimized.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase on latest master and resolve the conflict?

src/cargo/util/context/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/context/mod.rs Show resolved Hide resolved
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

We discussed the concerns about potentially breaking behavior, since it is feasible someone is relying on something like runner concatenating arguments. Our perspective is that this fixes behavior that we weren't expecting, and brings it in line with how env merging works. We decided that we will monitor for any issues that get raised in nightly or beta, and consider backing it out. We'll make sure that the release notes for stable have a notice about this change.

@ehuss ehuss added this pull request to the merge queue Jan 21, 2025
Merged via the queue into rust-lang:master with commit 2668a4f Jan 21, 2025
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2025
Update cargo

5 commits in 045bf21b36a2e1f3ed85e38278d1c3cc4305e134..cecde95c119a456c30e57d3e4b31fff5a7d83df4
2025-01-17 14:59:36 +0000 to 2025-01-24 17:15:24 +0000
- Remove unused `-C link-arg=-fuse-ld=lld` (rust-lang/cargo#15097)
- Remove `unsafe` by using `LazyLock` (rust-lang/cargo#15096)
- Print globs when workspace members can't be found (rust-lang/cargo#15093)
- Make --allow-dirty imply --allow-staged (rust-lang/cargo#15013)
- fix(config): When merging, replace rather than combining specific configuration keys (rust-lang/cargo#15066)
@rustbot rustbot added this to the 1.86.0 milestone Jan 25, 2025
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 26, 2025
Update cargo

5 commits in 045bf21b36a2e1f3ed85e38278d1c3cc4305e134..cecde95c119a456c30e57d3e4b31fff5a7d83df4
2025-01-17 14:59:36 +0000 to 2025-01-24 17:15:24 +0000
- Remove unused `-C link-arg=-fuse-ld=lld` (rust-lang/cargo#15097)
- Remove `unsafe` by using `LazyLock` (rust-lang/cargo#15096)
- Print globs when workspace members can't be found (rust-lang/cargo#15093)
- Make --allow-dirty imply --allow-staged (rust-lang/cargo#15013)
- fix(config): When merging, replace rather than combining specific configuration keys (rust-lang/cargo#15066)
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Jan 27, 2025
Update cargo

5 commits in 045bf21b36a2e1f3ed85e38278d1c3cc4305e134..cecde95c119a456c30e57d3e4b31fff5a7d83df4
2025-01-17 14:59:36 +0000 to 2025-01-24 17:15:24 +0000
- Remove unused `-C link-arg=-fuse-ld=lld` (rust-lang/cargo#15097)
- Remove `unsafe` by using `LazyLock` (rust-lang/cargo#15096)
- Print globs when workspace members can't be found (rust-lang/cargo#15093)
- Make --allow-dirty imply --allow-staged (rust-lang/cargo#15013)
- fix(config): When merging, replace rather than combining specific configuration keys (rust-lang/cargo#15066)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration merge doesn't make sense for credential-provider
4 participants