-
Notifications
You must be signed in to change notification settings - Fork 21
feat: expose a build's options to the run step as {build_opts_file} #256
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 |
|---|---|---|
|
|
@@ -431,6 +431,7 @@ def __init__(self, build_mode: "BuildMode", sim_cfg: "SimCfg") -> None: | |
| self.build_cmd: str = "" | ||
| self.build_dir: str = "" | ||
| self.build_opts: list[str] = [] | ||
| self.build_opts_file: str = "" | ||
| self.post_build_cmds: list[str] = [] | ||
| self.build_fail_patterns: list[str] = [] | ||
| self.build_pass_patterns: list[str] = [] | ||
|
|
@@ -485,6 +486,7 @@ def _define_attrs(self) -> None: | |
| self.mandatory_misc_attrs.update( | ||
| { | ||
| "build_fail_patterns": False, | ||
| "build_opts_file": False, | ||
| "build_pass_patterns": False, | ||
| "build_timeout_mins": False, | ||
| "cov_db_dir": False, | ||
|
|
@@ -509,6 +511,25 @@ def _set_attrs(self) -> None: | |
| if self.sim_cfg.args.build_timeout_mins is not None: | ||
| self.build_timeout_mins = self.sim_cfg.args.build_timeout_mins | ||
|
|
||
| def _write_build_opts_file(self) -> None: | ||
| """Record the options this build used, in the build directory. | ||
|
|
||
| A run step may compile and elaborate for itself instead of loading the snapshot the build | ||
| produced. Such a run has to compile the way the build did. Build modes and individual cfgs | ||
| each contribute their own defines, include paths and libraries, and a run compiled with a | ||
| different set would simulate a differently configured design. | ||
|
|
||
| The merged option list exists only here, so write it out as a file the tools accept in | ||
| place of command line options. The run step names that file with {build_opts_file}, which | ||
| SimCfg sets and the HJson may override. | ||
| """ | ||
| opts_file = Path(self.build_opts_file) | ||
| opts_file.parent.mkdir(parents=True, exist_ok=True) | ||
| opts_file.write_text( | ||
| "".join(f"{opt.strip()}\n" for opt in self.build_opts if opt.strip()), | ||
| encoding="UTF-8", | ||
| ) | ||
|
Comment on lines
+526
to
+531
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 squinted at this for a few minutes. I'm was sad about the repeated calls to I think the following structure is probably a bit simpler to understand, and also splits up "render an option" from "write a bunch of options to a file". What do you think? maybe_options = [opt.strip() for opt in self.build_opts]
options = [opt for opt in maybe_options if opt]
opts_file = Path(self.build_opts_file)
opts_file.parent.mkdir(parents=True, exist_ok=True)
opts_file.write_text("\n".join(options) + "\n", encoding="UTF-8") |
||
|
|
||
| def pre_launch(self) -> Callable[[], None]: | ||
| """Get pre-launch callback.""" | ||
|
|
||
|
|
@@ -518,6 +539,8 @@ def callback() -> None: | |
| # need to do this because the build directory is not 'renewed'. | ||
| rm_path(Path(self.cov_db_dir)) | ||
|
|
||
| self._write_build_opts_file() | ||
|
|
||
| return callback | ||
|
|
||
| def get_timeout_mins(self) -> float: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,6 +134,7 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: | |
| self.post_build_cmds = [] | ||
| self.post_build_opts = [] | ||
| self.build_dir = "" | ||
| self.build_opts_file = "" | ||
| self.pre_run_cmds = [] | ||
| self.post_run_cmds = [] | ||
| self.run_dir = "" | ||
|
|
@@ -200,6 +201,13 @@ def _expand(self) -> None: | |
| if self.args.verbosity is not None: | ||
| self.verbosity = self.args.verbosity | ||
|
|
||
| # Where each build records the options it compiled with, for a run step that compiles for | ||
| # itself rather than loading the snapshot the build produced, see | ||
| # CompileSim._write_build_opts_file(). The HJSON can name this path as {build_opts_file}, | ||
| # and can also set it, to move the file or to share one between cfgs. | ||
|
Comment on lines
+204
to
+207
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. Please could this comment move to the "declaration" of the field in constructor? (I know that dvsim isn't great at this! But here's an opportunity to do it right :-) |
||
| if not self.build_opts_file: | ||
| self.build_opts_file = "{build_dir}/build_opts.f" | ||
|
|
||
| super()._expand() | ||
|
|
||
| if self.variant: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| """Test Job deployment models.""" | ||
|
|
||
| from collections.abc import Mapping | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from hamcrest import assert_that, equal_to | ||
|
|
@@ -48,6 +49,7 @@ def __init__(self) -> None: | |
| self.pre_build_cmds = ["A", "B"] | ||
| self.post_build_cmds = ["C", "D"] | ||
| self.build_dir = "build/dir" | ||
| self.build_opts_file = "{build_dir}/build_opts.f" | ||
| self.build_pass_patterns = None | ||
| self.build_fail_patterns = None | ||
| self.build_seed = 123 | ||
|
|
@@ -202,6 +204,49 @@ def test_seed( | |
|
|
||
| assert_that(job.seed, equal_to(seed)) | ||
|
|
||
| @staticmethod | ||
| def test_build_opts_file(tmp_path: Path) -> None: | ||
| """Test that a CompileSim records the options it built with, for the run step.""" | ||
| build_dir = tmp_path / "build" / "dir" | ||
| job = _build_compile_sim( | ||
| sim_overrides={"build_dir": str(build_dir), "cov_db_dir": str(tmp_path / "cov")}, | ||
|
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. What does |
||
| ) | ||
|
|
||
| assert_that(job.build_opts_file, equal_to(str(build_dir / "build_opts.f"))) | ||
|
|
||
| # The build directory does not exist until the build job launches. | ||
| job.pre_launch()() | ||
|
|
||
| assert_that( | ||
| Path(job.build_opts_file).read_text(encoding="UTF-8"), | ||
| equal_to('-b path/here\n-a "Quoted"\n'), | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def test_build_opts_file_from_cfg(tmp_path: Path) -> None: | ||
| """Test that a cfg-supplied build_opts_file is used as given. | ||
|
|
||
| SimCfg only fills in a default, so an HJson that sets build_opts_file itself can put the | ||
| file outside the build directory. | ||
| """ | ||
| opts_file = tmp_path / "elsewhere" / "opts.f" | ||
| job = _build_compile_sim( | ||
| sim_overrides={ | ||
| "build_dir": str(tmp_path / "build" / "dir"), | ||
| "build_opts_file": str(opts_file), | ||
| "cov_db_dir": str(tmp_path / "cov"), | ||
| }, | ||
| ) | ||
|
|
||
| assert_that(job.build_opts_file, equal_to(str(opts_file))) | ||
|
|
||
| job.pre_launch()() | ||
|
|
||
| assert_that( | ||
| opts_file.read_text(encoding="UTF-8"), | ||
| equal_to('-b path/here\n-a "Quoted"\n'), | ||
| ) | ||
|
|
||
| @staticmethod | ||
| @pytest.mark.parametrize( | ||
| ("cli_args_overrides", "build_overrides", "timeout"), | ||
|
|
||
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.
I spent a while trying to work out whether there was a more concise way to write this, and make a docstring that's easier to understand. I think this contains all the information we need to convey, and is much easier to understand. Does it look reasonable to you?