Setting target.runner via --config twice concatenates lists #10893

RalfJung · 2022-07-23T21:49:29Z

Consider the following invocation:

cargo +nightly run --target "x86_64-unknown-linux-gnu" --config "target.'cfg(all())'.runner=['run']" --config "target.'cfg(all())'.runner=['run']"

This fails saying

error: could not execute process `run run target/x86_64-unknown-linux-gnu/debug/xargo` (never executed)

I think there is a bug here: cargo tried to execute run run ..., i.e., it concatenated the two configs I gave! I would have expected the second config to overwrite the first, similar to what happens since #8629 when the runner is set both in cargo.toml and via the env var.

The text was updated successfully, but these errors were encountered:

Rustin170506 · 2022-08-08T14:32:08Z

@rustbot claim

Rustin170506 · 2022-08-09T13:41:35Z

I think this is a feature, not a bug.

/// A config type that is a program to run.
///
/// This supports a list of strings like `['/path/to/program', 'somearg']`
/// or a space separated string like `'/path/to/program somearg'`.
///
/// This expects the first value to be the path to the program to run.
/// Subsequent values are strings of arguments to pass to the program.
///
/// Typically you should use `ConfigRelativePath::resolve_program` on the path
/// to get the actual program.
#[derive(Debug, Clone)]
pub struct PathAndArgs {
    pub path: ConfigRelativePath,
    pub args: Vec<String>,
}

So cargo treats the second run as an argument to the first run.

RalfJung · 2022-08-09T13:44:30Z

But when would that be what you want?
I might have set one runner for all targets and then another runner for a specific target. It shouldn't just concatenate those lists.

When using the env var CARGO_RUNNER, it correctly overwrites the value it would otherwise use. That used to also concatenate, but in #8629 the old behavior was deemed incorrect.

Rustin170506 · 2022-08-09T13:57:53Z

Could you try cargo run --config "target.'cfg(all())'.runner=['run1']" --config "target.'cfg(cfg(not(target_os = "none")))'.runner=['run2']" --config "target.'cfg(all())'.runner=['run3']"?

I think it will use the last one and overwrite the second one.

Also you can try cargo run -config "target.'cfg(cfg(not(target_os = "none")))'.runner=['run2']" --config "target.'cfg(all())'.runner=['run3']"

It will use the last one.

epage · 2022-08-09T14:02:28Z

I believe this is being released on stable in 2 days which will likely limit what we can change about this

Mark-Simulacrum · 2022-08-09T14:09:08Z

There's still room for any patches, but please communicate closely with me (over Zulip, most likely) if the Cargo team would like to adjust things.

Rustin170506 · 2022-08-09T14:13:20Z

For now, cargo run --config "target.'cfg(all())'.runner=['run1']" --config "target.'cfg(all())'.runner=['run3']"
equal to

[target.'cfg(all())']
runner = ["run1","run3"]

I'm not sure if should we change the behavior.

Rustin170506 · 2022-08-09T14:20:46Z

If you try to add this config and also pass the CLI args, it will be concatenated.

[target.'cfg(all())']
runner = ["run1","run2"]

cargo run --config "target.'cfg(all())'.runner=['run3']" --config "target.'cfg(all())'.runner=['run4']"

You will get error: could not execute process run1 run2 run3 run4 target/debug/cargo (never executed).

And if you use the config and env variable, it won't be concatenated.

~~So maybe we should fix it, at least don't concatenate the .cargo/config and the CLI args.~~ I am not sure does it is expected.

RalfJung · 2022-08-09T14:26:57Z

I believe this is being released on stable in 2 days which will likely limit what we can change about this

This is long stable via .cargo/config.toml, --config is just an easier way to describe these examples.

#8629 changed stable behavior AFAIK?

RalfJung · 2022-08-09T14:32:26Z

Oh interesting, this is rejected in config.toml:

[target.'cfg(all())']
runner = ["run1"]

[target.'cfg(all())']
runner = ["run1"]

So it is somewhat strange that it is accepted in --config.

However, if I have this in my ~/.cargo/config.toml

[target.'cfg(all())']
runner = ["run1"]

and this in my local .cargo/config.toml

[target.'cfg(all())']
runner = ["run1"]

Then it also merges the lists. So --config is consistent with having this set it different config files. This is not about --config.

It is about how cargo "merges" the value when it has a value set in different sources. Currently it is inconsistent:

