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.

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

refactor(cli): Lazy load config #11029

Merged
merged 3 commits into from
Sep 1, 2022
Merged

refactor(cli): Lazy load config #11029

merged 3 commits into from
Sep 1, 2022

Conversation

epage
Copy link
Contributor

@epage epage commented Aug 29, 2022

This is trying to clarify -C support when it is implemented in #10952

Cargo currently has two initialization states for Config, Config::default (process start) and config.configure (after parsing args). The most help we provide for a developer touching this code is a giant CAUTION comment in one of the relevant functions.

Currently, #10952 adds another configuration state in the middle where the current_dir has been set.

The goal of this PR is to remove that third configuration state by

  • Lazy loading Config::default so it can be called after parsing -C
  • Allowing -C support to assert that the config isn't loaded yet to catch bugs with it

The hope is this will make the intent of the code clearer and reduce the chance for bugs.

In doing this, there are two intermediate refactorings

  • Make help behave like other subcommands
    • Before, we had hacks to intercept raw arguments and to intercept clap errors and assume what their intention was to be able to implement our help system.
    • This flips it around and makes help like any other subcommand,
      simplifying cargo initialization.
    • We had to upgrade clap because this exposed a bug where Command::print_help wasn't respecting disable_colored_help(true)
  • Delay fix's access to config

Personally, I also find both changes make the intent of the code clearer.

To review this, I recommend looking at the individual commits. As this is just refactors, this has no impact on testing.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2022
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.

Clever solution for lazy loading! Thank you!

src/bin/cargo/commands/mod.rs Outdated Show resolved Hide resolved
src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
src/bin/cargo/cli.rs Show resolved Hide resolved
epage added 3 commits August 31, 2022 19:18
Before, we had hacks to intercept raw arguments and to intercept clap
errors and assume what their intention was to be able to implement our
help system.

This flips it around and makes help like any other subcommand,
simplifying cargo initialization.
My hope is to make it so we can lazy load the config.  This makes it so
we only load the config for the fix proxy if needed.

I also feel like this better clarifies the intention of the code that we
are running in a special mode.
This will be a help for cases like rust-lang#10952 which I would expect would
assert that the config is not loaded before changing the current_dir.
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.

👍🏾

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit eda1652 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 Sep 1, 2022
@bors
Copy link
Contributor

bors commented Sep 1, 2022

⌛ Testing commit eda1652 with merge 04a4081...

@bors
Copy link
Contributor

bors commented Sep 1, 2022

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

@bors bors merged commit 04a4081 into rust-lang:master Sep 1, 2022
@epage epage deleted the config branch September 1, 2022 11:45
@lqd
Copy link
Member

lqd commented Sep 1, 2022

@weihanglo
Copy link
Member

I think this PR introduced this warning https://github.com/rust-lang/cargo/runs/8128683225?check_suite_focus=true#step:10:257 ?

It might eventually be used in this patch #10952 (comment), though I cannot guarantee when the author will pick it up. If the warning is annoying, we can remove the function for sure.

@lqd
Copy link
Member

lqd commented Sep 1, 2022

Oh not annoying, I was mostly surprised as I was seeing "deny warnings" features and wondered if CI had an issue checking them 😅

weihanglo added a commit to rust-lang/rust that referenced this pull request Sep 5, 2022
8 commits in 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5..646e9a0b9ea8354cc409d05f10e8dc752c5de78e
2022-08-27 18:41:39 +0000 to 2022-09-02 14:29:28 +0000
- Support inheriting jobserver fd for external subcommands (rust-lang/cargo#10511)
- refactor(cli): Lazy load config (rust-lang/cargo#11029)
- chore: Don't show genned docs in ripgrep (rust-lang/cargo#11040)
- Document private items for Cargo and publish under contributor guide (rust-lang/cargo#11019)
- Add names to CI jobs (rust-lang/cargo#11039)
- Rework test error handling (rust-lang/cargo#11028)
- Very slight `cargo add` documentation improvements (rust-lang/cargo#11033)
- Update compiling requirements. (rust-lang/cargo#11030)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2022
Update cargo

8 commits in 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5..646e9a0b9ea8354cc409d05f10e8dc752c5de78e
2022-08-27 18:41:39 +0000 to 2022-09-02 14:29:28 +0000
- Support inheriting jobserver fd for external subcommands (rust-lang/cargo#10511)
- refactor(cli): Lazy load config (rust-lang/cargo#11029)
- chore: Don't show genned docs in ripgrep (rust-lang/cargo#11040)
- Document private items for Cargo and publish under contributor guide (rust-lang/cargo#11019)
- Add names to CI jobs (rust-lang/cargo#11039)
- Rework test error handling (rust-lang/cargo#11028)
- Very slight `cargo add` documentation improvements (rust-lang/cargo#11033)
- Update compiling requirements. (rust-lang/cargo#11030)
@Muscraft Muscraft mentioned this pull request Sep 13, 2022
@ehuss ehuss added this to the 1.65.0 milestone Sep 21, 2022
bors added a commit that referenced this pull request Feb 21, 2024
fix(cli): Control clap colors through config

### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants