feat: expose a build's options to the run step as {build_opts_file} - #256
feat: expose a build's options to the run step as {build_opts_file}#256martin-velay wants to merge 1 commit into
Conversation
A run step that compiles for itself instead of loading the build's
snapshot has to compile the way the build did. Every build now records
its options in build_opts.f, named by the hjson as {build_opts_file}.
Signed-off-by: Martin Velay <mvelay@lowrisc.org>
4843503 to
13e6aca
Compare
rswarbrick
left a comment
rswarbrick
left a comment
There was a problem hiding this comment.
This looks nice, and I'm sorry for not getting back more quickly. I've got some suggestions about how to make the code structure a bit clearer, but I'm a fan of the change.
| 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", | ||
| ) |
There was a problem hiding this comment.
I squinted at this for a few minutes. I'm was sad about the repeated calls to opt.strip() and the fact that we concatenate a bunch of lines, appending \n to each. It took me a while to spot that the reason to do so is to ensure there's a \n at the end of the file.
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")| 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. |
There was a problem hiding this comment.
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?
"""Write build options to the build directory.
Doing so allows a run step to recompile and elaborate for itself, without having to
re-invoke dvsim from scratch. It would be complicated for an external tool to infer these
options: they depend on lots of configuration files. Writing them out here solves that
problem.
"""| # 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. |
There was a problem hiding this comment.
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 :-)
| """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")}, |
There was a problem hiding this comment.
What does cov_db_dir do here and in the next test?
2 participants
Description
A run step that compiles for itself instead of loading the build's snapshot has to compile the way the build did. Every build now records its options in build_opts.f, named by the hjson as {build_opts_file}.