-
Notifications
You must be signed in to change notification settings - Fork 20
feat: report EDA tool version from a dedicated version query #242
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,22 +5,42 @@ | |
| """EDA simulation tool interface.""" | ||
|
|
||
| from collections.abc import Mapping, Sequence | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Protocol, runtime_checkable | ||
| from typing import TYPE_CHECKING, ClassVar, Protocol, runtime_checkable | ||
|
|
||
| from dvsim.job.data import JobSpec | ||
| from dvsim.sim.data import CoverageMetrics | ||
|
|
||
| if TYPE_CHECKING: | ||
| from dvsim.job.deploy import Deploy | ||
|
|
||
| __all__ = ("SimTool",) | ||
| __all__ = ("SimTool", "VersionQuery") | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class VersionQuery: | ||
| """Declarative description of how to query an EDA tool for its version. | ||
|
|
||
| Attributes: | ||
| 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. | ||
|
Comment on lines
+26
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
|
||
| """ | ||
|
|
||
| cmd: str | ||
| pattern: str | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class SimTool(Protocol): | ||
| """Simulation tool interface required by the Sim workflow.""" | ||
|
|
||
| version_query: ClassVar[VersionQuery | None] | ||
| """How to query this tool's version, or ``None`` if unsupported.""" | ||
|
|
||
| @staticmethod | ||
| def get_cov_summary_table(cov_report_path: Path) -> tuple[Sequence[Sequence[str]], str]: | ||
| """Get a coverage summary. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,10 +7,11 @@ | |
| import re | ||
| from collections.abc import Mapping, Sequence | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING | ||
| from typing import TYPE_CHECKING, ClassVar | ||
|
|
||
| from dvsim.job.data import JobSpec | ||
| from dvsim.sim.data import CodeCoverageMetrics, CoverageMetrics | ||
| from dvsim.sim.tool.base import VersionQuery | ||
|
|
||
| if TYPE_CHECKING: | ||
| from dvsim.job.deploy import Deploy | ||
|
|
@@ -21,6 +22,12 @@ | |
| class VCS: | ||
| """Implement VCS tool support.""" | ||
|
|
||
| # `vcs -id` reports a line like: "Compiler version = VCS X-2025.06-SP2-1_Full64". | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd probably either explain the 64 bit thing, or give "vcs -full64 -id" in the string here. |
||
| version_query: ClassVar[VersionQuery | None] = VersionQuery( | ||
| cmd="vcs -full64 -id", | ||
| pattern=r"^Compiler version\s*=\s*(?:VCS\s+)?(\S+)", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| ) | ||
|
|
||
| @staticmethod | ||
| def get_cov_summary_table(cov_report_path: Path) -> tuple[Sequence[Sequence[str]], str]: | ||
| """Get a coverage summary. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,11 @@ | |
| from collections import defaultdict | ||
| from collections.abc import Mapping, Sequence | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING | ||
| from typing import TYPE_CHECKING, ClassVar | ||
|
|
||
| from dvsim.job.data import JobSpec | ||
| from dvsim.sim.data import CodeCoverageMetrics, CoverageMetrics | ||
| from dvsim.sim.tool.base import VersionQuery | ||
|
|
||
| if TYPE_CHECKING: | ||
| from dvsim.job.deploy import Deploy | ||
|
|
@@ -22,6 +23,12 @@ | |
| class Xcelium: | ||
| """Implement Xcelium tool support.""" | ||
|
|
||
| # `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+)", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As with the VCS example, I'd suggest being floppier. How about something like this? |
||
| ) | ||
|
|
||
| @staticmethod | ||
| def get_cov_summary_table(cov_report_path: Path) -> tuple[Sequence[Sequence[str]], str]: | ||
| """Get a coverage summary. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,20 +4,30 @@ | |
|
|
||
| """EDA Tool base.""" | ||
|
|
||
| import re | ||
| import shlex | ||
| import subprocess | ||
| from collections.abc import Callable | ||
| from functools import cache | ||
|
|
||
| from dvsim.logging import log | ||
| from dvsim.sim.tool.base import SimTool | ||
| from dvsim.sim.tool.vcs import VCS | ||
| from dvsim.sim.tool.xcelium import Xcelium | ||
| from dvsim.sim.tool.z01x import Z01X | ||
|
|
||
| __all__ = ("get_sim_tool_plugin",) | ||
| __all__ = ("get_sim_tool_plugin", "query_tool_version") | ||
|
|
||
| _SUPPORTED_SIM_TOOLS = { | ||
| "vcs": VCS, | ||
| "xcelium": Xcelium, | ||
| "z01x": Z01X, | ||
| } | ||
|
|
||
| # EDA tools should respond to a `--version`-style query near-instantly, but guard | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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." |
||
| # against a hung/misconfigured tool so report generation cannot block forever. | ||
| _VERSION_QUERY_TIMEOUT_S = 30 | ||
|
|
||
|
|
||
| def get_sim_tool_plugin(tool: str) -> SimTool: | ||
| """Get a simulation tool plugin.""" | ||
|
|
@@ -31,3 +41,73 @@ def get_sim_tool_plugin(tool: str) -> SimTool: | |
| raise NotImplementedError(msg) | ||
|
|
||
| return _SUPPORTED_SIM_TOOLS[tool] | ||
|
|
||
|
|
||
| @cache | ||
| def _run_version_command(cmd: str) -> str | None: | ||
| """Run a tool version-query command, returning its combined output or None. | ||
|
|
||
| The tool's version may be printed to either stdout or stderr, so both are | ||
| captured and concatenated. Any failure to launch or run the command (the | ||
| tool is not on PATH, a non-EDA host, a timeout, ...) is treated as "version | ||
| unknown" rather than an error: querying the version must never break a run. | ||
|
|
||
| Results are cached for the lifetime of the process since a tool's version is | ||
| invariant across a single dvsim invocation; this dedupes the query when | ||
| multiple configs share a tool. | ||
| """ | ||
| try: | ||
| result = subprocess.run( # noqa: S603 | ||
| shlex.split(cmd), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks odd to me. Since |
||
| capture_output=True, | ||
| text=True, | ||
| timeout=_VERSION_QUERY_TIMEOUT_S, | ||
| check=False, | ||
| ) | ||
| except (OSError, subprocess.SubprocessError) as e: | ||
| log.debug("Failed to query tool version via '%s': %s", cmd, e) | ||
| return None | ||
|
|
||
| return result.stdout + result.stderr | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
|
||
|
|
||
| def query_tool_version( | ||
| tool: str, | ||
| *, | ||
| run: Callable[[str], str | None] = _run_version_command, | ||
| ) -> str | None: | ||
| """Query the version of an EDA tool from the runtime environment. | ||
|
|
||
| Runs the tool plugin's declared version-query command and parses the version | ||
| out of its output. This is a local, best-effort probe: it assumes the tool | ||
| is available on PATH of the host running dvsim. | ||
|
|
||
| TODO: for farm launchers (LSF/SGE/SLURM) the tool may only exist on the | ||
| compute nodes. To be correct there, this should run as a lightweight | ||
| preflight job dispatched through the runtime backend so it inherits the same | ||
| environment (e.g. `module load`) as real jobs. | ||
|
|
||
| Args: | ||
| tool: the name of the tool to query (as passed to `--tool`). | ||
| run: callable that executes the query command and returns its combined | ||
| output, or None on failure. Injectable for testing. | ||
|
|
||
| Returns: | ||
| The parsed version string, or None if the tool declares no query, the | ||
| command failed, or the output did not match the expected pattern. | ||
|
|
||
| """ | ||
| query = get_sim_tool_plugin(tool).version_query | ||
| if query is None: | ||
| return None | ||
|
|
||
| output = run(query.cmd) | ||
| if output is None: | ||
| return None | ||
|
|
||
| match = re.search(query.pattern, output, re.MULTILINE) | ||
| if match is None: | ||
| log.debug("Could not parse %s version from output of '%s'", tool, query.cmd) | ||
| return None | ||
|
|
||
| return match.group(1).strip() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, but does it make sense to catch |
||
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.
This should probably be split over 2 lines to avoid falsy strings with "is None".