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

Always compile rustdoc with debug logging enabled when download-rustc is set #81932

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 9, 2021

Previously, logging at DEBUG or below would always be silenced, because
rustc compiles tracing with the static_max_level_info feature. That
makes sense for release artifacts, but not for developing rustdoc.

Instead, this compiles two different versions of tracing: one in the
release artifacts, distributed in the sysroot, and a new version
compiled by rustdoc. Since rustc_driver is always linked to the
version of sysroot, this copy/pastes init_env_logging into rustdoc.

To avoid compiling an unnecessary version of tracing when
download-rustc isn't set, this adds a new using-ci-artifacts
feature for rustdoc and passes that feature in bootstrap.

Addresses #81930. This builds on #79540.

r? @Mark-Simulacrum

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Feb 9, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2021
@Mark-Simulacrum
Copy link
Member

(Looks like this could use a rebase; but I should be able to review 2nd and 3rd commits anyway)

@jyn514
Copy link
Member Author

jyn514 commented Feb 9, 2021

Looks like this could use a rebase

Done :) #79540 just got merged 🎉

@Mark-Simulacrum
Copy link
Member

Does it actually take long to compile tracing? What is our win here wall clock wise vs. just always having rustdoc depend on tracing on its own?

@jyn514
Copy link
Member Author

jyn514 commented Feb 13, 2021

You're right, it doesn't have much effect. It goes (on my machine) from 55 seconds to 60, which seems reasonable given it's a one-time cost. timings.zip

However that means that rustdoc will either
a) never get logging from rustc (never call rustc_driver::init_env_logger), which IMO makes the cure worse than the disease, or
b) get warnings that rustc logging is disabled whenever debug-logging = false is set, even if rustdoc logging is enabled (always call rustc_driver::init_env_logger).

@Mark-Simulacrum
Copy link
Member

b seems.. eminently reasonable though? If someone is requesting logging in a generic way that (might) include rustc, giving them a warning seems pretty good. We should endeavor to make the warning talk about rustc specifically but IIRC that was hard, so we'll just want to document it in debug-logging's comment in config.toml.example.

@jyn514
Copy link
Member Author

jyn514 commented Feb 13, 2021

b seems.. eminently reasonable though? If someone is requesting logging in a generic way that (might) include rustc, giving them a warning seems pretty good. We should endeavor to make the warning talk about rustc specifically but IIRC that was hard, so we'll just want to document it in debug-logging's comment in config.toml.example.

Hmm, ok, that sounds useful. The warning looks like this:

> RUSTDOC_LOG=debug rustdoc >/dev/null
warning: some trace filter directives would enable traces that are disabled statically
 | `debug` would enable the DEBUG level for all targets
 = note: the static max level is `info`
 = help: to enable DEBUG logging, remove the `max_level_info` feature

which seems ok - not great, but ok. Unfortunately I can't test much until the beta compiler gets bumped (I get the LLVM linker error from #81930).

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 24, 2021

I updated this to compile tracing twice unconditionally. It works much better than I expected :) even when both versions are active and have DEBUG logging enabled, you still see each log exactly once.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2021
@jyn514 jyn514 mentioned this pull request Feb 24, 2021
13 tasks
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Feb 24, 2021

Hmm, so the test failure here is odd. What happened is that

