feat: add new npm script to compare the build output against master - #1212
feat: add new npm script to compare the build output against master#1212mu-hun wants to merge 29 commits into
Conversation
@Alex-302 I will keep this PR in draft status until the above problem is fixed. Please take a look for an initial review. |
mu-hun
left a comment
mu-hun
left a comment
There was a problem hiding this comment.
No diffs were reported after running twice. so, I'm converting this PR as ready for review.
Step 8/9: Report
=== Regression Test Report ===
Branch (reference): master @ 29ee5260e069fdb10a9873babfb32a213e06f57c
Branch (feature): feature/#1211 @ 400ad9c11100f9c634aeeb6077bec194eb5d7dc6
Build command: yarn generate-cache && yarn build:local --no-patches-prepare --strip-generated-meta
Platforms compared: android cli extension ios mac mac_v2 mac_v3 windows
--- Rule Files ---
Total .txt files compared: 3361
Files with diffs: 0
--- Metadata Files (informational) ---
filters.json/filters.js diffs: 32 (version counter noise, not a regression)
--- Verdict ---
✓ PASS — no rule file diffs
Step 9/9: Cleanup
✓ cleaning up worktrees and build output
✨ Done in 553.45s.
| # --- Step 6: sync changed worktree's filters/ to the $BASED_BRANCH baseline --- | ||
|
|
||
| step_header 6 "Sync filters/ baseline" | ||
| if ! run_with_spinner "syncing filters/ to $BASED_BRANCH baseline" "$LOG_SYNC_BASELINE" \ | ||
| git -C "$CHANGED_WORK_TREE" checkout "$MASTER_SHA" -- filters/; then |
There was a problem hiding this comment.
I need to double-check if we actually need a step to sync the filters/ directory.
I'm wondering if the --use-cache flag is supposed to skip the filters directory build, and if so, how that skip actually works. The arguments being passed to the compile function look similar, so while reviewing, it occurred to me that it might be downloading filters from a remote source and building them there.
FiltersRegistry/scripts/build/build.js
Lines 143 to 152 in 202ea67
There was a problem hiding this comment.
P.S. How about requesting a review from the dev team after our team review is done?
This instruction was originally written at 5b7f70f#diff-fc2a310fcfedfefb0046def697f932def80cbb1e78c689bc0af3c8eab3e33eceR116-R132 with dev team review.
There was a problem hiding this comment.
Do you mean reviev of the SH script?
There was a problem hiding this comment.
P.S. How about requesting a review from the dev team after our team review is done?
Right. This shell script.
There was a problem hiding this comment.
I'm wondering if the
--use-cacheflag is supposed to skip the filters directory build, and if so, how that skip actually works. The arguments being passed to the compile function look similar, so while reviewing, it occurred to me that it might be downloading filters from a remote source and building them there.
I'm waiting for an answer to my question above. Is it possible to skip the filters directory build when the compile function is emitted with the passed cachedFiltersDir?
@slvvko I checked your most recent work in this project. Can you answer my question? cc @maximtop
https://github.com/AdguardTeam/FiltersRegistry/graphs/contributors?from=7%2F19%2F2025
There was a problem hiding this comment.
I'm wondering if the --use-cache flag is supposed to skip the filters directory build
Doesn't this skip the build? The option itself involves the use of local filter.txt files
There was a problem hiding this comment.
It doesn't skip the filters directory build — compile() runs the full pipeline either way, and the arguments look similar on purpose.
What --use-cache changes is the input: before compiling, we copy filters to temp/filters_cached/ and replace every template.txt with a shadow template containing only @include "./filter.txt". So when the compiler calls FiltersDownloader.resolveIncludes, it resolves a local file instead of downloading remote sources. The compilation itself (platform outputs, removeRulePatterns, patches) still runs, which is required so that --use-cache produces identical output to a normal build.
There was a problem hiding this comment.
- Add detection of old results when executing and a request to remove it
- Path to logs better print complete (support Win/Mac). It is hard to find the location.
Also I suggested to use temp dir of the repo - in any case it is gitignored.
-
Logs extension must be
.log -
Use cached build (generate-cache + build:local) instead of a plain build?
better Use cached sources instead of a regular build? without (generate-cache + build:local) - it is explained in docs.
- Step 2/9: Build mode
Use cached build (generate-cache + build:local) instead of a plain build?
[Y/N] (default: Y):
I think the default shoulde No
- Step 9/9: Cleanup
→ Kept for later use: temp/reg-master-build, temp/reg-changed-build, platforms_master_build, platforms_changed_build
=> Cleanup skipped. + paths.
Btw why platforms_changed_build and platforms_master_build are created in the root of the repo, but not in temp dir or the repo?
|
Please also check AI review Bugs
Robustness
Maintainability
|
|
Since
|
3 items are real to scope for this fixes. 6 of the 12 are false positives — already handled correctly:
Interactive-only design is intended for checking locally. Should I make compatibility to CI? How to test it?It could test at
Because. It's hard to manually swap between the 7 - 9 steps. |
No. For local testing only. At least for now, there's no need for that. |
mu-hun
left a comment
mu-hun
left a comment
There was a problem hiding this comment.
The regression test script is now runnable.
Step 9/11: Build both branches
✓ [master] build ✓ [feature/#1211] build
✗ [master] build FAILED — see /Users/muhun/github.com/AdguardTeam/FiltersRegistry/temp/logs/master-build.log
--- /Users/muhun/github.com/AdguardTeam/FiltersRegistry/temp/logs/master-build.log ---
(node:20997) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
(Use `node --trace-deprecation ...` to show where the warning was created)
error Command "download-stats" not found.
Please clear the yarn cache for the filters-compiler version to refresh the unreleased development version — yarn cache clean @adguard/filters-compiler. Without doing this, you will encounter the following error:
import { compile, localOptimizationStatistics, OptimizationStatsError } from '@adguard/filters-compiler';
^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@adguard/filters-compiler' does not provide an export named 'OptimizationStatsError'
|
@mu-hun it seems broken usage of 
Even 
In master it works. Please check. According to docs it must be supported. Also ask llm to perform review against AI Review: PR #1186 — Local downloading of optimization statistics for the buildScope: the PR branch ( The PR itself is solid overall. The findings below are ordered by impact. For each
Origin verified against 1.
|
@Alex-302 Please update the
It works after updated. |
Weird, sure. But modules were frest 
Must work. In build plan 
|
|
I have push new commit to the parent PR, describe this details at ea70a38 |
The Based on the current source code implementation: export class OptimizationStatsError extends Error {
code = 'OPTIMIZATION_STATS_UNAVAILABLE' as const;
constructor(
public filterId: number,
public sourcePath: string,
options?: ErrorOptions,
) {
super(
`Unable to retrieve optimization stats for ${filterId}, at ${sourcePath}. `
+ 'Please ensure the stats file exists and is accessible.',
options,
);
this.name = 'OptimizationStatsError';
}
} |
|
There might be a bug in the master |
|
Regarding #1186 (comment), We agreed to prevent the simultaneous use of both flags within |
It does not look like a problem. |
|
Okay, I'll allow the combination to Note that |
Turns the manual copy-paste workflow from DEVELOPMENT.md into a single interactive command: prompts for the branch to compare, build mode, and cleanup preference, then builds master and the branch in parallel git worktrees with a progress spinner, and prints the same pass/fail rule-file comparison as before. Resolves #1211
The report's diff loop only walked platforms_master_build/, so a .txt file the compare branch adds that master doesn't have was invisible to the comparison and could report PASS despite a real, uncompared change.
git worktree add can fail as "already registered" if temp/ was ever deleted manually instead of via this script's own cleanup, leaving stale bookkeeping under .git/worktrees/. Confirmed this repo already has one such stale entry (reg-changed-local).
Two simultaneous invocations collide on the same fixed paths (temp/reg-master-build, temp/reg-meta.env, same log files) with no detection — last writer wins, silently.
Was hardcoded identically in build_branch() and in generate_report()'s displayed build_cmd string.
Applied to the 4 call sites that report and exit immediately (worktree setup, both installs, filters/ sync). The other 5 report_failure sites don't exit immediately (they set BUILD_FAILED and keep going, or just warn on cleanup), so they keep calling report_failure directly.
[skip ci] I'm planning to run after next released FiltersCompiler version. Per team discussion, --generate-cache and stats download aren't required on every cached-mode run Adds two new steps (3: Generate cache, 4: Download stats), both defaulting to No/skip, shown only when cached build mode was chosen in Step 2. build_branch() now conditionally runs `yarn generate-cache` and/or `yarn download-stats` before build:local based on the answers, instead of always running generate-cache. The choices are also persisted to $META_FILE so a reused report (Step 0) shows the build command that was actually run. TOTAL_STEPS bumped 9 -> 11, renumbering steps 3-9 to 5-11.
TOTAL_STEPS back to 9, steps renumbered 4-9 accordingly.
Based on #1226 (comment) @Alex-302 comment.
maximtop
left a comment
maximtop
left a comment
There was a problem hiding this comment.
The cleanup step is the one that needs fixing before merge — it deletes the whole shared temp/ directory, not just this run's artifacts. Rest is inline.
| git worktree remove "$CHANGED_WORK_TREE" -f 2>/dev/null || rm -rf "$CHANGED_WORK_TREE" | ||
| # Remove everything under $TEMP_DIR_NAME except $LOG_DIR_NAME, so logs | ||
| # from this run stay available for inspection after cleanup. | ||
| find "$TEMP_DIR_NAME" -mindepth 1 -maxdepth 1 ! -name "$LOG_DIR_NAME" -exec rm -rf {} + |
There was a problem hiding this comment.
This deletes far more than the script creates: it also removes temp/optimization/stats/, temp/platforms/ (the patch-generation baseline) and temp/filters_cached/. The cleanup prompt only mentions worktrees and platforms_*_build, so answering "yes" silently destroys state that yarn download-stats, yarn build:patches and cached builds rely on. Please restrict it to the four paths this script actually created (the two worktrees plus the two platforms_*_build dirs).
|
|
||
| if [ -f "$META_FILE" ] && [ -d "$PLATFORMS_MASTER" ] && [ -d "$PLATFORMS_CHANGED" ]; then | ||
| # shellcheck disable=SC1090 | ||
| source "$META_FILE" |
There was a problem hiding this comment.
The meta file is written with unquoted values (CHANGED_BRANCH=$CHANGED_BRANCH below) and loaded here with source. Git refnames allow $() and backticks, so a branch named feature/$(...) would execute on the next run that reaches Step 0. Quoting the values on write (CHANGED_BRANCH="$CHANGED_BRANCH") or parsing the file line-by-line instead of sourcing closes it.
| - If `platforms_master_build/` and `platforms_changed_build/` already exist | ||
| from a previous run, it offers to generate the report from them instead of rebuilding. | ||
| - If a worktree from a previous run is still present, it offers to reuse it | ||
| (keeping `node_modules`, skipping reinstall) instead of recreating it from |
There was a problem hiding this comment.
This claims worktree reuse skips the reinstall, but Step 5 runs yarn install unconditionally (since c794cb8) — the documented fast path doesn't exist. Either fix the doc or make the install conditional.
There was a problem hiding this comment.
I'm considering to fixing the docs. But I'm unsure about the wordings.
- If a worktree from a previous run is still present, it offers to reuse it
instead of removing and re-adding it.yarn installstill runs, but over
the keptnode_modulesit only reconciles what changed. - If a worktree from a previous run is still present, it offers to reuse it
instead of removing and re-adding it.yarn installstill runs — the
reused tree is checked out onto a different commit. but against the kept
node_modules, not from scratch. - If a worktree from a previous run is still present, it offers to reuse it
instead of removing and re-adding it. Worth it when the compared commits
haven't changed much since the last run;yarn installstill runs, just
incrementally over the keptnode_modules.
Which one do you think is best? 🤔
| echo "Reuse it instead of recreating?" | ||
| if confirm default_no; then | ||
| echo "${C_CYAN}${ARROW}${C_RESET} [$label] reusing existing worktree at $path" | ||
| git -C "$path" checkout -f "$sha" |
There was a problem hiding this comment.
If this checkout fails (locked files, broken worktree), the function still returns success and the build runs against whatever content happens to be in the worktree — a comparison against the wrong code with no error surfaced. Check the exit status before return 0.
| # e.g. if temp/ was ever deleted manually instead of via `git worktree remove`. | ||
| # To verify: `rm -rf temp/reg-master-build`, then rerun — `git worktree add` | ||
| # would otherwise fail as "already registered". | ||
| git worktree prune |
There was a problem hiding this comment.
Running git worktree prune at startup touches the whole repository, not just this script's worktrees — it can remove admin files for unrelated worktrees the user still considers valid (e.g. one on an unmounted volume). Scope it to the two known paths or drop it.
|
|
||
| BUILD_FAILED=false | ||
|
|
||
| if wait "$PID_MASTER_BUILD"; then |
There was a problem hiding this comment.
The master/changed result-collection blocks here and just below are near-identical (~13 lines each), differing only in labels and variable names. A small helper taking label + pid + log path would collapse them.
Code Review — combined (3 reviewers)Reviewed via three independent review passes (open-code-review/OCR, a detailed line-by-line review, and a generic review sub-agent), then deduplicated and re-verified against the head of this PR. Scope: The script is well-written, well-commented, and faithfully automates the manual workflow — parallelization, locking, pass/fail propagation, and the new reverse "ADDED" check are sound, and several worried-about behaviors (worktree re-add after HighH1 — The heredoc writes fields unquoted, including Suggested fix: write values shell-safe, e.g. H2 — Ctrl-C can't stop the run; forced termination orphans the build children (concurrency/signals) Background children are spawned with job control off (they inherit Suggested fix: MediumM1 — "Removing previous build output" removes nothing; a partial failure leaves a stale/mixed comparison that the next run silently reuses The dirs are only removed inside each branch's success path. If the master build succeeds and the changed build fails, the script exits at Suggested fix: M2 — Worktree reuse: discarded
Suggested fix: M3 — Cleanup wipes unrelated
Suggested fix: scope deletion to the script's own artifacts (the two worktrees + M4 — Doc/code mismatch: "skipping reinstall" vs always-install The doc promises a reused worktree means "keeping Suggested fix: reword the doc ("keeping node_modules so Low / nits
Discarded after double-check: Summary
|
Align to other shell scripts with pure whitespace change. Suggested in #1212 (comment)
Suggested in #1212 (comment)
platforms_master_build/ and platforms_changed_build/ live under temp/, not the repo root. Discussed in #1212 (comment)
Print recorded short SHAs under the prompt to identify when kept output matches fresh commits. Append "(now at <short>)" to branch lines when the branch has moved since the last build to clarify stale reuse status. Suggested in #1212 (comment)
The workflow section framed the cached path as one "generate-cache + build:local" choice; This script actually asks three yes/no prompts: cached sources, then generate-cache, then download-stats. Mentioned in #1212 (comment)
5 participants
Turns the manual copy-paste workflow from
DEVELOPMENT.mdinto a single interactive command. Please see details in the below issue.Note
fix:, which were not supported in the previous script.