feat: report EDA tool version from a dedicated version query - #242
feat: report EDA tool version from a dedicated version query#242hcallahan-lowrisc wants to merge 1 commit into
Conversation
Replace the hardcoded "unknown" tool version in reports with a value probed from the tool at runtime. Each SimTool plugin declares a VersionQuery (command + regex) as data, and a single generic runner executes it, so adding a tool needs no new logic. Any failure falls back to "unknown". Signed-off-by: Harry Callahan <hcallahan@lowrisc.org>
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
This looks really sensible to me. Lots of nitty comments (sorry), but it looks good.
| cmd: the command that makes the tool print its version. | ||
| pattern: a regex applied (in multiline mode) to the combined | ||
| stdout/stderr of ``cmd``. The first capture group is used as the | ||
| version string. |
There was a problem hiding this comment.
I'm not a Python expert, but I'm slightly surprised by this. Is there a reason not to use this text to decorate the fields, rather than as a string at the top?
| class VCS: | ||
| """Implement VCS tool support.""" | ||
|
|
||
| # `vcs -id` reports a line like: "Compiler version = VCS X-2025.06-SP2-1_Full64". |
There was a problem hiding this comment.
I'd probably either explain the 64 bit thing, or give "vcs -full64 -id" in the string here.
| # `vcs -id` reports a line like: "Compiler version = VCS X-2025.06-SP2-1_Full64". | ||
| version_query: ClassVar[VersionQuery | None] = VersionQuery( | ||
| cmd="vcs -full64 -id", | ||
| pattern=r"^Compiler version\s*=\s*(?:VCS\s+)?(\S+)", |
There was a problem hiding this comment.
I take it that some versions don't include "VCS"? Maybe this needs a comment explaining why the extra group is here.
Alternatively, would something like this work?
^Compiler version\s*=.*\s([^ ]+)$
| # `xrun -version` reports a line like: "TOOL: xrun(64) 24.03-s007". | ||
| version_query: ClassVar[VersionQuery | None] = VersionQuery( | ||
| cmd="xrun -version", | ||
| pattern=r"^TOOL:\s*xrun\(\d+\)\s+(\S+)", |
There was a problem hiding this comment.
As with the VCS example, I'd suggest being floppier. How about something like this?
^TOOL:.*xrun.*\s([^ ]+)$
| revision_info=self.revision, | ||
| ) | ||
| tool = ToolMeta(name=self.tool.lower(), version="unknown") | ||
| tool = ToolMeta(name=self.tool.lower(), version=query_tool_version(self.tool) or "unknown") |
There was a problem hiding this comment.
This should probably be split over 2 lines to avoid falsy strings with "is None".
| "z01x": Z01X, | ||
| } | ||
|
|
||
| # EDA tools should respond to a `--version`-style query near-instantly, but guard |
There was a problem hiding this comment.
So that it doesn't sound like "EDA tools should do X but might guard Y", I'd suggest splitting the string. "... query near-instantly. This timeout is just to ensure that a hung/misconfigured tool can't block things forever."
| """ | ||
| try: | ||
| result = subprocess.run( # noqa: S603 | ||
| shlex.split(cmd), |
There was a problem hiding this comment.
This looks odd to me. Since subprocess.run will do the splitting, there's no reason to do it ourselves - things are already globbed together, so we don't know any more than the library code.
| log.debug("Failed to query tool version via '%s': %s", cmd, e) | ||
| return None | ||
|
|
||
| return result.stdout + result.stderr |
There was a problem hiding this comment.
Nit: If the order doesn't matter, I'd suggest doing them the other way round (because the result is more like what normally happens on the terminal)
| log.debug("Could not parse %s version from output of '%s'", tool, query.cmd) | ||
| return None | ||
|
|
||
| return match.group(1).strip() |
There was a problem hiding this comment.
I'm not sure, but does it make sense to catch IndexErrors here and spit out an explicit error to make it easier to write the EDA backend? Or is this just "the author should read the docs..."? :-)
2 participants
Description
Replace the hardcoded "unknown" tool version in reports with a value probed from the tool at runtime. Each SimTool plugin declares a VersionQuery (command + regex) as data, and a single generic runner executes it, so adding a tool needs no new logic. Any failure falls back to "unknown".
A tool version is now printed inside the final report badge. E.g.
Checklist
git commit -s), indicating acceptance of the CLA<type>[(<scope>)][!]: <description>)!or aBREAKING CHANGE:footer