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

refactor: migrate to tracing #12458

Merged
merged 8 commits into from
Aug 7, 2023
Merged

refactor: migrate to tracing #12458

merged 8 commits into from
Aug 7, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Aug 7, 2023 •
edited
Loading

What does this PR try to resolve?

The migrates CARGO_LOG logging from log to tracing.

tracing is already widely used among the ecosystem and rustc adopted it for a while. We've chatted about this a while back in the weekly meeting.

Last week in the office hour, epage, Muscraft, and arlosi have no objection to this when I mentioned this upcoming PR. ehuss seems to looking forward to the migration #12445 (comment). Hence posted this pull request.

How should we test and review this PR?

By commits. And please check if CARGO_LOG works for you.

Largely, a search & replace worked just fine. Only in util/network/http.rs I did a manual fix.

The format is also a bit different. Should we make it compatible? (I don't want to though)

# before
[2023-08-07T10:53:48Z DEBUG cargo::sources::registry::http_remote] loading config
# after
2023-08-07T10:43:47.867106Z DEBUG cargo::sources::registry::http_remote: loading config

By default log enables all levels, whereas the built-in fmt subscriber in tracing-subscriber enables up to INFO only. Is that a thing we need to consider? I do feel like INFO is a better default.

Additional information

-2 dependencies
+11 dependencies

Last year during 1.63.0 release cargo only had ~180 deps, but today it doubles the number to ~337 🥲.

@rustbot
Copy link
Collaborator

rustbot commented Aug 7, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-cache-messages Area: caching of compiler messages A-cfg-expr Area: Platform cfg expressions A-cli Area: Command-line interface, option parsing, etc. A-configuration Area: cargo config files and env vars A-crate-dependencies Area: [dependencies] of any kind A-dep-info Area: dep-info, .d files A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-diagnostics Area: Error and warning messages generated by Cargo itself. A-features2 Area: issues specifically related to the v2 feature resolver A-future-incompat Area: future incompatible reporting A-git Area: anything dealing with git A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-layout Area: target output directory layout, naming, and organization A-lockfile Area: Cargo.lock issues A-manifest Area: Cargo.toml issues A-networking Area: networking issues, curl, etc. A-rebuild-detection Area: rebuild detection and fingerprinting A-registries Area: registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-sparse-registry Area: http sparse registries A-timings Area: timings labels Aug 7, 2023
@weihanglo weihanglo added A-debugging-cargo Area: debugging cargo itself and removed A-sparse-registry Area: http sparse registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. labels Aug 7, 2023
@epage
Copy link
Contributor

epage commented Aug 7, 2023

The format is also a bit different. Should we make it compatible? (I don't want to though)

Compatibility only matters if we intend people to parse it which I don't think we do.

The only other issue is usability which it is probably good enough

epage
epage reviewed Aug 7, 2023
View reviewed changes
Cargo.toml Show resolved Hide resolved
epage
epage reviewed Aug 7, 2023
View reviewed changes
Cargo.toml Show resolved Hide resolved
@weihanglo
Copy link
Member Author

I'd like to call out the other change. By default log enables all levels, whereas the built-in fmt subscriber in tracing-subscriber enables up to INFO only. Is that a thing we need to consider? I do feel like INFO is a better default.

@epage
Copy link
Contributor

epage commented Aug 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit 964c083 has been approved by epage

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 Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit 964c083 with merge b19934c...

bors added a commit that referenced this pull request Aug 7, 2023
@bors
Auto merge of #12458 - weihanglo:tracing, r=epage
b19934c 
refactor: migrate to `tracing`
@bors
Copy link
Contributor

bors commented Aug 7, 2023

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 7, 2023
@weihanglo
Copy link
Member Author

Timed out again…

@bors retry

@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 Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 7, 2023

⌛ Testing commit 964c083 with merge 211fd7e...

@bors
Copy link
Contributor

bors commented Aug 7, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 211fd7e to master...

@bors bors merged commit 211fd7e into rust-lang:master Aug 7, 2023
@weihanglo weihanglo deleted the tracing branch August 7, 2023 21:53
@weihanglo weihanglo mentioned this pull request Aug 13, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2023
@bors
Auto merge of rust-lang#114773 - weihanglo:update-cargo, r=weihanglo
1702d0f 
Update cargo

21 commits in d78bbf4bde3c6b95caca7512f537c6f9721426ff..7e9de3f4ec3708f500bec142317895b96131e47c
2023-08-03 12:58:25 +0000 to 2023-08-13 00:47:32 +0000
- feat: remove `--keep-going` from `cargo test/bench` (rust-lang/cargo#12478)
- chore: window-sys should be a platform-specific dependency (rust-lang/cargo#12483)
- docs: make the env var source of rerun-if-env-changed clearer (rust-lang/cargo#12482)
- doc: note the backward compatible `.cargo/credential` file exists (rust-lang/cargo#12479)
- Fix elided lifetime in associated const (rust-lang/cargo#12475)
- prompt the use of `--nocapture` flag if `cargo test` process is terminated via a signal. (rust-lang/cargo#12463)
- cargo-credential: reset stdin & stdout to the Console (rust-lang/cargo#12469)
- Fix cargo remove incorrectly removing used patches (rust-lang/cargo#12454)
- chore(gh): Expand update window (rust-lang/cargo#12466)
- Fix panic when enabling http.debug for certain strings (rust-lang/cargo#12468)
- fix(cli): Make `--help` easier to browse (rust-lang/cargo#11905)
- fix: preserve jobserver file descriptors on rustc invocation to get `TargetInfo` (rust-lang/cargo#12447)
- refactor: migrate to `tracing` (rust-lang/cargo#12458)
- docs: add example for cargo-credential (rust-lang/cargo#12461)
- Bail out an error when using cargo:: in custom build script (rust-lang/cargo#12332)
- Fix printing multiple warning messages for unused fields in [registries] table (rust-lang/cargo#12439)
- Update windows dependencies (rust-lang/cargo#12453)
- Rustfmt a let-else statement (rust-lang/cargo#12451)
- Add allow(internal_features) (rust-lang/cargo#12450)
- Update pretty_env_logger to 0.5 (rust-lang/cargo#12445)
- Remove build metadata from libgit2-sys dependency (rust-lang/cargo#12444)

r? `@ghost`
@weihanglo weihanglo mentioned this pull request Aug 13, 2023
Merged
@ehuss ehuss added this to the 1.73.0 milestone Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

@arlosi arlosi arlosi approved these changes

Assignees

@ehuss ehuss

Labels
A-debugging-cargo Area: debugging cargo itself 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.73.0
Development

Successfully merging this pull request may close these issues.
6 participants
@weihanglo @rustbot @epage @bors @arlosi @ehuss