Lint flow config parser and format checker#118
Lint flow config parser and format checker#118machshev wants to merge 3 commits intolowRISC:masterfrom
Conversation
machshev
force-pushed
the
lint-flow
branch
4 times, most recently
from
ad9071c to
266fb0b
Compare
Move lint flow from flow/lint.py to linting/flow.py to consolidate lint-related code in the linting module. This is part of separating flows into standalone components. Changes: - Move flow/lint.py → linting/flow.py - Rename linting/parser.py → linting/output_parser.py to make room for new config parser - Update imports in factory.py, cdc.py, rdc.py Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
Add Pydantic-based config parser separate from existing flow config infrastructure to enable gradual migration to standalone components. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
Add 'dvsim-admin check' subcommand to validate flow configuration files. The check subsystem detects the flow type and validates the config using the appropriate parser. Currently supports lint flows with extensibility for other flow types. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
| build_dir: str = Field(default="", description="Build directory path") | ||
| build_log: str = Field(default="", description="Build log file path") | ||
| build_cmd: str = Field(default="", description="Build command") |
There was a problem hiding this comment.
I've just had to do some reading to understand how the default parameter works in Pydantic's Field function. It's rather clever!
If I understand correctly, these lines mean that build_dir, build_log and build_cmd are optional and default to "".
If so, wouldn't it make more sense to give them type str | None and default to None? Also, I think this is currently the only place they are documented. Maybe there needs to be some text that explains what the default means?
| build_opts: list[str] = Field(default_factory=list, description="Build options") | ||
|
|
||
| # FuseSoC configuration | ||
| fusesoc_core: str = Field(default="", description="FuseSoC core to use") |
There was a problem hiding this comment.
Similarly, if this is optional I think it should probably be str | None and default to None, and it also probably needs some documentation to explain what the default means.
Same for additional_fusesoc_argument below.
| # Lint-specific configuration | ||
| is_style_lint: bool = Field(default=False, description="Whether this is a style lint run") | ||
| report_severities: list[str] = Field( | ||
| default_factory=lambda: ["info", "warning", "error"], |
There was a problem hiding this comment.
Clearly for a later PR, but this really looks like it should be an enum.
|
|
||
| # Tool configuration | ||
| tool: str | None = Field(default=None, description="Lint tool to use") | ||
| dut: str = Field(default="", description="Design under test name") |
There was a problem hiding this comment.
Again, the default should probably be None and maybe the documentation needs to explain what that means.
Similarly for scratch_path and rel_path below.
| """ | ||
| hjson_path = Path(hjson_path) | ||
|
|
||
| if not hjson_path.exists(): |
There was a problem hiding this comment.
Can't this be folded into the next try block? I think it duplicates the OSError case at line 52.
| except hjson.HjsonDecodeError as e: | ||
| msg = f"Failed to parse hjson: {e}" | ||
| raise FlowCheckError(msg) from e | ||
| except OSError as e: |
There was a problem hiding this comment.
Hmm. Is this the right thing to do? We end up squashing "bad config" and "nonexistent config" as a result. I think the API would probably be better if they were left separate?
| """ | ||
| hjson_path = Path(hjson_path) | ||
|
|
||
| if not hjson_path.exists(): |
There was a problem hiding this comment.
If we don't squash the errors in the previous function, this could be folded into the next try block.
| # Check based on flow type | ||
| if flow_type == "lint": | ||
| try: | ||
| config = parse_lint_flow_config(hjson_path) |
There was a problem hiding this comment.
This is upsetting: it will load and parse the hjson file twice. Can't we change detect_flow_type and parse_lint_flow_config so that they both take the parsed dictionary object?
| try: | ||
| config = parse_lint_flow_config(hjson_path) | ||
| except (FileNotFoundError, RuntimeError) as e: | ||
| return False, f"Invalid lint flow config: {e}", flow_type |
There was a problem hiding this comment.
Hmm. I'm not convinced about the description here (and not really convinced about globbing the two error types either)
| config = parse_lint_flow_config(hjson_path) | ||
| except (FileNotFoundError, RuntimeError) as e: | ||
| return False, f"Invalid lint flow config: {e}", flow_type | ||
| else: |
There was a problem hiding this comment.
I wouldn't suggest this structure. Since there is no finally, I think the code is probably more understandable if it's "Do X, returning early on an exception. Now report success".
2 participants
First step towards moving towards full schema checked and validated config files. This PR adds a lint config model, a parser, and a checker which can be used to check lint flows. Both unit tests and integration tests are added to ensure the correct format.
The hjson config file check tool has been run on the OT lint hjson files to make sure we can load them. The model uses pydantic strict mode which does not allow extra fields. Which means we have high confidence that the model/parser are correct.
Next step would be to refactor the lint flow to make use of this model/parser, as well as separate it out from the FlowCfg base class. At the moment we inherit too many behaviours from the base class and it's not clear which parts are required in the lint flow and which are not.