-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fix unknown-crate
when using -Z self-profile with rustdoc
#79586
Conversation
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
I don't know why this is going wrong :/ the only behavior I changed in rustc was https://github.com/rust-lang/rust/pull/79586/files#diff-5d2a824073d4fcc6b4e60bae734988e6e37f6483fff54626ab3352774d2c55cfL188, which reads |
Actually I might know the issue - Is it intended that rustc looks at rust/compiler/rustc_session/src/output.rs Lines 54 to 59 in b7ebc6b
So I guess |
let parse_result = self.parse()?; | ||
let krate = parse_result.peek(); | ||
// parse `#[crate_name]` even if `--crate-name` was passed, to make sure it matches. | ||
find_crate_name(self.session(), &krate.attrs, &self.compiler.input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I am a bit worried -- is this a breaking change? It sounds like it would be, since we presumably weren't validating previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #79586 (comment) - this is the same behavior as before, just easier to understand. Previously, the crate name would never be Some so this would always run.
@@ -400,6 +400,7 @@ impl SelfProfiler { | |||
) -> Result<SelfProfiler, Box<dyn Error + Send + Sync>> { | |||
fs::create_dir_all(output_directory)?; | |||
|
|||
debug!(?crate_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't want this debug statement, or at least, we don't want it in the current form 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the policy on debug logging is - I'm happy to revert this, but I'm not sure what a better form would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general we try to include some kind of context about what is going on when this value is logged. Seeing for example
[DEBUG] rustc_data_structures::profiling my_crate_name`
isn't super useful but something like debug!("Creating self-profiler: output_directory = {:?}, crate_name = {:?}, event_filters = {:?}", output_directory, crate_name, event_filters);
would add more context about what is being logged and could be useful for someone else in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, what does the prefixed ?
do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... by removing a duplicate `crate_name` field in `interface::Config`, making it clear that rustdoc should be passing it to `config::Options` instead.
LGTM and I think the review comments have been addressed: @bors r+ |
📌 Commit 878cfb5 has been approved by |
☀️ Test successful - checks-actions |
... by removing a duplicate
crate_name
field ininterface::Config
,making it clear that rustdoc should be passing it to
config::Options
instead.Unblocks rust-lang/rustc-perf#797.