Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Feb 22, 2024
Merged

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

merged 5 commits into from
Feb 22, 2024

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
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

Thanks. This looks great 👍🏾

@bors r+

@bors
Copy link
Collaborator

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
Collaborator

bors commented Feb 22, 2024

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

@bors
Copy link
Collaborator

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
21 checks passed
@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