patina_internal_depex: Report protocol present status#1552
Conversation
❌ QEMU Readiness Check Failedpatina-dxe-core-qemu needs to be updated before QEMU validation can run.
Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/27247084172 Dependencies
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a detailed “DEPEX evaluation report” logging path to help diagnose why drivers remain pending/not dispatched.
Changes:
- Added optional detailed pending-driver reporting in the PI dispatcher guarded by a log-enabled check.
- Introduced a
Depex::eval_with_reportAPI that logs DEPEX evaluation under a dedicated log target. - Switched DEPEX debug logs to use explicit log targets (default vs report).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| patina_dxe_core/src/pi_dispatcher.rs | Adds a “detail report” loop that logs pending drivers and evaluates DEPEX with reporting enabled. |
| core/patina_internal_depex/src/lib.rs | Adds report log target constants and routes DEPEX debug output through an overridable target via eval_with_report. |
yangrongwei
changed the title
yangrongwei
force-pushed
the
ray-report-depex-log
branch
from
aa8fa25 to
2726141
Compare
yangrongwei
force-pushed
the
ray-report-depex-log
branch
from
2726141 to
f02fd80
Compare
| match opcode { | ||
| Opcode::Push(guid, present) => { | ||
| log::debug!( | ||
| target: "patina_depex_report", |
There was a problem hiding this comment.
I think this information is generally useful, I don’t think it needs to be hidden behind a target
There was a problem hiding this comment.
"patina_depex_report" target removed.
Using the same target and level of the summary section, which is log::warn!(target: "patina_dxe_core"
|
@Javagedes @makubacki can one of you please review? |
I was out of office Mon & Tue. I'm reviewing now. |
| log::warn!( | ||
| target: "patina_dxe_core", | ||
| " {guid:?} : {present}" | ||
| ); |
There was a problem hiding this comment.
Having this at warn level coupled with the patina_dxe_core target is a code smell in terms of coupling. Basically, this assumes code exists a certain way in the code (patina_dxe_core) that uses this crate (patina_internal_depex) as a dependency.
This type of print should be at least info or a more verbose type if left at the patina_dxe_core target level.
But I really think this code should really just return something like an iterator of protocols present for a given Depex so all logging stays in the caller.
There was a problem hiding this comment.
Agreed. I just meant don't use a target at all for the logging. It will inherit the patina_internal_depex target if a consumer wants to filter it.
But, to Michael's point, I think that returning an iterator here and letting the caller print it works well.
| // Detail report | ||
| log::warn!("Begin Detail Report:"); | ||
| for driver in self.dispatcher_context.lock().pending_drivers.iter() { | ||
| log::warn!( | ||
| "Driver {} ({:?}) found but not dispatched. Protocols present:", | ||
| driver.name.as_deref().unwrap_or("Unnamed"), | ||
| guid_fmt!(driver.file_name) | ||
| ); | ||
|
|
||
| match &driver.depex { | ||
| Some(depex) => { | ||
| depex.report_protocol_present(); | ||
| } | ||
| _ => { | ||
| log::warn!(" No Depex"); | ||
| } | ||
| } | ||
| } | ||
| log::warn!("End Detail Report."); | ||
| } |
There was a problem hiding this comment.
The messages in this "detailed report" should be at info level or higher. I think debug level.
The normal case of a driver not dispatched is already covered by warn above. This is debug information.
| } | ||
|
|
||
| // Detail report | ||
| log::warn!("Begin Detail Report:"); |
There was a problem hiding this comment.
"Begin Detail Report" is too ambiguous. Please change to something like "Begin Report of Drivers Not Dispatched".
|
@yangrongwei, can you please put an example of output in the PR description to help reviewers and those reading the release notes (that come from the PR description) to know what to expect? |
|
@yangrongwei, please rebase your branch locally for your next push. It is 59 commits behind now and that should allow it to run against the latest CI checks. |
Description
pi_dispatcher.rsreports a detailed dependency list for each "found but not dispatched" driver.How This Was Tested
Tested on real hardware.
Integration Instructions
No action.