fix(cargo-wdk): use --message-format=json-render-diagnostics in the build command to render compiler errors instead of the plain json option#645
Conversation
--message-format=json-render-diagnostics in the build command to show compiler errors instead of json--message-format=json-render-diagnostics in the build command to render compiler errors instead of the plain json option
There was a problem hiding this comment.
Pull request overview
Updates cargo-wdk’s build invocation and error handling so compiler diagnostics are shown to users/tests in a readable form (instead of being buried in JSON), addressing the debugging pain described in #613.
Changes:
- Switch
cargo buildto--message-format=json-render-diagnosticsso diagnostics are rendered tostderrwhile keeping parseable JSON onstdout. - Simplify
BuildTaskError::CargoBuildto store a descriptiveStringmessage instead of the fullCommandError. - Update unit/integration tests to expect the new cargo argument and error shape.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/cargo-wdk/src/actions/build/tests.rs | Updates expected cargo args in build-action tests to use the new message format. |
| crates/cargo-wdk/src/actions/build/error.rs | Changes BuildTaskError::CargoBuild payload from CommandError to String and updates its display format. |
| crates/cargo-wdk/src/actions/build/build_task.rs | Updates cargo args to json-render-diagnostics and maps command failures to a simplified compilation-failed message; adjusts unit tests accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map_err(|_| { | ||
| BuildTaskError::CargoBuild("Compilation failed. See compiler errors above.".into()) |
There was a problem hiding this comment.
map_err(|_| ...) discards the underlying CommandError, so a cargo invocation failure (e.g., IoError when cargo isn’t found) will be reported as “Compilation failed”, which is misleading and makes debugging harder (especially since the CLI logs only Display). Consider matching on CommandError and producing a different message for IoError (including the io::Error), while keeping the simplified message for CommandFailed compilation errors.
| .map_err(|_| { | |
| BuildTaskError::CargoBuild("Compilation failed. See compiler errors above.".into()) | |
| .map_err(|err| match err { | |
| crate::providers::exec::CommandError::IoError(err) => { | |
| BuildTaskError::CargoBuild(format!("Failed to invoke cargo build: {err}")) | |
| } | |
| crate::providers::exec::CommandError::CommandFailed(_) => { | |
| BuildTaskError::CargoBuild( | |
| "Compilation failed. See compiler errors above.".into(), | |
| ) | |
| } |
There was a problem hiding this comment.
@svasista-ms This is important. We should return "Compilation failed" only in the case of a compilation error. We should return something else in all other cases as shown above by copilot.
Do have a test for cargo failures that are not due to compilation errors? If not then we should add them.
| #[error("Error running cargo build command: {0}")] | ||
| CargoBuild(String), |
There was a problem hiding this comment.
Changing BuildTaskError::CargoBuild to hold only a String removes error chaining/source context from the original CommandError (notably the IoError case). If keeping the simplified user message for compile failures, consider adding a separate variant or optional #[source] field so non-compilation execution failures still surface actionable details.
There was a problem hiding this comment.
Yes, we should not change it to string as we need to also support non-compilation-error failures (see the other comment).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #645 +/- ##
==========================================
+ Coverage 78.44% 78.46% +0.01%
==========================================
Files 25 25
Lines 5201 5196 -5
Branches 5201 5196 -5
==========================================
- Hits 4080 4077 -3
+ Misses 1001 999 -2
Partials 120 120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes #613
Problem
When
cargo wdk buildencounters a compilation error, the error output is hard to debug:--message-format=jsonsends even the compiler diagnostics as JSON tostdoutinstead of redirecting it tostderr. So, compiler errors are not directly visible instead they are buried deep in the output json. This is affecting test outputs because in case of failures, the errors cannot be seen easily in thejsonoutput.BuildTaskError::CargoBuildwraps the fullCommandError(containingstdoutJSON), even though the user may have already seen the real compiler errors onstderrin real time.Fixes
The following changes fix the above problems:
Switch to
--message-format=json-render-diagnosticsThis option instructs cargo to render diagnostics from
rustcdirectly tostderrinstead of putting it in thejsonoutput.stdout: only machine-parseable JSON (artifacts, build-finished) — no diagnostic messages
stderr: human-readable compiler errors and warnings, rendered by Cargo (visible to the user in real time since stderr is inherited)
Replace
CommandErrorwrapping with a clean message:When cargo build fails,
BuildTask::run()now maps the error to a simple "Compilation failed. See compiler errors above." string instead of forwarding the rawCommandError(which contained unhelpful JSON fromstdout).Change
BuildTaskError::CargoBuildto holdStringinstead ofCommandErrorSince we no longer need the structured error from the command, the variant now holds a descriptive String message.
Before
Running
kmdf_driver_cross_compiles_with_cli_option_successfullytest after removingaarch64-pc-windows-msvctarget:Compiler diagnostics not printed to
stderr, instead it is buried in the json output whencargo wdk buildfails:Output seen when tests fail due to compilation errors:
After
Running
kmdf_driver_cross_compiles_with_cli_option_successfullytest after removingaarch64-pc-windows-msvctarget:Compiler diagnostics available on terminal:
Output seen when tests fail due to compilation errors:
