Improve logging #29
Merged
Improve logging #29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Simplify the cli main function by pulling out the logging setup. The cli function is more difficualt to test, the more we can full out the easier it is to test. Signed-off-by: James McCorrie <[email protected]>
Collaborator
Author
|
It looks like later versions of ruff have introduced some extra linting rules that now fail. I've added a commit that resolves the failed lints, although not directly related to this PR. |
rswarbrick
reviewed
Sep 26, 2025
Contributor
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
These look like nice cleanups! It's lovely to see things getting more solid.
Simplify logging with a custom logger. There is no need to import VERBOSE as this is replaced with a `log.verbose(...)` helper. Now that we have resolved all instances of LOG015, lets enable the rule in CI. Signed-off-by: James McCorrie <[email protected]>
It doesn't look like these scripts are used in this version of DVSim. Instead the versions shipped with OpenTitan are used if they are used at all. For the moment remove the shebangs and remove the executable flag from the files. They would not be useable from a package install anyway. When it comes to reintegrating with OpenTitan, then a new solution will be required. For the moment this patch resolves new failing linting checks. Signed-off-by: James McCorrie <[email protected]>
hcallahan-lowrisc
approved these changes
Sep 27, 2025
Contributor
hcallahan-lowrisc
left a comment
hcallahan-lowrisc
left a comment
There was a problem hiding this comment.
I think a custom logger is fine, but I would love to see us move to module-level loggers in the future. It has always bothered me that we never moved towards something properly idiomatic here.
Anyway, that's a future task. This is a great improvement for now, and long overdue! Thanks @machshev
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of this PR is twofold:
Python programs and especially libraries should not use the default logger but create a logger instance. If you use the default logger, then setting the log level for example would set the log level for any 3rd party libraries used (polluting the logs and making them difficult to read). Typically a logger instance would would be crated at module level with something like
logger = logging.getLogger(__name__). For the moment though, lets start with something simpler and make it more complicated later if desired.The
LOG015lint rule checks for default logger use instances. In the DVSim codebase that accounts for129instances (out of1128lint fails). With this PR the total lint fails is reduced to a mere968;)