Fix stale progress counters for console/restat edges (#1336, #2507)#2724
Open
jkammerland wants to merge 4 commits intoninja-build:masterfrom
Open
Fix stale progress counters for console/restat edges (#1336, #2507)#2724jkammerland wants to merge 4 commits intoninja-build:masterfrom
jkammerland wants to merge 4 commits intoninja-build:masterfrom
Conversation
Author
|
Here is how you can test that the regression tests I added fail before this patch, and succeed after: # First checkout my fork!
# from repo root
PRE_FIX=$(git merge-base HEAD origin/master)
WT=.worktrees/pre-fix-progress
git worktree add "$WT" "$PRE_FIX"
# bring current regression tests into pre-fix tree
git show HEAD:misc/output_test.py > "$WT/misc/output_test.py"
# pre-fix run: should FAIL
(
cd "$WT"
./configure.py --bootstrap
python3 -m pytest -q misc/output_test.py -k \
"issue_1336_dumb_tty or restat_prunes_progress_total or generator_restat_prunes_progress_total"
)
# current PR run: should PASS
(
cd .
./configure.py --bootstrap
python3 -m pytest -q misc/output_test.py -k \
"issue_1336_dumb_tty or restat_prunes_progress_total or generator_restat_prunes_progress_total"
)
# cleanup (comment out if you want)
# git worktree remove --force "$WT" |
Fixes console-edge completion progress reporting on dumb terminals and includes related status-policy cleanup plus NINJA_STATUS-empty regression coverage.
Status was emitted before plan_.EdgeFinished() updated the plan. For restat edges (including CMake glob checks and gentest codegen), this left the denominator stale for one print and produced jumps like [1/14] -> [2/6] or no-op [0/2]. Finalize the edge in the plan first, then print completion status via FinalizeEdgeAndReportStatus(), and cover both restat and generator+restat cases with regression tests.
Author
|
To improve UX further we could also try to mark "phase boundary" to better understand the progress of different steps. E.g [1/2] Re-checking globbed directories...
[2/2] Re-running CMake...
ninja: regeneration complete... <---- NEW
[1/1] Re-checking globbed directories...
ninja: manifest check complete... <---- NEW
[1/2] Building ...
[2/2] Linking ...But I have not included any such change here, better put that in a new PR after discussion/approval. I'm also looking a bit at #2709, while I'm at it - but I will make no changes related to that here. |
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.
This PR fixes two progress-reporting issues in Ninja where [current/total] could be incorrect:
final console steps (for example install) could show [N-1/N] and never reach [N/N].
progress could start with an old total (for example [1/14]) and then “jump down” (for example [2/6]) once pruned edges were removed from the plan.
The fixes are limited to status/progress reporting behavior and regression coverage. I have tested on linux and windows, for multiple of my non-trivial repos with code generation involved. I used llms (codex) to help me implement this.
Problem details
#1336 (console edge in dumb TTY)
In non-smart TTY mode, console edges were shown at start, but completion reporting did not consistently advance %f/%t to the final count.
That produced output like:
[1/5] ...
[2/5] ...
[3/5] ...
[4/5] ...
[4/5] Installing files.
#2507 (restat/generator pruning)
Status printing happened before plan finalization (plan_.EdgeFinished()), so %t could be computed from a stale pre-prune total for one print.
That produced patterns like:
[1/14] Running gentest_codegen ...
[2/6] Building ...
or no-op glob checks like:
[0/2] Re-checking globbed directories...
ninja: no work to do.
What changed
1) Status policy cleanup for console vs non-console edges (src/status_printer.cc)
2) Finalize plan before printing completion (src/build.cc, src/build.h)
3) Regression tests (misc/output_test.py)
Added/updated tests for:
Before / after examples
Install-style console final edge (#1336)
Before:
[4/5] Installing files.
After:
[5/5] Installing files.
Globbing/no-op check (#2507)
Before:
[0/2] Re-checking globbed directories...
ninja: no work to do.
After:
[1/1] Re-checking globbed directories...
ninja: no work to do.
Codegen + prune
Before:
[1/14] Running gentest_codegen ...
[2/6] Building ...
After:
[1/3] Running gentest_codegen ...
[2/3] Building ...
[3/3] Linking ...
Scope / risk statement
This change is intended to affect status/progress output only:
It should not change actual build decisions, dependency resolution, command execution, or produced artifacts.
The only externally visible behavior change is progress/log line content and timing of those status lines.