[Builder] Check verify_steps prerequisites are actually run - #1673
Open
mumallaeng wants to merge 1 commit into
Open
[Builder] Check verify_steps prerequisites are actually run#1673mumallaeng wants to merge 1 commit into
mumallaeng wants to merge 1 commit into
Conversation
Each verify_steps entry only ever gets triggered from inside one specific step (e.g. stitched_ip_rtlsim from step_create_stitched_ip). If the configured steps/start_step/stop_step leaves that step out, the entry is silently never verified - the build reports success with no indication that the requested check didn't happen. Add a verify_step_prereq check to run_all_config_checks: resolve the actual step list via resolve_build_steps and flag any verify_steps entry whose owning phase or fine-grained step isn't in it. resolve_build_steps is imported lazily via importlib, since build_dataflow imports this module and a top-level import back would be circular. Verified with pytest locally (setup-local.sh-style venv, no Docker/Vivado) - all 15 tests pass, including 3 new ones covering the missing-phase error, the phase-present pass-through, and a custom steps list naming the fine-grained step directly. Signed-off-by: mumallaeng <mumallaeng@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #1428.
Problem
Each
VerificationStepTypeentry (verify_steps) is only ever triggered from inside one specific step -stitched_ip_rtlsimfromstep_create_stitched_ip,node_by_node_rtlsimfromstep_hw_ipgen, and so on. If the configuredsteps/start_step/stop_stepexcludes that step (e.g.stop_stepset beforephase_build_hardwarewhileverify_stepsstill asks fornode_by_node_rtlsim), the verification is silently never run. The build reports success with no indication that the requested check never happened - exactly the failure mode #1428 describes.Fix
Added a
verify_step_prereqcheck torun_all_config_checksinbuild_dataflow_checks.py. It resolves the actual list of steps that will run viaresolve_build_steps(the same functionbuild_dataflow.pyuses internally) and flags anyverify_stepsentry whose owning phase or fine-grained step isn't in that resolved list.resolve_build_stepsis imported lazily viaimportlib.import_modulerather than a top-level import, sincebuild_dataflow.pyimportsbuild_dataflow_checks.py- a top-level import back would be circular..ruff.tomlenablesPLC(includingPLC0415, import-outside-top-level), so a plain function-localfrom ... import ...would fail lint;importlib.import_moduleisn't anImport/ImportFromAST node, so it isn't flagged.Testing
Ran the full
test_build_dataflow_checks.pysuite locally (setup-local.sh-style venv, no Docker/Vivado) - all 15 tests pass, including 3 new ones: averify_stepsentry whose phase is excluded bystop_step(errors), one whose phase is included (no error), and a customstepslist naming the fine-grained step directly instead of its phase (also no error, since the check accepts either).isort/black/ruff(pinned versions from.pre-commit-config.yaml) pass on both changed files.