-
Notifications
You must be signed in to change notification settings - Fork 102
refactor: improve logging for build action #495
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
Conversation
515c0e4 to
92d640b
Compare
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.
Pull Request Overview
This pull request improves logging consistency and conciseness throughout the build workflow by updating log messages across the cargo-wdk crate. The changes focus on making log output more streamlined and user-friendly while maintaining clarity about the build process status.
- Updated log messages to use more concise and consistent phrasing
- Simplified warning and info messages for better readability
- Updated test assertions to match the new log message format
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| build_command_test.rs | Updates test assertions to match new log message format from the build workflow |
| mod.rs | Streamlines log messages in build orchestration, making them more concise and consistent |
| build_task.rs | Simplifies cargo build log message to be more general and less verbose |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Added some comments. To be addressed later as we're currently not focusing on this PR.
gurry
left a comment
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 should also shorten this log line:
Processing Rust(possibly driver) project:
to this:
Processing project:
because the Rust(possible driver) part is a bit awkwardly worded and secondly not providing any useful information.
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Gurinder Singh <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Gurinder Singh <[email protected]>
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Gurinder Singh <[email protected]>
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
wmmc88
left a comment
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.
good improvements. agree with some of the other comments that it could be better but i think it a good incremental improvement. Also it seems like the screenshots in the pr are not up to date with the latest code here (which is also why i suggested maybe snapshotting these types of tests is a better way forward medium term)
Yeah, I noticed the issues in the screenshots, especially the last one, and then fixed them in code, but was loath to update the screenshots again :D |
krishnakumar4a4
left a comment
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.
These log improvements look good to me. Thanks @gurry for taking this forward.
This pull request updates and streamlines log messages throughout the build workflow in the
cargo-wdkcrate. The changes focus on making the output more concise and consistent, removing redundant or overly verbose details, and improving clarity in log statements.Logging improvements:
Running cargo buildlog line that appeared before we invoke cargoRunning InfVerifif we are skipping it. We print onlyInvVerif skipped...in that case now.InvVerif skipped...log line from info to warn because it refers to an event that is unusualprocessingand replaced them with the more natural termbuildingThere is still more work to do in this area. The logging for instance can be confusing when building an emulated workspace -- it may be hard know which project is which. We will make such improvements later when we refactor cargo-wdk code which is also overdue.
Here are comparisons showing the effect of this PR on various build scenarios: standalone driver, workspace and emulated workspace. The left part is the output before this PR and the right side is after the PR.
Standalone Driver Project
Workspace
Emulated Workspace