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)." + ]