Cargo doesn't respect `--color` or `CARGO_TERM_COLOR` for help text and errors from clap

[mchernyavsky]

Problem

^title

I expect that with --color=always option (or CARGO_TERM_COLOR=always environment variable) the output will contain expected ANSI escape codes.

Steps

1. Run cargo build --color=always --unknown &> out.txt
2. Theout.txt will contain the following output (without ANSI escape codes):

error: Found argument '--unknown' which wasn't expected, or isn't valid in this context

USAGE:
    cargo build --color <WHEN>

For more information try --help

Notes

Output of cargo version:

cargo 1.50.0-nightly (a3c2627fb 2020-12-14)

[epage]

The main problem here is the circular nature of this.

• To read --color=always, we need to parse the CLI but the error is coming from our CLI parser
• To read CARGO_TERM_COLOR, we need the Config configured which requires the CLI to be parsed.

We can't workaround it for --color=always but we can for CARGO_TERM_COLOR by directly looking up the value and passing that to clap. We'd probably want to extract the value parser out of set_color_choice into a std::str::FromStr impl on ColorChoice.

This would make it so fn cli() would have something like

    let color_choice = std::env::var_os("CARGO_TERM_COLOR");
    let color_choice = color_choice.and_then(|c| c.parse::<ColorChoice>().ok());
    let color_choice = match color_choice {
        Some(ColorChoice::CargoAuto) | None => clap::ColorChoice::Auto,
        Some(ColorChoice::Always) => clap::ColorChoice::Always,
        Some(ColorChoice::Never) => clap::ColorChoice::Never,
    };

This intentionally ignores the errors since it isn't worth reporting at this stage.

The main question is if this is worth it or not.

[epage]

#13463 resolves the config side of this but leaves --color alone.

To handle --color, we'd need to be able to parse the command line to get those which requires knowing what level of styling to use. We could say "parse the command line, take the result, and strip the message based on our own decision". However, --help and errors short-circuits CLI parsing. We'd have to parse, see if an error gets back, and then re-parse using the "ignore errors" setting which is a very brittle feature in clap.

I'd propose that we reject --color and close this as "resolved" with the merge of #13463

[weihanglo]

I'd propose that we reject --color and close this as "resolved" with the merge of #13463

That sounds fair. Should we document this unfortunate truth somewhere?

[epage]

I untagged it; unsure why github closed it.

[epage]

Should we document this unfortunate truth somewhere?

I'm for that but unsure where it would work that it both helps the user and doesn't overwhelm them with details when they don't care (CLI parsing is a bit niche).

[weihanglo]

I untagged it; unsure why github closed it.

https://github.com/rust-lang/cargo/pull/13463/commits/67474259e9893fdfa3a9f98148dc871e3c970e31

The close keyword also works in commit messages 😈.

[jtmoon79]

FWIW cargo bench still prints with color with despite CARGO_TERM_COLOR=never and --color=never.

Steps to reproduce

For some project with benches

$ CARGO_TERM_COLOR=never cargo bench --benches --quiet --color=never

I used my project. Using cargo 1.79.0 on Ubuntu 22, target x86_64-unknown-linux-gnu.

[weihanglo]

@jtmoon79 thanks for the report. That is a different issue than this. Test harness (by default it's libtest from rustc) usually provide their own way to customize console output. Those arguments could be passed after double dashes --, e.g. cargo test -- --nocapture. Cargo makes no assumption to test/benchmark harness so it doesn't control the color of the output. See https://github.com/rust-lang/cargo/issues/1983 for the relevant issue.