Skip to content

Tell cargo to track logging environment variables #82763

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

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 4, 2021

Previously, changing the value of RUSTC_LOG would not rebuild, and,
because cargo caches output, show exactly the same logging as before.
This was confusing. Emit RUSTC_LOG with other depinfo dependencies
so cargo knows to rebuild when it's changed.

Note that this only adds RUSTC_LOG to depinfo. Unfortunately, adding
RUSTDOC_LOG only when running as rustdoc doesn't help because rustdoc
doesn't emit depinfo at all. Unconditionally emitting RUSTDOC_LOG in the
depinfo doesn't help because cargo ignores it for everything except
rustc itself. I am not yet sure how to fix this; see
rust-lang/cargo#8374 for more info.

@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-contributor-roadblock Area: Makes things more difficult for new or seasoned contributors to Rust labels Mar 4, 2021
@rust-highfive
Copy link
Contributor

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2021
Previously, changing the value of `RUSTC_LOG` would not rebuild, and,
because cargo caches output, show exactly the same logging as before.
This was confusing. Emit `RUSTC_LOG` with other `depinfo` dependencies
so cargo knows to rebuild when it's changed.

Note that this only adds RUSTC_LOG to depinfo.  Unfortunately, adding
RUSTDOC_LOG only when running as rustdoc doesn't help because rustdoc
doesn't emit depinfo at all. Unconditionally emitting RUSTDOC_LOG in the
depinfo doesn't help because cargo ignores it for everything except
rustc itself. I am not yet sure how to fix this; see
rust-lang/cargo#8374 for more info.
@cuviper
Copy link
Member

cuviper commented Mar 4, 2021

Alternatively, could you just add a build script emitting rerun-if-env-changed?

Wait, no, you don't want to rebuild the compiler, just rerun it for any given project. Nevermind me.

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

Wait, no, you don't want to rebuild the compiler, just rerun it for any given project. Nevermind me.

Right - I want this to work for arbitrary projects, not just projects I work on a lot.

@Aaron1011
Copy link
Member

I don't think this is a good idea. RUSTC_LOG is most useful when debugging the compiler, where it's important to have tight control over when the compiler is invoked.

For example, when debugging an ICE in a project with a large number of dependencies, it can be very useful to repeatedly rebuild the root crate while varying RUSTC_LOG. With this change, the entire dependency graph will be rebuilt, greatly slowing down debugging. If the crash is due to a bug in incremental compilation, then enabling logging will cause the incr cache to be rebuilt, hiding the bug.

@jyn514
Copy link
Member Author

jyn514 commented Mar 5, 2021

With this change, the entire dependency graph will be rebuilt

Oh good catch, I hadn't thought of that. Is there a way to tell cargo to rebuild only the outermost crate (or maybe just path dependencies)? If not I guess cargo would have to special case logging.

@cuviper
Copy link
Member

cuviper commented Mar 5, 2021

cargo rustc might be what you want. There's a cargo rustdoc too.

@jyn514
Copy link
Member Author

jyn514 commented Mar 5, 2021

cargo rustc might be what you want. There's a cargo rustdoc too.

Right, I know about both, but that doesn't help with being surprised that RUSTC_LOG=info has no output, or has cached and incorrect output. See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/RUSTC_LOG.20output.20get.20somehow.20.22recorded.22 - this is especially bad when going through bootstrap, because there's no clear way to invalidate the cache.

@petrochenkov
Copy link
Contributor

because there's no clear way to invalidate the cache

touch compiler/rustc_crate_of_interest/src/lib.rs

?

@petrochenkov
Copy link
Contributor

Regarding putting env vars to depinfo in general, rustc uses many env vars, I guess for some of them you want to rerun the compiler on change, but for some you do not.
I certainly wouldn't want variables like SOMETHING_COLOR cause rebuilds.

I'm not sure who should make this choice, but I suspect it would be better to leave this job to a higher level tool (like cargo).
rustc could simply write all such variables to depinfo, using a separate format, e.g. # rustc-env-dep instead of # env-dep, and leave it to others to decide what to do with them.

We already have functions for tracked access to env variables in proc macros (tracked_env::var) and rustc build scripts (tracked_env_var_os), rustc itself could get such function as well.
We could even catch uses of untracked env::var(_os)s with an internal lint.

@petrochenkov petrochenkov 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 5, 2021
@Aaron1011
Copy link
Member

I was debugging some incremental compilation issues the other day, and this change would have made doing do unecessarily difficult. Once I got to an incremental-related ICE, I was able to re-run the compiler with different RUSTC_LOG values, which would repeatedly trigger the ICE with different log output. With this change, I would need to set up the corrupted state each time I changed RUSTC_LOG

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 or seasoned contributors to Rust S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants