From 27df69206ddb31b81b872858bd9e977b67a57d6e Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Sun, 25 Oct 2026 01:10:39 +0000 Subject: [PATCH 1/2] feat: add --cfg-extend to merge a file into a named cfg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no way to add to a cfg that dvsim loads from the project tree without editing that tree: import_cfgs and overrides only act on the file that holds them, and a primary cfg can't reach into its children. A partner or licensed-VIP testbench kept outside the tree therefore has to patch the project's hjson. --cfg-extend NAME=PATH (repeatable) loads PATH with its imports and merges it into the cfg whose name is NAME, after that cfg has merged all of its own files and before overrides and wildcards are processed. The normal rules apply: lists concatenate, so the extension's build_opts come after the tool's, which hold its filelist; a conflicting scalar is an error (tool included, unless --tool is given); the extension's overrides join the cfg's own. Because the arguments are shared with every child cfg, this works whether NAME is run on its own or is a child of a primary cfg. The extension names its own files with {file_dir}. Extensions apply in command-line order. An extension that no cfg matched is an error, which catches typos and upstream renames; so is naming the same NAME=PATH twice, naming a primary cfg, an extension that sets use_cfgs, or one that names an import that can't be resolved. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- doc/design_doc.md | 18 +++ src/dvsim/cli/run.py | 24 ++++ src/dvsim/flow/base.py | 42 ++++++ src/dvsim/flow/extension.py | 113 +++++++++++++++ src/dvsim/flow/factory.py | 25 +++- src/dvsim/flow/hjson.py | 9 +- tests/flow/test_extension.py | 267 +++++++++++++++++++++++++++++++++++ 7 files changed, 495 insertions(+), 3 deletions(-) create mode 100644 src/dvsim/flow/extension.py create mode 100644 tests/flow/test_extension.py diff --git a/doc/design_doc.md b/doc/design_doc.md index 7865918c..79869c28 100644 --- a/doc/design_doc.md +++ b/doc/design_doc.md @@ -149,6 +149,24 @@ Within any of these files, `{file_dir}` names the directory that holds that file `{self_dir}`, by contrast, is the directory of the configuration file that dvsim loaded, at the top of the import tree. `{file_dir}` is replaced as each file is loaded, so it can only be used in Hjson files, not in options given on the command line. +A configuration can also be extended from outside the project tree with `--cfg-extend NAME=PATH`, for example to add tests that need a licensed verification IP that the project itself can't hold: + +```hjson +// ext.hjson, merged with: dvsim top_sim_cfgs.hjson --cfg-extend foo=/path/to/ext.hjson ... +{ + import_cfgs: ["{file_dir}/vip_tests.hjson"] + build_modes: [{name: "vip", build_opts: ["-f {file_dir}/vip.f"]}] + overrides: [{name: "foo_setting", value: "with_vip"}] +} +``` + +The Hjson file at `PATH`, with its imports, is merged into the configuration whose `name` is `NAME` once that configuration has loaded all of its own files; this works whether that configuration is run on its own or is a child of a primary configuration. +`NAME` is matched against the name that the configuration's own files give it, before any `overrides`. +The usual merge rules apply: the extension's list entries follow the configuration's own, and a conflicting scalar is an error, so an extension changes a scalar with `overrides` (but not one that the configuration already overrides). +In an extension, `{self_dir}` is still the directory of the configuration file that dvsim loaded, so the extension names its own files with `{file_dir}`. +Several extensions apply in command-line order. +An extension whose `NAME` matches no loaded configuration is an error, as is naming one extension twice. + ### Parser stage The **parser stage** parses all Hjson files in the tree and constructs a **flow manager object**. diff --git a/src/dvsim/cli/run.py b/src/dvsim/cli/run.py index 60d36877..63eae362 100644 --- a/src/dvsim/cli/run.py +++ b/src/dvsim/cli/run.py @@ -37,6 +37,7 @@ load_config_file, proj_root_from_config, ) +from dvsim.flow.extension import CfgExtension from dvsim.flow.factory import make_cfg from dvsim.fusesoc import CONFIG_SECTION as FUSESOC_CONFIG_SECTION from dvsim.fusesoc import resolve_options as resolve_fusesoc_options @@ -185,6 +186,14 @@ def parse_resource(s: str) -> tuple[str, int | None]: raise argparse.ArgumentTypeError(msg) from e +def parse_cfg_extension(s: str) -> CfgExtension: + """Parse a cfg extension (NAME=PATH) from the argparse CLI.""" + try: + return CfgExtension.parse(s) + except ValueError as e: + raise argparse.ArgumentTypeError(str(e)) from e + + def resolve_branch(branch, cwd=None): """Choose a branch name for output files. @@ -453,6 +462,21 @@ def parse_args(argv: list[str] | None = None): ), ) + whatg.add_argument( + "--cfg-extend", + action="append", + default=[], + type=parse_cfg_extension, + metavar="NAME=PATH", + help=( + "Merge the hjson file PATH (and its imports) into the cfg whose " + "'name' is NAME, after the cfg's own files. NAME may be a child of " + "a primary config. Lists are appended to; scalars are changed " + "with 'overrides'. PATH names its own files with {file_dir}. " + "Repeatable; applied in order. It is an error if NAME matches no cfg." + ), + ) + disg = parser.add_argument_group("Dispatch options") disg.add_argument( diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index c6badf9a..152ddc35 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -17,6 +17,7 @@ import hjson import dvsim.instrumentation.runtime as instrumentation +from dvsim.flow.extension import CfgExtension, load_cfg_extension from dvsim.flow.hjson import set_target_attribute from dvsim.fusesoc import rewrite_fusesoc_opts from dvsim.job.data import CompletedJobStatus, JobSpec, WorkspaceConfig @@ -147,7 +148,15 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # configuration". self.is_primary_cfg = "use_cfgs" in hjson_data + # The cfg extensions (--cfg-extend) merged into this cfg, and the name + # that they were matched against. + self.cfg_extensions: list[CfgExtension] = [] + self.cfg_extend_name: str | None = None + if not self.is_primary_cfg: + # Extensions merge after everything the cfg loads itself, and + # before overrides and wildcards are processed. + self._merge_cfg_extensions() self.cfgs.append(self) else: for entry in self.use_cfgs: @@ -205,6 +214,39 @@ def _merge_hjson(self, hjson_data: Mapping) -> None: for key, value in hjson_data.items(): set_target_attribute(self.flow_cfg_file, self.__dict__, key, value) + def _merge_cfg_extensions(self) -> None: + """Merge in the cfg extensions (--cfg-extend) that name this cfg. + + They apply in command-line order. Their list values follow the cfg's + own, and their overrides join the cfg's own. + """ + # The name as the cfg's own files give it. Overrides could still change + # it, but those are processed after the extensions merge. + name = subst_wildcards(self.name, self.__dict__, ignore_error=True) + self.cfg_extend_name = name + + # What load_hjson needs to resolve the names of imported files. As in + # the cfg's own files, {self_dir} is the cfg's directory: an extension + # names its own files with {file_dir}. + initial_values = {"proj_root": self.proj_root, "self_dir": self.self_dir} + if self.tool: + initial_values["tool"] = self.tool + # Only a tool given on the command line can't be changed by the files. + arg_keys = {"tool"} if self.args.tool is not None else set() + + for ext in self.args.cfg_extend: + if ext.cfg_name != name: + continue + log.info("[cfg_extend]: %s: %s", name, ext.path) + try: + ext_data = load_cfg_extension(ext, initial_values, arg_keys) + for key, value in ext_data.items(): + set_target_attribute(str(ext.path), self.__dict__, key, value) + except RuntimeError as err: + log.error("Cannot extend cfg %r: %s", name, err) + sys.exit(1) + self.cfg_extensions.append(ext) + def _expand(self) -> None: """Expand wildcards after merging hjson. diff --git a/src/dvsim/flow/extension.py b/src/dvsim/flow/extension.py new file mode 100644 index 00000000..dba1ecc6 --- /dev/null +++ b/src/dvsim/flow/extension.py @@ -0,0 +1,113 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Cfg extensions: hjson files merged into a named flow cfg (--cfg-extend). + +See the "DUT configuration" section of doc/design_doc.md. +""" + +import os +from collections.abc import Iterable, Mapping +from dataclasses import dataclass +from pathlib import Path + +from dvsim.flow.hjson import load_hjson + +__all__ = ( + "CfgExtension", + "check_cfg_extensions_used", + "load_cfg_extension", +) + + +@dataclass(frozen=True) +class CfgExtension: + """An extension file, and the name of the cfg it extends.""" + + cfg_name: str + path: Path + + @classmethod + def parse(cls, value: str) -> "CfgExtension": + """Parse NAME=PATH, as given to --cfg-extend. + + A relative PATH is taken from the current directory. Symlinks are + kept, as they are in {file_dir}. + """ + name, sep, path = value.partition("=") + name, path = name.strip(), path.strip() + if not sep or not name or not path: + msg = f"Invalid cfg extension {value!r}: expected NAME=PATH." + raise ValueError(msg) + return cls(name, Path(os.path.normpath(Path(path).absolute()))) + + +def load_cfg_extension( + ext: CfgExtension, initial_values: Mapping[str, object], arg_keys: Iterable[str] +) -> dict: + """Load an extension file and its imports. + + initial_values holds what is needed to resolve the names of imported files, + and arg_keys those of its keys that came from the command line (as for + load_hjson). The returned data holds only what the extension sets. + + Raises a RuntimeError if the file can't be loaded or isn't allowed. + """ + path = str(ext.path) + if not ext.path.is_file(): + msg = f"{path!r}: Not a file." + raise RuntimeError(msg) + + try: + data = load_hjson(path, initial_values, set(arg_keys)) + except ValueError as err: + # An import_cfgs entry with a wildcard that can't be resolved. + msg = f"{path!r}: Can't resolve the name of an imported file: {err}" + raise RuntimeError(msg) from err + + if "use_cfgs" in data: + msg = f"{path!r}: An extension can't set use_cfgs." + raise RuntimeError(msg) + + # The initial values are the cfg's own. If the extension set one of them, + # it set it to the same value (or load_hjson reported a conflict), so + # merging it would change nothing. + for key in initial_values: + data.pop(key, None) + + return data + + +def check_cfg_extensions_used( + extensions: Iterable[CfgExtension], + used: Iterable[CfgExtension], + cfg_names: Iterable[str], + primary_name: str | None, +) -> list[str]: + """Check that every extension was used by a loaded cfg. + + used are the extensions that some cfg matched. cfg_names are the names that + the cfgs that can be extended had when they matched them (before any + overrides); a primary cfg can't be extended, so its name is passed + separately to give a clearer message. Returns a list of error messages + (empty if all is well). + """ + used = set(used) + names = set(cfg_names) + errors = [] + for ext in extensions: + if ext in used: + continue + if ext.cfg_name == primary_name: + errors.append( + f"Cfg extension {ext.path} names the primary cfg {ext.cfg_name!r}: " + "only a cfg that runs jobs (a child, or a cfg run on its own) can be extended." + ) + else: + known = ", ".join(sorted(names)) or "none" + errors.append( + f"Cfg extension {ext.path} names {ext.cfg_name!r}, but no loaded cfg has " + f"that name (loaded: {known})." + ) + return errors diff --git a/src/dvsim/flow/factory.py b/src/dvsim/flow/factory.py index 166db158..be76401c 100644 --- a/src/dvsim/flow/factory.py +++ b/src/dvsim/flow/factory.py @@ -7,6 +7,7 @@ from dvsim.flow.base import FlowCfg from dvsim.flow.cdc import CdcCfg +from dvsim.flow.extension import check_cfg_extensions_used from dvsim.flow.formal import FormalCfg from dvsim.flow.hjson import load_hjson from dvsim.flow.lint import LintCfg @@ -113,6 +114,13 @@ def make_cfg(path, args, proj_root) -> FlowCfg: of the project. """ + # An extension named twice would be merged twice. + repeated = [ext for ext in dict.fromkeys(args.cfg_extend) if args.cfg_extend.count(ext) > 1] + for ext in repeated: + log.error("Cfg extension %s=%s is given more than once.", ext.cfg_name, ext.path) + if repeated: + sys.exit(1) + initial_values = { "proj_root": proj_root, "self_dir": pathlib.Path(path).parent, @@ -131,4 +139,19 @@ def factory(child_path): child_ivs["flow"] = hjson_data["flow"] return _make_child_cfg(child_path, args, child_ivs) - return cls(path, hjson_data, args, factory) + cfg = cls(path, hjson_data, args, factory) + + # Every extension must have found its cfg. A name that matches nothing is + # a typo or a cfg that has been renamed, and would otherwise pass silently. + errors = check_cfg_extensions_used( + args.cfg_extend, + [ext for child in cfg.cfgs for ext in child.cfg_extensions], + [child.cfg_extend_name for child in cfg.cfgs], + cfg.name if cfg.is_primary_cfg else None, + ) + for error in errors: + log.error("%s", error) + if errors: + sys.exit(1) + + return cfg diff --git a/src/dvsim/flow/hjson.py b/src/dvsim/flow/hjson.py index 37187667..9d946c0d 100644 --- a/src/dvsim/flow/hjson.py +++ b/src/dvsim/flow/hjson.py @@ -17,7 +17,7 @@ _FILE_DIR_WILDCARD = "{file_dir}" -def load_hjson(path, initial_values): +def load_hjson(path, initial_values, arg_keys=None): """Load an hjson file and any includes. Combines them all into a single dictionary, which is then returned. This @@ -29,6 +29,10 @@ def load_hjson(path, initial_values): is not modified). It needs to contain values for anything needed to resolve include files (typically, this is 'proj_root' and 'tool' (if set)). + arg_keys are the keys of initial_values that came from the command line, + which the files can't change. By default, that is any of them that the + command line can set. + """ worklist = [path] seen = {path} @@ -38,7 +42,8 @@ def load_hjson(path, initial_values): # Figure out a list of fields that had a value from the command line. These # should have been passed in as part of initial_values and we need to know # that we can safely ignore updates. - arg_keys = _CMDLINE_FIELDS & initial_values.keys() + if arg_keys is None: + arg_keys = _CMDLINE_FIELDS & initial_values.keys() while worklist: next_path = worklist.pop() diff --git a/tests/flow/test_extension.py b/tests/flow/test_extension.py new file mode 100644 index 00000000..abcacd44 --- /dev/null +++ b/tests/flow/test_extension.py @@ -0,0 +1,267 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for cfg extensions (--cfg-extend).""" + +import subprocess +from collections.abc import Sequence +from pathlib import Path + +import pytest + +from dvsim.cli.run import parse_args +from dvsim.flow.base import FlowCfg +from dvsim.flow.extension import CfgExtension, check_cfg_extensions_used +from dvsim.flow.factory import make_cfg + +__all__ = () + +# A sim cfg with just enough in it to load. Like a real block cfg, it gets its +# tool options from an imported file, so the test can check that an +# extension's list entries come after those. +CHILD_CFG = """{ + name: blk + flow: sim + tool: vcs + scratch_path: "{scratch_root}/blk" + import_cfgs: ["{proj_root}/hw/tool.hjson"] + build_modes: [{name: "waves_off"}, {name: "xprop"}, {name: "build_seed"}] + tests: [{name: "t1"}] + regressions: [] + testplan: "" + blk_var: "orig" + blk_empty: "" +}""" + +TOOL_CFG = '{build_opts: ["-f {proj_root}/sv.f"], run_opts: ["+tool"]}' + +PRIMARY_CFG = """{ + name: top + flow: sim + scratch_path: "{scratch_root}/top" + use_cfgs: ["{proj_root}/hw/blk_sim_cfg.hjson"] +}""" + + +@pytest.fixture +def proj(tmp_path: Path) -> Path: + """A git repo holding a primary cfg and its one child, "blk".""" + root = tmp_path.resolve() / "proj" + (root / "hw").mkdir(parents=True) + (root / "hw" / "blk_sim_cfg.hjson").write_text(CHILD_CFG) + (root / "hw" / "tool.hjson").write_text(TOOL_CFG) + (root / "hw" / "top_sim_cfgs.hjson").write_text(PRIMARY_CFG) + git = ["git", "-C", str(root)] + subprocess.run([*git, "init", "-q"], check=True) + subprocess.run([*git, "commit", "-q", "--allow-empty", "-m", "init"], check=True) + return root + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + return path + + +def _load(proj: Path, cfg: str, *extends: str, argv: Sequence[str] = ()) -> FlowCfg: + argv = [str(proj / "hw" / cfg), "--scratch-root", str(proj.parent / "scratch"), *argv] + argv += ["--branch", "b"] + for ext in extends: + argv += ["--cfg-extend", ext] + args = parse_args(argv) + args.timestamp = args.timestamp_long = "ts" + return make_cfg(args.cfg, args, str(proj)) + + +def _blk(cfg: FlowCfg) -> FlowCfg: + (blk,) = [c for c in cfg.cfgs if c.name == "blk"] + return blk + + +class TestParse: + """Parsing NAME=PATH.""" + + def test_name_and_path(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.chdir(tmp_path) + assert CfgExtension.parse("blk=ext/x.hjson") == CfgExtension( + "blk", tmp_path.resolve() / "ext" / "x.hjson" + ) + + def test_symlinks_are_kept(self, tmp_path: Path) -> None: + """The path is spelt as {file_dir} spells it, through any symlink.""" + (tmp_path / "real").mkdir() + (tmp_path / "link").symlink_to(tmp_path / "real") + + ext = CfgExtension.parse(f"blk={tmp_path}/link/../link/x.hjson") + + assert ext.path == tmp_path / "link" / "x.hjson" + + @pytest.mark.parametrize("value", ["blk", "=x.hjson", "blk=", " = "]) + def test_rejects_malformed(self, value: str) -> None: + with pytest.raises(ValueError, match="NAME=PATH"): + CfgExtension.parse(value) + + def test_cli_rejects_malformed(self, proj: Path) -> None: + with pytest.raises(SystemExit): + parse_args([str(proj / "hw" / "top_sim_cfgs.hjson"), "--cfg-extend", "blk"]) + + +class TestMerge: + """Merging an extension into its cfg.""" + + @pytest.mark.parametrize("top", ["top_sim_cfgs.hjson", "blk_sim_cfg.hjson"]) + def test_lists_follow_the_cfgs_own(self, proj: Path, tmp_path: Path, top: str) -> None: + """The extension reaches the cfg whether it is a child or run alone. + + Its list entries land after everything the cfg imports, so build + options that need the tool's filelist come after it. + """ + ext = _write( + tmp_path / "overlay" / "ext.hjson", + """{ + build_opts: ["+incdir+/overlay", "/overlay/bind.sv"] + tests: [{name: "t2"}] + }""", + ) + + blk = _blk(_load(proj, top, f"blk={ext}")) + + assert blk.build_opts[:3] == [f"-f {proj}/sv.f", "+incdir+/overlay", "/overlay/bind.sv"] + assert blk.run_opts[:1] == ["+tool"] + assert [t.name for t in blk.tests] == ["t1", "t2"] + assert blk.cfg_extensions == [CfgExtension("blk", ext)] + + def test_imports_and_order(self, proj: Path, tmp_path: Path) -> None: + """Extensions apply in command-line order, each with its own imports.""" + first = _write( + tmp_path / "a" / "ext.hjson", + '{import_cfgs: ["{file_dir}/more.hjson"], run_opts: ["+a"]}', + ) + _write(tmp_path / "a" / "more.hjson", '{run_opts: ["+a_more"]}') + second = _write(tmp_path / "b" / "ext.hjson", '{run_opts: ["+b"]}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={first}", f"blk={second}")) + + assert blk.run_opts[:4] == ["+tool", "+a", "+a_more", "+b"] + assert blk.cfg_extensions == [CfgExtension("blk", first), CfgExtension("blk", second)] + + def test_named_twice_is_an_error( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture + ) -> None: + """The same NAME=PATH twice would merge the extension twice.""" + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}", f"blk={ext}") + + assert f"blk={ext} is given more than once" in dvsim_log.text + + def test_overrides_change_a_scalar(self, proj: Path, tmp_path: Path) -> None: + ext = _write( + tmp_path / "ext.hjson", + '{overrides: [{name: "blk_var", value: "new"}]}', + ) + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}")) + + assert blk.blk_var == "new" + + def test_sets_a_scalar_the_cfg_leaves_empty(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{blk_empty: "set"}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}")) + + assert blk.blk_empty == "set" + + @pytest.mark.parametrize( + ("key", "value"), [("tool", "xcelium"), ("scratch_path", "/elsewhere")] + ) + def test_conflicting_scalar_is_an_error( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture, key: str, value: str + ) -> None: + ext = _write(tmp_path / "ext.hjson", f'{{{key}: "{value}"}}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}") + + assert f"Cannot extend cfg 'blk': '{ext}': Value for key '{key}'" in dvsim_log.text + + def test_tool_from_the_command_line_wins(self, proj: Path, tmp_path: Path) -> None: + """As in the cfg's own files, the extension's tool gives way to --tool.""" + ext = _write(tmp_path / "ext.hjson", '{tool: "xcelium"}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}", argv=["--tool", "vcs"])) + + assert blk.tool == "vcs" + + def test_unresolved_import_is_an_error( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture + ) -> None: + ext = _write(tmp_path / "ext.hjson", '{import_cfgs: ["{nope}/x.hjson"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}") + + assert "Can't resolve the name of an imported file" in dvsim_log.text + + def test_use_cfgs_is_an_error(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{use_cfgs: ["/x.hjson"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}") + + def test_missing_file_is_an_error(self, proj: Path, tmp_path: Path) -> None: + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={tmp_path / 'nope.hjson'}") + + +class TestNames: + """Matching an extension to a cfg by name.""" + + def test_unknown_name_is_an_error(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blkk={ext}") + + def test_name_as_the_cfgs_own_files_give_it( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture + ) -> None: + """An extension matches the name before overrides; one that matched nothing is an error. + + Here the cfg's own overrides rename it from 'blk' to 'blk2' after the extensions merge. + """ + cfg = proj / "hw" / "blk_sim_cfg.hjson" + cfg.write_text( + CHILD_CFG.replace("name: blk", 'name: "{n}"\n n: blk').replace( + "testplan:", 'overrides: [{name: "n", value: "blk2"}]\n testplan:' + ) + ) + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk2={ext}") + assert "(loaded: blk)" in dvsim_log.text + + (blk2,) = _load(proj, "top_sim_cfgs.hjson", f"blk={ext}").cfgs + assert (blk2.name, blk2.run_opts.count("+x")) == ("blk2", 1) + + def test_primary_name_is_an_error(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"top={ext}") + + def test_messages(self) -> None: + exts = [CfgExtension("a", Path("/a")), CfgExtension("top", Path("/t"))] + + assert check_cfg_extensions_used(exts, exts[:1], ["a", "b"], "top") == [ + ( + "Cfg extension /t names the primary cfg 'top': only a cfg that runs jobs " + "(a child, or a cfg run on its own) can be extended." + ) + ] + assert check_cfg_extensions_used(exts[:1], [], ["b"], None) == [ + "Cfg extension /a names 'a', but no loaded cfg has that name (loaded: b)." + ] From 9649c09a2464efd7e8b0671467eb2dea785723e6 Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Mon, 26 Oct 2026 10:24:53 +0000 Subject: [PATCH 2/2] feat: record the git revision of each cfg extension in the reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A report says which project commit its results came from, but not which version of a --cfg-extend file was merged in. When the extension lives in its own repository, as it usually does, the results of the tests it adds can't be traced back to it. When a cfg merges an extension, dvsim now reads the git revision of the checkout that holds the extension file: the origin URL, with any credentials stripped as for the project's own, the commit, and whether the checkout is dirty, which includes the file itself being untracked or ignored. The sim flow JSON results list each cfg's extensions in command-line order with that revision (null outside git), the Markdown block report lists them with their revisions, and the HTML block report shows them in its header. This adds a key, extensions, to the JSON results: to each flow's .json and to each entry of flow_results in index.json. A flow that merged no extension leaves the key out, so the JSON of a run that doesn't use --cfg-extend is unchanged. A run that does use it writes JSON that an older dvsim can't load: SimFlowResults and SimResultsSummary forbid unknown keys (extra="forbid"), so for example dvsim-admin report gen of an earlier release fails to validate it. Readers that parse the JSON leniently are not affected. The JSON has no schema version that a reader could check; making the models ignore unknown keys, or adding a version, is left to a later change. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- doc/design_doc.md | 3 + src/dvsim/flow/base.py | 13 +- src/dvsim/report/data.py | 36 ++++++ src/dvsim/sim/data.py | 30 ++++- src/dvsim/sim/flow.py | 3 + src/dvsim/sim/report.py | 15 ++- src/dvsim/templates/reports/block_report.html | 9 ++ src/dvsim/utils/git.py | 30 +++++ tests/flow/test_extension.py | 27 +++- tests/report/test_extensions.py | 117 ++++++++++++++++++ tests/utils/test_git.py | 49 ++++++++ 11 files changed, 322 insertions(+), 10 deletions(-) create mode 100644 tests/report/test_extensions.py diff --git a/doc/design_doc.md b/doc/design_doc.md index 79869c28..6fa789fa 100644 --- a/doc/design_doc.md +++ b/doc/design_doc.md @@ -166,6 +166,9 @@ The usual merge rules apply: the extension's list entries follow the configurati In an extension, `{self_dir}` is still the directory of the configuration file that dvsim loaded, so the extension names its own files with `{file_dir}`. Several extensions apply in command-line order. An extension whose `NAME` matches no loaded configuration is an error, as is naming one extension twice. +The sim reports and JSON results list the extensions that each configuration merged, with the git revision of the checkout that holds each extension file: its `origin` (without credentials), its commit, and whether it was dirty, which includes the extension file itself not being tracked. +Files that an extension imports from elsewhere are not recorded. +The JSON results of a configuration that merged no extension have no `extensions` key, as before extensions were recorded. ### Parser stage diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 152ddc35..51aad8ef 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -23,6 +23,7 @@ from dvsim.job.data import CompletedJobStatus, JobSpec, WorkspaceConfig from dvsim.job.status import JobStatus from dvsim.logging import log +from dvsim.report.data import CfgExtensionMeta from dvsim.scheduler.resources import UnknownResourcePolicy from dvsim.scheduler.runner import ( build_default_scheduler_backend, @@ -34,7 +35,7 @@ rm_path, subst_wildcards, ) -from dvsim.utils.git import git_commit_hash, git_is_dirty +from dvsim.utils.git import git_commit_hash, git_is_dirty, git_revision if TYPE_CHECKING: from dvsim.job.deploy import Deploy @@ -148,9 +149,9 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # configuration". self.is_primary_cfg = "use_cfgs" in hjson_data - # The cfg extensions (--cfg-extend) merged into this cfg, and the name - # that they were matched against. - self.cfg_extensions: list[CfgExtension] = [] + # The cfg extensions (--cfg-extend) merged into this cfg, in order, as + # the reports describe them, and the name that they were matched against. + self.cfg_extensions: dict[CfgExtension, CfgExtensionMeta] = {} self.cfg_extend_name: str | None = None if not self.is_primary_cfg: @@ -245,7 +246,9 @@ def _merge_cfg_extensions(self) -> None: except RuntimeError as err: log.error("Cannot extend cfg %r: %s", name, err) sys.exit(1) - self.cfg_extensions.append(ext) + self.cfg_extensions[ext] = CfgExtensionMeta( + path=str(ext.path), revision=git_revision(ext.path) + ) def _expand(self) -> None: """Expand wildcards after merging hjson. diff --git a/src/dvsim/report/data.py b/src/dvsim/report/data.py index e385d204..08aadd0f 100644 --- a/src/dvsim/report/data.py +++ b/src/dvsim/report/data.py @@ -4,9 +4,13 @@ """Report data models.""" +from pathlib import Path + from pydantic import BaseModel, ConfigDict __all__ = ( + "CfgExtensionMeta", + "GitRevisionMeta", "IPMeta", "ToolMeta", ) @@ -50,3 +54,35 @@ class ToolMeta(BaseModel): """Name of the tool.""" version: str """Version of the tool.""" + + +class CfgExtensionMeta(BaseModel): + """A cfg extension (--cfg-extend) merged into a flow's cfg.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + path: str + """Path of the extension file.""" + revision: "GitRevisionMeta | None" = None + """The git revision of the checkout that holds the extension file (not of + any files it imports from elsewhere), or None if it isn't in git.""" + + @property + def name(self) -> str: + """The name of the extension file.""" + return Path(self.path).name + + +class GitRevisionMeta(BaseModel): + """The git revision of a checkout, when the run started.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + origin: str | None + """URL of the ``origin`` remote (credentials stripped), or None.""" + commit: str + """Git commit.""" + commit_short: str + """Shortened git commit.""" + dirty: bool + """Whether the checkout had uncommitted changes, or didn't track the file.""" diff --git a/src/dvsim/sim/data.py b/src/dvsim/sim/data.py index 888dfad1..2f61874b 100644 --- a/src/dvsim/sim/data.py +++ b/src/dvsim/sim/data.py @@ -7,10 +7,11 @@ from collections.abc import Mapping from datetime import datetime from pathlib import Path +from typing import Any -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, SerializerFunctionWrapHandler, model_serializer -from dvsim.report.data import IPMeta, ToolMeta +from dvsim.report.data import CfgExtensionMeta, IPMeta, ToolMeta from dvsim.sim_results import BucketedFailures __all__ = ( @@ -169,6 +170,17 @@ def flattened(self) -> dict[str, float | None]: return items +def _without_empty_extensions(data: dict[str, Any]) -> dict[str, Any]: + """Leave out the extensions of a flow that has none. + + A flow run without --cfg-extend then has the keys it had before extensions were recorded, so a + reader built for those (whose models forbid unknown keys) can still load it. + """ + if not data.get("extensions"): + data.pop("extensions", None) + return data + + class SimFlowResults(BaseModel): """Flow results data.""" @@ -207,6 +219,13 @@ class SimFlowResults(BaseModel): percent: float """Percentage test pass rate.""" + extensions: tuple[CfgExtensionMeta, ...] = () + """The cfg extensions (--cfg-extend) named for this flow.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_empty_extensions(handler(self)) + def summary(self) -> "SimFlowSummary": """Load results from JSON file. @@ -247,6 +266,13 @@ class SimFlowSummary(BaseModel): percent: float """Percentage test pass rate.""" + extensions: tuple[CfgExtensionMeta, ...] = () + """The cfg extensions (--cfg-extend) named for this flow.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_empty_extensions(handler(self)) + @staticmethod def load(path: Path) -> "SimFlowSummary": """Load results from JSON file. diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index a907a832..9a0fc75a 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -877,6 +877,8 @@ def make_test_result(tr) -> TestResult | None: if failures.buckets: self.errors_seen = True + extensions = self.cfg_extensions.values() + # --- Final result --- return SimFlowResults( block=block, @@ -893,6 +895,7 @@ def make_test_result(tr) -> TestResult | None: passed=total_passed, total=total_runs, percent=100.0 * total_passed / total_runs if total_runs else 0.0, + extensions=tuple(extensions), ) def _fake_policy(self, job: JobSpec) -> JobStatus: diff --git a/src/dvsim/sim/report.py b/src/dvsim/sim/report.py index bdda6311..0574a588 100644 --- a/src/dvsim/sim/report.py +++ b/src/dvsim/sim/report.py @@ -14,7 +14,7 @@ from dvsim.logging import log from dvsim.report.artifacts import ReportArtifacts, display_report, render_static_content -from dvsim.report.data import IPMeta +from dvsim.report.data import CfgExtensionMeta, IPMeta from dvsim.sim.data import SimFlowResults, SimResultsSummary from dvsim.templates.render import render_template from dvsim.utils import TS_FORMAT_LONG @@ -159,6 +159,16 @@ def render( return artifacts +def _revision(ext: CfgExtensionMeta) -> str: + """Describe the git revision of a cfg extension, for the Markdown reports.""" + rev = ext.revision + if rev is None: + return "" + dirty = ", dirty" if rev.dirty else "" + origin = f", from {rev.origin}" if rev.origin else "" + return f" (revision `{rev.commit_short}`{dirty}{origin})" + + class MarkdownReportRenderer: """Renders a Markdown report of the sim results.""" @@ -213,6 +223,9 @@ def render_block(self, results: SimFlowResults) -> ReportArtifacts: if len(results.stages) > 0 and testplan_ref: report_md += f"\n### [Testplan]({testplan_ref})" report_md += f"\n### Simulator: {results.tool.name.upper()}" + if results.extensions: + report_md += "\n### Cfg extensions:\n" + report_md += "".join(f"\n* `{ext.path}`{_revision(ext)}" for ext in results.extensions) # Record a summary of the simulation results, coverage, and failure buckets, if applicable. result_summary = self.render_block_results(results) diff --git a/src/dvsim/templates/reports/block_report.html b/src/dvsim/templates/reports/block_report.html index 4f6177fd..fbf31341 100644 --- a/src/dvsim/templates/reports/block_report.html +++ b/src/dvsim/templates/reports/block_report.html @@ -58,6 +58,15 @@

