Skip to content

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp'#13479

Merged
bors merged 5 commits intorust-lang:masterfrom
epage:cfg
Feb 22, 2024
Merged

fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp'#13479
bors merged 5 commits intorust-lang:masterfrom
epage:cfg

Conversation

@epage
Copy link
Contributor

@epage epage commented Feb 22, 2024

What does this PR try to resolve?

Similar to #9012, we aren't respecting CARGO_TERM_COLOR for -Zhelp and other places. This corrects that.

How should we test and review this PR?

#9012 was about initialization order to get the value. Here, the problem is that we don't update Shell with CARGO_TERM_COLOR.

In doing this, I was concerned about keeping track of where it is safe to call config_configure without running it twice. To this end, I refactored main to make it clear that each call to config_configure is in a mutually exclusive path that exists immediately.

Additional information

Found this with the test for -Zhelp in #13461.

epage added 3 commits February 21, 2024 21:19
@epage
refactor(cli): Shrink main
ad85ef4 
I'm going to add more `config_configure`s and this will make it more
obvious that these are done in early returns.
@epage
refactor(cli): Make it obvious that the cases are mutually exclusive
808b4f6
@epage
refactor(cli): Allow calling config_configure without Exec
1834244
@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2024
weihanglo
weihanglo reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

The question is that how resilient to broken .cargo/config.toml these commands should be. Calling config_configure implies they fail if .cargo/config.toml is ill-formatted.

@rustbot rustbot added the A-configuration Area: cargo config files and env vars label Feb 22, 2024
@epage
Copy link
Contributor Author

epage commented Feb 22, 2024

The question is that how resilient to broken .cargo/config.toml these commands should be. Calling config_configure implies they fail if .cargo/config.toml is ill-formatted.

This was caught in test failures and I've hardened it to make it work

  • We make sure we initialize the Shell before erroring out
  • We ignore the errors

@weihanglo
Copy link
Member

weihanglo commented Feb 22, 2024

Thanks. This looks great 👍🏾

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2024

📌 Commit 2ec0461 has been approved by weihanglo

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 Feb 22, 2024
@bors
Copy link
Contributor

bors commented Feb 22, 2024

⌛ Testing commit 2ec0461 with merge 98079d9...

@bors
Copy link
Contributor

bors commented Feb 22, 2024

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

@bors bors merged commit 98079d9 into rust-lang:master Feb 22, 2024
@epage epage deleted the cfg branch February 23, 2024 03:03
@weihanglo weihanglo mentioned this pull request Feb 27, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2024
@bors
Auto merge of rust-lang#121699 - weihanglo:update-cargo, r=weihanglo
fb0c80b 
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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 28, 2024
@bors
Auto merge of rust-lang#121699 - weihanglo:update-cargo, r=weihanglo
d377991 
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)
@ehuss ehuss added this to the 1.78.0 milestone Mar 2, 2024
This was referenced May 31, 2024
Closed
Merged
bors added a commit that referenced this pull request May 31, 2024
@bors
Auto merge of #13992 - epage:config, r=weihanglo
7a6fad0 
fix(config): Ensure `--config net.git-fetch-with-cli=true` is respected

### What does this PR try to resolve?

#13479 changed the global context initialization order so that command line stuff is processed after we read some config.
This had a side effect of breaking `--config net.git-fetch-with-cli=true`.
I reverted the change to restore support for `--config`.

Fixes #13991

### How should we test and review this PR?

### Additional information

This reverts commit f525e1f.

This removes color control from warnings for unstable features.
For some reason this removed color support from `cargo -Zhelp` in the
tests but I can't reproduce it locally.

The most important thing was getting the config fix in.
There are two follow ups
- Can we have the config working *and* color?
- Why did this fail for this field and not the others we already had
  tests for?

I ran out my immediate time box for looking into these.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels

A-cli Area: Command-line interface, option parsing, etc. 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.78.0

Development

Successfully merging this pull request may close these issues.

5 participants

@epage @rustbot @weihanglo @bors @ehuss