Environment variables override cargo --config <filename> in precedence

[sunshowers]

Problem

If both an environment variable and cargo --config <filename> are specified at the same time, the environment variable overrides the file passed in over the CLI. This is counter-intuitive and also the documentation at https://doc.rust-lang.org/cargo/reference/config.html#command-line-overrides is unclear about this:

Configuration values specified this way take precedence over environment variables, which take precedence over configuration files.

Steps

Create .cargo/foo.toml:

[build]
target-dir = "from-foo-toml"

Then, run:

CARGO_BUILD_TARGET_DIR=from-env cargo build --config .cargo/foo.toml

Notice that the build happens in "from-env" rather than the expected "from-too-toml".

Possible Solution(s)

I think the behavior should be changed so that config files passed in over --config come first and environment variables come later.

(Unlike #10991, this I think is straight-up incorrect and should be fixed.)

Notes

I'm guessing this is a regression from #10755 and specifically this change, though I could be wrong. cc @jonhoo, @ehuss

Version

cargo 1.63.0 (fd9c4297c 2022-07-01)
release: 1.63.0
commit-hash: fd9c4297ccbee36d39e9a79067edab0b614edb5a
commit-date: 2022-07-01
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1n)
os: Pop!_OS 22.04 (jammy) [64-bit]

[ehuss]

Hm, yea, that's a strange intersection. I'm not sure how hard that would be to fix, but would be good to investigate.

[sunshowers]

I also realized that there's a related issue. If --config is passed in in this fashion:

--config 'k=v' --config foo.toml --config 'k2=v2' --config bar.toml

Then the behavior I expect is that configs would have precedence in this order:

1. k=v
2. foo.toml
3. k2=v2
4. bar.toml
5. environment
6. .cargo/config.toml, with discovery as before

However, in reality, configs have precedence in this order:

1. k=v
2. k2=v2
3. environment
4. foo.toml
5. bar.toml
6. .cargo/config.toml, with discovery as before

[weihanglo]

From my understanding, the change you referred to might not be the root cause. Either config-include or Config::_load_file will eventually create a ConfigValue with Definition::Path. As Definition::Path is the lowest priority of the three, and the comparison with env var occurs here, so env vars always win over configs from --config <path> and config-include.

-config is passed in in this fashion:

--config 'k=v' --config foo.toml --config 'k2=v2' --config bar.toml

Then the behavior I expect is that configs would have precedence in this order:
...

You are correct that the behaviour is wrong. The precedence you describe could be more precise. AFAIK the flow looks like,

1. CLI args get merged first. The latter arg takes precedence over former (force merge).
2. A request of a ConfigValue triggers a deserialization
3. During the deserialization, get_cv_with_env get called.

For instance, when one runs cargo check with args and env vars below,

k=env0
--config k='cli0'
--config k='cli1'
--config .cargo/file0.toml # k='file0'
--config k='cli2'
--config .cargo/file1.toml # k='file1'

It would look like the followings after cli-args merge before env merge:

k='file1' [from path]
k=env0 [from env]

I feel like the fix would be tricky. We may want to construct a ConfigValue with Definition:Cli here when _load_file is triggered by config-cli, though doing that loses the path information of Definition::Path, regressing the error message. The other way is attaching the requested-from-cli information right on Definition::Path enum, which is more intrusive IMO but the good error message remains.

^{Click to expand this cargo test case exercising the behaviour aforementioned.}

#[cargo_test]
fn merge_primitives_for_multiple_cli_occurences() {
    let config_path0 = ".cargo/file0.toml";
    write_config_at(config_path0, "k = 'file0'");
    let config_path1 = ".cargo/file1.toml";
    write_config_at(config_path1, "k = 'file1'");

    // k=env0
    let config = ConfigBuilder::new().env("CARGO_K", "env0").build();
    assert_eq!(config.get::<String>("k").unwrap(), "env0");

    // k=env0
    // --config k='cli0'
    // --config k='cli1'
    let config = ConfigBuilder::new()
        .env("CARGO_K", "env0")
        .config_arg("k='cli0'")
        .config_arg("k='cli1'")
        .build();
    assert_eq!(config.get::<String>("k").unwrap(), "cli1");

    // Env has a lower priority when comparing with file from CLI arg.
    //
    // k=env0
    // --config k='cli0'
    // --config k='cli1'
    // --config .cargo/file0.toml
    let config = ConfigBuilder::new()
        .env("CARGO_K", "env0")
        .config_arg("k='cli0'")
        .config_arg("k='cli1'")
        .config_arg(config_path0)
        .build();
    assert_eq!(config.get::<String>("k").unwrap(), "file0");

    // k=env0
    // --config k='cli0'
    // --config k='cli1'
    // --config .cargo/file0.toml
    // --config k='cli2'
    let config = ConfigBuilder::new()
        .env("CARGO_K", "env0")
        .config_arg("k='cli0'")
        .config_arg("k='cli1'")
        .config_arg(config_path0)
        .config_arg("k='cli2'")
        .build();
    assert_eq!(config.get::<String>("k").unwrap(), "cli2");

    // k=env0
    // --config k='cli0'
    // --config k='cli1'
    // --config .cargo/file0.toml
    // --config k='cli2'
    // --config .cargo/file1.toml
    let config = ConfigBuilder::new()
        .env("CARGO_K", "env0")
        .config_arg("k='cli0'")
        .config_arg("k='cli1'")
        .config_arg(config_path0)
        .config_arg("k='cli2'")
        .config_arg(config_path1)
        .build();
    assert_eq!(config.get::<String>("k").unwrap(), "file1");
}