flow: flow.sh runs the stage through RUN_CMD, like the rest of the flow - #4555
Merged
maliberty merged 3 commits intoSep 26, 2026
Merged
Conversation
genElapsedTime.py writes the per-stage summary, the only place ORFS
reports a hash of a stage's result. It read the elapsed time
positionally: strip the line, delete "Elapsed time: ", split on "[h:]",
on the first "." and on ":". That works only when the timing line starts
the log line. Give it a prefix, as a RUN_CMD wrapper that stamps log
lines does ("[ 123.45] "), and the prefix survives the delete, the
split on "." cuts at the prefix's own decimal point, and the line falls
to "Elapsed time not understood". The row is then skipped: elapsed,
peak memory and the sha1sum column vanish, "No elapsed time found" does
not fire because the line was found, and the exit status is 0.
Match the timing line with a regex anchored on its format instead,
hours optional as run_command.py only emits them past an hour. An
unprefixed line gives exactly the number it did before, fraction of a
second dropped as before, and an unparseable line still reaches "not
understood".
test_genElapsedTime_parses_output asserted only that the stage name
appeared, which it does even when the row is dropped; it now asserts
the elapsed and memory columns. Two cases are added: a prefixed line,
which fails before this change, and the hours form.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
variables.mk defines RUN_CMD as the way a logged tool invocation is run, and every logged target in flow/Makefile goes through it: yosys metrics, generate_abstract, merge, drc, cdl, lvs, `run`. The exception was flow.sh, which every stage goes through, and which spelled out `$PYTHON_EXE $SCRIPTS_DIR/run_command.py` by hand. RUN_CMD governed the peripheral logs and none of the stage logs; an override (to stamp lines, tee elsewhere, wrap the tool) silently missed floorplan, place, cts, grt and route. flow.sh now runs the stage through RUN_CMD, defaulting to the command it spelled out, so a plain `make` is unchanged. Depends on the previous commit: flow.sh parses the stage log back with genElapsedTime.py, which until then dropped the summary row for a line a wrapper had prefixed. flow/test/test_flow_sh.py runs flow.sh with stub tools: with RUN_CMD unset the stage log has its timing line and summary row; with RUN_CMD set to a recording wrapper the stage goes through it and the summary row is still there. The second fails before this change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates flow.sh to run stages through a configurable RUN_CMD wrapper and refactors genElapsedTime.py to use a regular expression for parsing elapsed times, allowing it to handle log prefixes and optional hour fields. Comprehensive unit tests are also introduced. The review feedback recommends using a Bash array for RUN_CMD to prevent word-splitting issues if paths contain spaces.
Member
🔍 QoR checkMetrics reflect the PR merge build — i.e. what will land on the target branch. Commit 62 design(s) checked — 0 with regression(s), 0 without a comparable baseline. |
maliberty
approved these changes
Sep 26, 2026
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
3 participants
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.
What
variables.mkdefinesRUN_CMDas the way a logged tool invocation is run, and every logged target inflow/Makefilegoes through it: yosys metrics,generate_abstract, merge, drc, cdl, lvs,run. The exception isflow.sh, which every stage goes through and which spelled out$PYTHON_EXE $SCRIPTS_DIR/run_command.pyby hand. SoRUN_CMDgoverned the peripheral logs and none of the stage logs; an override (stamp lines, tee elsewhere, wrap the tool) silently missed floorplan, place, cts, grt and route.Fix
flow.shruns the stage throughRUN_CMD, defaulting to the command it spelled out, so a plainmakeis unchanged.Depends on
#4554:
flow.shparses the stage log back withgenElapsedTime.py, which until that fix dropped the summary row for a prefixed timing line, so this change alone would hand that regression to anyone whoseRUN_CMDprefixes lines. This branch is on top of it.Test
New
flow/test/test_flow_sh.pyrunsflow.shwith stub tools (OPENROAD_EXE=true,OPENROAD_CMD=echo):RUN_CMDunset: the stage log has its timing line and summary row.RUN_CMDset to a recording wrapper: the stage goes through it, and the summary row is still there. Fails before this change ("RUN_CMD did not run the stage").flow/test/*.pyall pass; black clean.🤖 Generated with Claude Code