if one source is toml and the other is the env var, "merging" means "the later value overwrites the other ones"
if both sources are toml, "merging" means "the later value gets appended after the previous ones"

I would expect "merging" to treat the env var as just another source, and do merging in the same way as it is done elsewhere. And for this specific case of the runner, IMO "merging" should mean to use the last value. Concatenating the lists makes sense for things like rustflags, but it doesn't make sense for runners.

You will get error: could not execute process run1 run2 run3 run4 target/debug/cargo (never executed).

There is no run2 in any of your settings, I am confused?

Rustin170506 · 2022-08-09T14:38:53Z

There is no run2 in any of your settings, I am confused?

Oops, I posted the out-of-date config and command. updated.

RalfJung · 2022-08-09T14:49:03Z

Right, so all the different toml sources get concatenated together. Having --config be consistent with multiple toml files makes perfect sense. It's just that that behavior is IMO consistently wrong. :)

When one tries using different target conditions, cargo shows an error:

error: several matching instances of `target.'cfg(..)'.runner` in `.cargo/config`
first match `cfg(all())` located in /home/r/.cargo/config
second match `cfg(not(target_os = "none"))` located in /home/r/src/rust/xargo/.cargo/config.toml

RalfJung · 2022-08-09T14:54:20Z

Hm, but there also seems to be a bug in --config here? When my ~/.cargo/config.toml has a all() condition, and then I run

cargo +nightly run --config "target.'cfg(not(target_os = "none"))'.runner=['run3']"

it entirely ignores the --config and only considers the thing in the config file.

So arguably cargo +nightly run --config "target.'cfg(not(target_os = "none"))'.runner=['run3']" --config "target.'cfg(all())'.runner=['run3']" should be rejected, since doing the same thing via config files is being rejected. And then probably my original command at the top of this issue also should be rejected.

So I think there are two bugs:

--config should behave exactly the same as if those configs were written into a cargo.toml file that is read last, after all the other files. But we have some counterexamples to that.
Having target.'cfg(all())'.runner set in different files should be rejected, because having it set twice in the same file is being rejected, and having target.'cfg(all())'.runner in one file and target.'cfg(not(any()))'.runner in another file is also being rejected.

Rustin170506 · 2022-08-09T15:20:02Z

But we have some counterexamples to that.

Could you indicate which ones? Do you mean cargo +nightly run --config "target.'cfg(not(target_os = "none"))'.runner=['run3']" --config "target.'cfg(all())'.runner=['run3']" this one?

Rustin170506 · 2022-08-09T15:21:08Z

Having target.'cfg(all())'.runner set in different files should be rejected, because having it set twice in the same file is being rejected, and having target.'cfg(all())'.runner in one file and target.'cfg(not(any()))'.runner in another file is also being rejected.

I agree. It's confusing.

RalfJung · 2022-08-09T15:23:54Z

Could you indicate which ones?

The original one that started this thread, with two runner in sections that use the same cfg.
And the other one you mentioned where they use different cfg that are both true for the same target.

Rustin170506 · 2022-08-10T14:51:26Z

And the other one you mentioned where they use different cfg that are both true for the same target.

I found that this is a bug in CfgExpr::matches_key. I will try to fix it tomorrow.

Rustin170506 · 2022-08-12T14:11:53Z

I found that this is a bug in CfgExpr::matches_key. I will try to fix it tomorrow.

cargo +nightly run --config "target.'cfg(not(target_os = "none"))'.runner=['run3']" --config "target.'cfg(all())'.runner=['run3']"
and
cargo +nightly run --config "target.'cfg(not(target_os = \"none\"))'.runner=['run3']" --config "target.'cfg(all())'.runner=['run3']"
are different.

Cargo works well with the second one. But it will not report an error for the first one.

We ignore the parse error in:

 /// Utility function to check if the key, "cfg(..)" matches the `target_cfg`
    pub fn matches_key(key: &str, target_cfg: &[Cfg]) -> bool {
        if key.starts_with("cfg(") && key.ends_with(')') {
            let cfg = &key[4..key.len() - 1];

            CfgExpr::from_str(cfg)
                // **Here**
                .ok()
                .map(|ce| ce.matches(target_cfg))
                .unwrap_or(false)
        } else {
            false
        }
    }

Do you think we should report the error for it?

RalfJung · 2022-08-12T14:43:09Z

Oh, good catch!

What happens when a config file omits the "?

[target.'cfg(not(target_os = none))']
runner = ["run1"]

