fix(cli): Control clap colors through config #13463

epage · 2024-02-20T17:25:59Z

What does this PR try to resolve?

Part of #9012

How should we test and review this PR?

To accomplish this, I pivoted in how we handle -C. In #11029, I made the config lazily loaded. Instead, we are now reloading the config for -C like we do for "cargo script" and cargo install. If there is any regression, it will be felt by those commands as well and we can fix all together. As this is unstable, if there is a regression, the impact is less. This allowed us access to the full config for getting the color setting, rather than taking more limited approaches like checking only CARGO_TERM_CONFIG.

Additional information

rustbot · 2024-02-20T17:26:04Z

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

bors · 2024-02-20T19:26:47Z

☔ The latest upstream changes (presumably #13464) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-02-20T19:59:34Z

☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts.

This reverts commit eda1652.

Fixes rust-lang#9012

src/cargo/util/config/mod.rs

src/bin/cargo/cli.rs

weihanglo

With this patch, the color control works for CARGO_TERM_COLOR environment variable and config.toml. However, it doesn't work with either --color never or --config 'term.color="never"', so I don't think #9012 is resolved.

Not sure if we should ship a partial solution or hold on until --color and --config get addressed.

weihanglo · 2024-02-21T18:15:40Z

src/bin/cargo/cli.rs

-pub fn cli() -> Command {
+pub fn cli(gctx: &GlobalContext) -> Command {
+    // Don't let config errors get in the way of parsing arguments
+    let term = gctx.get::<TermConfig>("term").unwrap_or_default();


The issue is that we sidestep without calling GlobalContext::merge_cli_args, which GlobalContext::configure does.

There might also be a discrepancy when we call reload_cwd(), but that is minimal as the code path eventually calls GlobalContext::configure.

I'm not too sure what the concern is here.

Is it that we aren't respecting --color which you raised elsewhere and you are just calling out the relevant code?

Or is it that we aren't respecting the config from -C to control the colors from CLI parsing? If so, that is a similar problem to --color support (and more difficult / brittle to support)

I was just making sure we're aware of this and also #9012 is not completely resolved. Should we reword #9012 to track --config and --color or open a new one?

I am good with merge this PR as-is btw. Feel free to r=weihanglo.

epage · 2024-02-21T18:24:33Z

Not sure if we should ship a partial solution or hold on until --color and --config get addressed.

I don't see a reasonable way for addressing that part. We 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.

weihanglo · 2024-02-21T18:31:27Z

src/bin/cargo/cli.rs

-pub fn cli() -> Command {
+pub fn cli(gctx: &GlobalContext) -> Command {
+    // Don't let config errors get in the way of parsing arguments
+    let term = gctx.get::<TermConfig>("term").unwrap_or_default();


I was just making sure we're aware of this and also #9012 is not completely resolved. Should we reword #9012 to track --config and --color or open a new one?

I am good with merge this PR as-is btw. Feel free to r=weihanglo.

epage · 2024-02-21T19:09:33Z

@bors r=weihanglo

bors · 2024-02-21T19:09:35Z

📌 Commit 6747425 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-02-21T19:11:09Z

⌛ Testing commit 6747425 with merge d325f9b...

bors · 2024-02-21T19:51:27Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing d325f9b to master...

bors · 2024-02-21T19:51:31Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing d325f9b to master...

bors · 2024-02-21T19:52:29Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

weihanglo · 2024-02-22T03:43:23Z

Seems like we can leverage #13461 to test colors for this PR, right?

weihanglo · 2024-02-22T03:56:34Z

Okay I didn't notice you've linked the original issue in that PR.

Update cargo 16 commits in 194a60b2952bd5d12ba15dd2577a97eed7d3c587..8964c8ccff6e420e2a38b8696d178d69fab84d9d 2024-02-21 01:53:45 +0000 to 2024-02-27 19:22:46 +0000 - feat: Add "-Zpublic-dependency" for public-dependency feature. (rust-lang/cargo#13340) - Stabilize global cache data tracking. (rust-lang/cargo#13492) - chore: bump baseline version requirement of sub crates (rust-lang/cargo#13494) - fix(doctest): search native libs in build script outputs (rust-lang/cargo#13490) - chore: fixed a typo(two->too) (rust-lang/cargo#13489) - test: relax help text assertion (rust-lang/cargo#13488) - refactor: clean up for `GlobalContext` rename (rust-lang/cargo#13486) - test(cli): Verify terminal styling (rust-lang/cargo#13461) - fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' (rust-lang/cargo#13479) - Error messages when collecting workspace members now mention the workspace root location (rust-lang/cargo#13480) - fix(add): Improve error when adding registry packages while vendored (rust-lang/cargo#13281) - [docs]:Add missing jump links (rust-lang/cargo#13478) - Add global_cache_tracker stability tests. (rust-lang/cargo#13467) - fix(cli): Control clap colors through config (rust-lang/cargo#13463) - chore: remove the unused function (rust-lang/cargo#13472) - Fix missing brackets (rust-lang/cargo#13470)

rustbot assigned ehuss Feb 20, 2024

epage force-pushed the term_color branch from 5a8ef3f to 426a93d Compare February 20, 2024 18:32

epage force-pushed the term_color branch from 426a93d to ae6920a Compare February 20, 2024 19:38

epage added 5 commits February 20, 2024 16:38

refactor(cli): Switch '-C' to using 'reload_rooted_at'

8434854

Revert "refactor(cli): Lazily do first-pass config loading"

e5fd597

This reverts commit eda1652.

refactor(shell): Group by type

4c3b0ad

refactor(shell): Allow reusing ColorChoice parsing

cf2cca9

fix(cli): Control clap colors through term.color

6747425

Fixes rust-lang#9012

epage force-pushed the term_color branch 2 times, most recently from 0998f84 to 6e6514a Compare February 21, 2024 01:27

weihanglo reviewed Feb 21, 2024

View reviewed changes

src/cargo/util/config/mod.rs Outdated Show resolved Hide resolved

src/bin/cargo/cli.rs Outdated Show resolved Hide resolved

epage force-pushed the term_color branch 2 times, most recently from cac6f70 to 6747425 Compare February 21, 2024 15:23

weihanglo reviewed Feb 21, 2024

View reviewed changes

weihanglo approved these changes Feb 21, 2024

View reviewed changes

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 Feb 21, 2024

epage mentioned this pull request Feb 21, 2024

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

Open

bors merged commit d325f9b into rust-lang:master Feb 21, 2024
21 checks passed

bors mentioned this pull request Feb 21, 2024

feat(tree): Control --charset via auto-detecting config value #13337

Merged

epage deleted the term_color branch February 21, 2024 20:17

weihanglo mentioned this pull request Feb 27, 2024

Update cargo rust-lang/rust#121699

Merged

ehuss added this to the 1.78.0 milestone Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): Control clap colors through config #13463

fix(cli): Control clap colors through config #13463

epage commented Feb 20, 2024 •

edited

Loading

rustbot commented Feb 20, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

weihanglo left a comment

weihanglo Feb 21, 2024

epage Feb 21, 2024

weihanglo Feb 21, 2024

epage commented Feb 21, 2024

weihanglo Feb 21, 2024

epage commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

weihanglo commented Feb 22, 2024

weihanglo commented Feb 22, 2024

fix(cli): Control clap colors through config #13463

fix(cli): Control clap colors through config #13463

Conversation

epage commented Feb 20, 2024 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Feb 20, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Feb 21, 2024

Choose a reason for hiding this comment

epage Feb 21, 2024

Choose a reason for hiding this comment

weihanglo Feb 21, 2024

Choose a reason for hiding this comment

epage commented Feb 21, 2024

weihanglo Feb 21, 2024

Choose a reason for hiding this comment

epage commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

bors commented Feb 21, 2024

weihanglo commented Feb 22, 2024

weihanglo commented Feb 22, 2024

epage commented Feb 20, 2024 •

edited

Loading