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.

Sign up for GitHub

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

Jump to bottom

Config file loaded via CLI takes priority over env vars #11077

Merged
merged 7 commits into from
Oct 9, 2022
Merged

Config file loaded via CLI takes priority over env vars #11077

merged 7 commits into from
Oct 9, 2022

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Sep 12, 2022 •
edited
Loading

What does this PR try to resolve?

Fixes #10992

Behaviour changes: After this commit, config files loaded via CLI take
priority over env vars. Config files loaded via config-include
feature also get a higher priority over env vars, if the initial config
file is being loaded via CLI.

Cargo knows why it loads a specific config file with WhyLoad enum,
and store in the field of Definition::Cli(…). With this additional data,
config files loaded via --config <path> get a Definition::Cli(Some(…)).
In contrast, --config <value> with ordinary values become Definition::Cli(None).

The error message of the Definition::Cli(Some(…)) is identical to
Definition::Path(…), so we won't lose any path information to debug.

How should we test and review this PR?

Reviewing commit by commit is probably fine. The first two commits adding tests to config-cli and config-include, which exercises the expected behaviour we are going to fix in the next commits.

To check the precedence, set up a project with an extra config file, such as

# Expect to have a target-dir named `foo`
CARGO_BUILD_TARGET_DIR='env' cargo c --config 'build.target-dir = "cli"' --config .cargo/foo.toml

# Inside .cargo/foo.toml
[build]
target-dir = "foo"

Additional information

This is a bit hacky IMO. I don't like leaking the path info to Definition::Cli. However, it is really tricky to provide information for deserialization before values get merged.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2022
@weihanglo weihanglo added A-configuration Area: cargo config files and env vars A-config-cli Area: --config CLI option labels Sep 13, 2022
@weihanglo
test(config_cli): multi-occurrence cli args with paths
fafb8a9
@weihanglo
test(config_include): take priority over env when loaded from cli
e997fbc
@weihanglo
Config file loaded via CLI takes priority over env vars
f27f733 
Behaviour changes: After this commit, config files loaded via CLI take
priority over env vars. Config files loaded via [`config-include`]
feature also get a higher priority over env vars, if the initial config
file is being loaded via CLI.

Cargo knows why it loads a specific config file with `WhyLoad` enum,
and store in the field of `Definition::Cli(…)`. With this additional data,
config files loaded via `--config <path>` get a `Definition::Cli(Some(…))`.
In contrast, `--config <value>` with ordinary values become `Definition::Cli(None)`.

The error message of the `Definition::Cli(Some(…))` is identical to
`Definition::Path(…)`, so we won't lose any path information to debug.

[`config-include`]: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#config-include
@weihanglo
add api docs for config loading functions
cd633ef
@weihanglo
Remove unnecessary parameter includes
dcf9669
jonhoo
jonhoo approved these changes Oct 9, 2022
View reviewed changes
Copy link
Contributor

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Looks good to me, subject to one typo in the test name 😅

tests/testsuite/config_include.rs Outdated Show resolved Hide resolved
@weihanglo @jonhoo
test(config-include): fix typo
e0502e4 
Co-authored-by: Jon Gjengset <jon@thesquareplanet.com>
@ehuss
Copy link
Contributor

ehuss commented Oct 9, 2022

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 9, 2022

📌 Commit e0502e4 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2022
@bors
Copy link
Contributor

bors commented Oct 9, 2022

⌛ Testing commit e0502e4 with merge 0c29d43...

@bors
Copy link
Contributor

bors commented Oct 9, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 0c29d43 to master...

@bors bors merged commit 0c29d43 into rust-lang:master Oct 9, 2022
@weihanglo weihanglo deleted the issue-10992 branch October 9, 2022 18:39
weihanglo added a commit to weihanglo/rust that referenced this pull request Oct 11, 2022
@weihanglo
Update cargo
1ff4f52 
9 commits in 3cdf1ab25dc4fe56f890e8c7330d53a23ad905d3..b8f30cb23c4e5f20854a4f683325782b7cff9837
2022-10-07 17:34:03 +0000 to 2022-10-10 19:16:06 +0000

- Add more doc comments for three modules (rust-lang/cargo#11207)
- docs: fix (rust-lang/cargo#11208)
- Add completions for `cargo remove` (rust-lang/cargo#11204)
- Config file loaded via CLI takes priority over env vars (rust-lang/cargo#11077)
- Use `#[default]` when possible (rust-lang/cargo#11197)
- Implement RFC 3289: source replacement ambiguity (rust-lang/cargo#10907)
- Use correct version of cargo in test (rust-lang/cargo#11193)
- Check empty input for login (rust-lang/cargo#11145)
- Add retry support to sparse registries (rust-lang/cargo#11069)
@weihanglo weihanglo mentioned this pull request Oct 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2022
@bors
Auto merge of rust-lang#102934 - weihanglo:update-cargo, r=ehuss
7e8d64e 
Update cargo

9 commits in 3cdf1ab25dc4fe56f890e8c7330d53a23ad905d3..b8f30cb23c4e5f20854a4f683325782b7cff9837 2022-10-07 17:34:03 +0000 to 2022-10-10 19:16:06 +0000

- Add more doc comments for three modules (rust-lang/cargo#11207)
- docs: fix (rust-lang/cargo#11208)
- Add completions for `cargo remove` (rust-lang/cargo#11204)
- Config file loaded via CLI takes priority over env vars (rust-lang/cargo#11077)
- Use `#[default]` when possible (rust-lang/cargo#11197)
- Implement RFC 3289: source replacement ambiguity (rust-lang/cargo#10907)
- Use correct version of cargo in test (rust-lang/cargo#11193)
- Check empty input for login (rust-lang/cargo#11145)
- Add retry support to sparse registries (rust-lang/cargo#11069)
@ehuss ehuss added this to the 1.66.0 milestone Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonhoo jonhoo jonhoo approved these changes

Assignees

@ehuss ehuss

Labels
A-config-cli Area: --config CLI option A-configuration Area: cargo config files and env vars S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.

Environment variables override cargo --config <filename> in precedence
5 participants
@weihanglo @rust-highfive @ehuss @bors @jonhoo