Rustin170506 · 2022-08-12T15:21:32Z

What happens when a config file omits the "?

Same. It will be ignored.

RalfJung · 2022-08-12T15:28:21Z

Okay. Yeah seems better to report parse errors to the user but this could be a breaking change.

Rustin170506 · 2022-08-13T10:42:05Z

Yeah seems better to report parse errors to the user but this could be a breaking change.

@epage What do you think?

Rustin170506 · 2022-09-13T12:55:14Z

I tried to add some warnings in matches_key but failed. Because matches_key in the cargo-platform crate we can not pass a Context or Shell to it because it does not depend on the cargo crate.

Does anyone have any ideas about how to fix or improve it?

Rustin170506 · 2022-09-13T13:11:57Z

Maybe we can move this function out.

RalfJung · 2025-01-31T12:45:13Z

@arlosi I noticed you recently fixed a similar issue in #15066. Would that approach also makes sense for the target runner, to fix this issue?

RalfJung · 2025-01-31T12:47:17Z

Actually I see target.*.runner mentioned there; seems your PR also fixes this old issue?

weihanglo · 2025-01-31T13:51:05Z

Good catch!

Yes I believe #15066 has fixed this issue. Closing.

weihanglo added A-config-cli Area: --config CLI option C-bug Category: bug labels Jul 25, 2022

rustbot assigned Rustin170506 Aug 8, 2022

Rustin170506 mentioned this issue Aug 8, 2022

[Not ready for review] Dedup cli args #10950

Closed

Rustin170506 mentioned this issue Oct 26, 2022

Add a warning for invalid target cfgs #11291

Closed

1 task

Rustin170506 removed their assignment Dec 7, 2022

RalfJung mentioned this issue Mar 27, 2024

Multiple rustflags configs should be cumulative #5376

Open

epage added the S-triage Status: This issue is waiting on initial triage. label Nov 20, 2024

weihanglo closed this as completed Jan 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting target.runner via --config twice concatenates lists #10893

Setting target.runner via --config twice concatenates lists #10893

RalfJung commented Jul 23, 2022 •

edited by rustbot

Loading

Rustin170506 commented Aug 8, 2022

Rustin170506 commented Aug 9, 2022 •

edited

Loading

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

epage commented Aug 9, 2022

Mark-Simulacrum commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022 •

edited

Loading

RalfJung commented Aug 9, 2022

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

RalfJung commented Aug 9, 2022

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 10, 2022

Rustin170506 commented Aug 12, 2022 •

edited

Loading

RalfJung commented Aug 12, 2022

Rustin170506 commented Aug 12, 2022

RalfJung commented Aug 12, 2022

Rustin170506 commented Aug 13, 2022

Rustin170506 commented Sep 13, 2022

Rustin170506 commented Sep 13, 2022

RalfJung commented Jan 31, 2025 •

edited

Loading

RalfJung commented Jan 31, 2025

weihanglo commented Jan 31, 2025

Setting target.runner via --config twice concatenates lists #10893

Setting target.runner via --config twice concatenates lists #10893

Comments

RalfJung commented Jul 23, 2022 • edited by rustbot Loading

Rustin170506 commented Aug 8, 2022

Rustin170506 commented Aug 9, 2022 • edited Loading

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

epage commented Aug 9, 2022

Mark-Simulacrum commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022 • edited Loading

RalfJung commented Aug 9, 2022

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

RalfJung commented Aug 9, 2022

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

Rustin170506 commented Aug 9, 2022

RalfJung commented Aug 9, 2022

Rustin170506 commented Aug 10, 2022

Rustin170506 commented Aug 12, 2022 • edited Loading

RalfJung commented Aug 12, 2022

Rustin170506 commented Aug 12, 2022

RalfJung commented Aug 12, 2022

Rustin170506 commented Aug 13, 2022

Rustin170506 commented Sep 13, 2022

Rustin170506 commented Sep 13, 2022

RalfJung commented Jan 31, 2025 • edited Loading

RalfJung commented Jan 31, 2025

weihanglo commented Jan 31, 2025

RalfJung commented Jul 23, 2022 •

edited by rustbot

Loading

Rustin170506 commented Aug 9, 2022 •

edited

Loading

Rustin170506 commented Aug 9, 2022 •

edited

Loading

Rustin170506 commented Aug 12, 2022 •

edited

Loading

RalfJung commented Jan 31, 2025 •

edited

Loading