Simulation Results: {{ block.variant_name(sep='/') }}

Tool: {{ tool.name }} [{{ tool.version }}] + {% for ext in results.extensions %} + {% set rev = ext.revision %} + + Extension: {{ ext.name }} + {%- if rev %} sha: {{ rev.commit_short }}{% endif %} + {%- if rev and rev.dirty %} (dirty){% endif %} + + {% endfor %} {% if build_seed %} Build seed: {{ build_seed }} diff --git a/src/dvsim/utils/git.py b/src/dvsim/utils/git.py index 0776d9de..a896f1bb 100644 --- a/src/dvsim/utils/git.py +++ b/src/dvsim/utils/git.py @@ -8,8 +8,10 @@ from urllib.parse import urlsplit, urlunsplit from git import Repo +from git.exc import GitCommandError, GitError from dvsim.logging import log +from dvsim.report.data import GitRevisionMeta __all__ = ("repo_root",) @@ -124,3 +126,31 @@ def git_https_url_with_commit(path: Path | None = None) -> str | None: url = "https://github.com/" + url.removeprefix(prefix) return f"{url}/tree/{commit}" + + +def git_revision(path: Path) -> GitRevisionMeta | None: + """Describe the checkout that holds path, for run metadata. + + Returns None if path isn't in a git checkout or its revision can't be read. + The checkout counts as dirty if path itself isn't tracked (untracked or + ignored), as the commit doesn't hold it. + """ + root = repo_root(path) + if root is None: + return None + try: + repo = Repo(root) + try: + repo.git.ls_files("--error-unmatch", str(path)) + tracked = True + except GitCommandError: + tracked = False + return GitRevisionMeta( + origin=git_origin_url(path=root), + commit=git_commit_hash(path=root), + commit_short=git_commit_hash(path=root, short=True), + dirty=git_is_dirty(path=root) or not tracked, + ) + except (GitError, ValueError) as err: + log.debug("Cannot read the git revision of %s: %s", root, err) + return None diff --git a/tests/flow/test_extension.py b/tests/flow/test_extension.py index abcacd44..6bda90aa 100644 --- a/tests/flow/test_extension.py +++ b/tests/flow/test_extension.py @@ -130,7 +130,7 @@ def test_lists_follow_the_cfgs_own(self, proj: Path, tmp_path: Path, top: str) - assert blk.build_opts[:3] == [f"-f {proj}/sv.f", "+incdir+/overlay", "/overlay/bind.sv"] assert blk.run_opts[:1] == ["+tool"] assert [t.name for t in blk.tests] == ["t1", "t2"] - assert blk.cfg_extensions == [CfgExtension("blk", ext)] + assert list(blk.cfg_extensions) == [CfgExtension("blk", ext)] def test_imports_and_order(self, proj: Path, tmp_path: Path) -> None: """Extensions apply in command-line order, each with its own imports.""" @@ -144,7 +144,7 @@ def test_imports_and_order(self, proj: Path, tmp_path: Path) -> None: blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={first}", f"blk={second}")) assert blk.run_opts[:4] == ["+tool", "+a", "+a_more", "+b"] - assert blk.cfg_extensions == [CfgExtension("blk", first), CfgExtension("blk", second)] + assert list(blk.cfg_extensions) == [CfgExtension("blk", first), CfgExtension("blk", second)] def test_named_twice_is_an_error( self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture @@ -265,3 +265,26 @@ def test_messages(self) -> None: assert check_cfg_extensions_used(exts[:1], [], ["b"], None) == [ "Cfg extension /a names 'a', but no loaded cfg has that name (loaded: b)." ] + + +class TestRevision: + """The git revision of each extension.""" + + def test_revision_is_recorded(self, proj: Path, tmp_path: Path) -> None: + overlay = tmp_path / "overlay" + ext = _write(overlay / "dv" / "ext.hjson", '{run_opts: ["+x"]}') + git = ["git", "-C", str(overlay)] + subprocess.run([*git, "init", "-q"], check=True) + subprocess.run([*git, "add", "dv"], check=True) + subprocess.run([*git, "commit", "-q", "-m", "init"], check=True) + head = subprocess.run( + [*git, "rev-parse", "HEAD"], check=True, capture_output=True, text=True + ).stdout.strip() + outside = _write(tmp_path / "loose" / "ext.hjson", '{run_opts: ["+y"]}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}", f"blk={outside}")) + + revision = blk.cfg_extensions[CfgExtension("blk", ext)].revision + assert revision is not None + assert (revision.commit, revision.dirty) == (head, False) + assert blk.cfg_extensions[CfgExtension("blk", outside)].revision is None diff --git a/tests/report/test_extensions.py b/tests/report/test_extensions.py new file mode 100644 index 00000000..839ea6f4 --- /dev/null +++ b/tests/report/test_extensions.py @@ -0,0 +1,117 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test how reports show the cfg extensions (--cfg-extend) of a flow.""" + +import json +from datetime import datetime, timezone +from pathlib import Path + +from dvsim.report.data import CfgExtensionMeta, GitRevisionMeta, IPMeta, ToolMeta +from dvsim.sim.data import SimFlowResults, SimResultsSummary +from dvsim.sim.report import HtmlReportRenderer, MarkdownReportRenderer +from dvsim.sim_results import BucketedFailures + +__all__ = () + +TS = datetime(2026, 1, 2, 3, 4, 5, tzinfo=timezone.utc) + + +def _results(*extensions: CfgExtensionMeta) -> SimFlowResults: + return SimFlowResults( + block=IPMeta( + name="blk", + commit="c" * 40, + commit_short="c" * 7, + branch="b", + url=None, + revision_info=None, + ), + tool=ToolMeta(name="vcs", version="1"), + timestamp=TS, + build_seed=None, + testplan_ref=None, + stages={}, + coverage=None, + cov_report_page=None, + vplan_report_page=None, + failed_jobs=BucketedFailures(buckets={}), + passed=1, + total=1, + percent=100.0, + extensions=extensions, + ) + + +def _summary(results: SimFlowResults) -> SimResultsSummary: + return SimResultsSummary( + top=None, + version=None, + timestamp=TS, + build_seed=None, + flow_results={"blk": results.summary()}, + report_path=Path("/reports"), + ) + + +NO_GIT = CfgExtensionMeta(path="/overlay/ok.hjson") +TRACKED = CfgExtensionMeta( + path="/overlay/dv/ext.hjson", + revision=GitRevisionMeta( + origin="git@example.com:org/overlay.git", + commit="a" * 40, + commit_short="aaaaaaa", + dirty=True, + ), +) + + +class TestExtensionsInReports: + """The extensions and their revisions in the results and reports.""" + + def test_summary_keeps_extensions(self) -> None: + assert _results(TRACKED, NO_GIT).summary().extensions == (TRACKED, NO_GIT) + + def test_json_round_trip(self) -> None: + results = _results(TRACKED, NO_GIT) + assert SimFlowResults.model_validate_json(results.model_dump_json()) == results + + def test_json_without_extensions_leaves_out_the_key(self) -> None: + results = _results() + + assert "extensions" not in json.loads(results.model_dump_json()) + flow = json.loads(_summary(results).model_dump_json())["flow_results"]["blk"] + assert "extensions" not in flow + assert SimFlowResults.model_validate_json(results.model_dump_json()) == results + + def test_json_with_extensions_has_the_key(self) -> None: + results = _results(NO_GIT) + + assert json.loads(results.model_dump_json())["extensions"] == [ + {"path": "/overlay/ok.hjson", "revision": None} + ] + flow = json.loads(_summary(results).model_dump_json())["flow_results"]["blk"] + assert flow["extensions"] == [{"path": "/overlay/ok.hjson", "revision": None}] + + def test_markdown_gives_the_revision(self) -> None: + block = MarkdownReportRenderer().render_block(_results(TRACKED, NO_GIT))["report.md"] + + assert ( + "### Cfg extensions:\n\n* `/overlay/dv/ext.hjson` (revision `aaaaaaa`, dirty, " + "from git@example.com:org/overlay.git)\n* `/overlay/ok.hjson`" + ) in block + + def test_markdown_without_extensions(self) -> None: + block = MarkdownReportRenderer().render_block(_results())["report.md"] + + assert "Cfg extensions" not in block + + def test_html_gives_the_revision(self) -> None: + results = _results(TRACKED) + + page = HtmlReportRenderer().render(_summary(results), {"blk": results})["blk.html"] + + assert "Extension: ext.hjson sha: aaaaaaa (dirty)" in page + assert "git@example.com:org/overlay.git" in page + assert "a" * 40 in page diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py index 5516fe22..77386ecf 100644 --- a/tests/utils/test_git.py +++ b/tests/utils/test_git.py @@ -10,11 +10,13 @@ from git import Repo from hamcrest import assert_that, calling, equal_to, raises +from dvsim.report.data import GitRevisionMeta from dvsim.utils.git import ( git_commit_hash, git_https_url_with_commit, git_is_dirty, git_origin_url, + git_revision, repo_root, ) @@ -226,3 +228,50 @@ def test_git_https_url_with_commit(tmp_path: Path, url: str, expected: str) -> N git_https_url_with_commit(tmp_path), equal_to(expected.format(commit=commit)), ) + + @staticmethod + def test_git_revision(tmp_path: Path) -> None: + """Test git_revision describes the checkout holding a file.""" + ext = tmp_path / "sub" / "ext.hjson" + ext.parent.mkdir() + ext.write_text("{}") + + # Not in a git repo, or in one without a commit: nothing to say. + assert_that(git_revision(ext), equal_to(None)) + r = Repo.init(path=tmp_path) + assert_that(git_revision(ext), equal_to(None)) + + r.index.add([ext]) + r.index.commit("initial commit") + commit = r.head.commit.hexsha + + assert_that( + git_revision(ext), + equal_to( + GitRevisionMeta( + origin=None, + commit=commit, + commit_short=git_commit_hash(tmp_path, short=True), + dirty=False, + ) + ), + ) + + # An untracked or ignored file isn't in the commit, so its checkout + # counts as dirty; the other files there are still clean. + untracked = tmp_path / "sub" / "new.hjson" + untracked.write_text("{}") + ignored = tmp_path / "sub" / "ignored.hjson" + ignored.write_text("{}") + (tmp_path / ".git" / "info" / "exclude").write_text("ignored.hjson\n") + assert_that(git_revision(ext).dirty, equal_to(False)) + assert_that(git_revision(untracked).dirty, equal_to(True)) + assert_that(git_revision(ignored).dirty, equal_to(True)) + untracked.unlink() + + r.create_remote("origin", "https://oauth2:token@example.com/org/overlay.git") + ext.write_text("{x: 1}") + revision = git_revision(ext) + assert revision is not None + assert_that(revision.origin, equal_to("https://example.com/org/overlay.git")) + assert_that(revision.dirty, equal_to(True))