#![doc(no_default_passes, passes = "collapse-docs unindent-comments")]
tells rustdoc to run the collapse-docs pass, which no longer exists (it was removed in #80261). Rustdoc doesn't actually give a proper diagnostic here; instead it prints an error! log. I think now that tracing is compiled unconditionally, the log is now being emitted by default, because it's at the error level?

Do you think printing error logging by default is a good change? If so, I can just bless the test output. If not I'll look into why it happens, I'm not 100% sure right now.

Either way, I think rustdoc shouldn't be using error! logging and should go through normal Diagnostics instead. I'll make that change in a separate PR (and change the test not to use a removed pass at the same time).

@Mark-Simulacrum
Copy link
Member

I agree that rustdoc should not use error! to report errors to downstream users. That said for presumably an unstable feature that only std uses, if the error is not seen outside tests, I think it'd be fine to just bless, but it'd also be fine to comment out the error! line or adjust the default level.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2021
@jyn514
Copy link
Member Author

jyn514 commented Mar 1, 2021

That said for presumably an unstable feature that only std uses, if the error is not seen outside tests, I think it'd be fine to just bless

Unfortunately this is stable, although it's deprecated:

rust/src/librustdoc/core.rs

Lines 590 to 595 in 43fc1b3

if name == sym::no_default_passes {
report_deprecated_attr("no_default_passes", diag);
if default_passes == passes::DefaultPassOption::Default {
default_passes = passes::DefaultPassOption::None;
}
}

That said, I don't know anyone using this in practice. For now I'll update the test not to use a removed pass so rustdoc doesn't emit the warning.

…c` is set

Previously, logging at DEBUG or below would always be silenced, because
rustc compiles tracing with the `static_max_level_info` feature. That
makes sense for release artifacts, but not for developing rustdoc.

Instead, this compiles two different versions of tracing: one in the
release artifacts, distributed in the sysroot, and a new version
compiled by rustdoc. Since `rustc_driver` is always linked to the
version of sysroot, this copy/pastes `init_env_logging` into rustdoc.

The builds the second version of tracing unconditionally; see the code
for details on why.
`src/test/rustdoc-ui/deprecated-attrs.rs`
tells rustdoc to run the `collapse-docs` pass, which no longer exists
(it was removed in rust-lang#80261).
Rustdoc doesn't actually give a proper diagnostic here; instead it
prints an `error!` log. Now that tracing is compiled unconditionally,
the log is now being emitted by default, because it's at the error
level.

rustdoc shouldn't be using `error!` logging for diagnostics in the first
place, but in the meantime this change gets the testsuite to pass.
@jyn514 jyn514 mentioned this pull request Mar 1, 2021
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Mar 1, 2021

📌 Commit 6dc9934 has been approved by Mark-Simulacrum

@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 Mar 1, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#80734 (check that first arg to `panic!()` in const is `&str`)
 - rust-lang#81932 (Always compile rustdoc with debug logging enabled when `download-rustc` is set)
 - rust-lang#82018 (Remove the dummy cache in `DocContext`; delete RenderInfo)
 - rust-lang#82598 (Check stability and feature attributes in rustdoc)
 - rust-lang#82655 (Highlight identifier span instead of whole pattern span in `unused` lint)
 - rust-lang#82662 (Warn about unknown doc attributes)
 - rust-lang#82676 (Change twice used large const table to static)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 22ebb86 into rust-lang:master Mar 2, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 2, 2021
@jyn514 jyn514 deleted the rustdoc-logging branch March 2, 2021 13:51
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 6, 2021
…eGomez

Cleanup rustdoc warnings

## Clean up error reporting for deprecated passes

Using `error!` here goes all the way back to the original commit, rust-lang#8540. I don't see any reason to use logging; rustdoc should use diagnostics wherever possible. See rust-lang#81932 (comment) for further context.

- Use spans for deprecated attributes
- Use a proper diagnostic for unknown passes, instead of error logging
- Add tests for unknown passes
- Improve some wording in diagnostics

##  Report that `doc(plugins)` doesn't work using diagnostics instead of `eprintln!`

This also adds a test for the output.

This was added in rust-lang#52194. I don't see any particular reason not to use diagnostics here, I think it was just missed in rust-lang#50541.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 6, 2021
…eGomez

Cleanup rustdoc warnings

## Clean up error reporting for deprecated passes

Using `error!` here goes all the way back to the original commit, rust-lang#8540. I don't see any reason to use logging; rustdoc should use diagnostics wherever possible. See rust-lang#81932 (comment) for further context.

- Use spans for deprecated attributes
- Use a proper diagnostic for unknown passes, instead of error logging
- Add tests for unknown passes
- Improve some wording in diagnostics

##  Report that `doc(plugins)` doesn't work using diagnostics instead of `eprintln!`

This also adds a test for the output.

This was added in rust-lang#52194. I don't see any particular reason not to use diagnostics here, I think it was just missed in rust-lang#50541.
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 6, 2021
…eGomez

Cleanup rustdoc warnings

## Clean up error reporting for deprecated passes

Using `error!` here goes all the way back to the original commit, rust-lang#8540. I don't see any reason to use logging; rustdoc should use diagnostics wherever possible. See rust-lang#81932 (comment) for further context.

- Use spans for deprecated attributes
- Use a proper diagnostic for unknown passes, instead of error logging
- Add tests for unknown passes
- Improve some wording in diagnostics

##  Report that `doc(plugins)` doesn't work using diagnostics instead of `eprintln!`

This also adds a test for the output.

This was added in rust-lang#52194. I don't see any particular reason not to use diagnostics here, I think it was just missed in rust-lang#50541.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 6, 2021
…eGomez

Cleanup rustdoc warnings

## Clean up error reporting for deprecated passes

Using `error!` here goes all the way back to the original commit, rust-lang#8540. I don't see any reason to use logging; rustdoc should use diagnostics wherever possible. See rust-lang#81932 (comment) for further context.

- Use spans for deprecated attributes
- Use a proper diagnostic for unknown passes, instead of error logging
- Add tests for unknown passes
- Improve some wording in diagnostics

##  Report that `doc(plugins)` doesn't work using diagnostics instead of `eprintln!`

This also adds a test for the output.

This was added in rust-lang#52194. I don't see any particular reason not to use diagnostics here, I think it was just missed in rust-lang#50541.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 7, 2021
…eGomez

Cleanup rustdoc warnings

## Clean up error reporting for deprecated passes

Using `error!` here goes all the way back to the original commit, rust-lang#8540. I don't see any reason to use logging; rustdoc should use diagnostics wherever possible. See rust-lang#81932 (comment) for further context.

- Use spans for deprecated attributes
- Use a proper diagnostic for unknown passes, instead of error logging
- Add tests for unknown passes
- Improve some wording in diagnostics

##  Report that `doc(plugins)` doesn't work using diagnostics instead of `eprintln!`

This also adds a test for the output.

This was added in rust-lang#52194. I don't see any particular reason not to use diagnostics here, I think it was just missed in rust-lang#50541.
@jyn514 jyn514 added the A-download-rustc Area: The `rust.download-rustc` build option. label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-download-rustc Area: The `rust.download-rustc` build option. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants