Fix #1180 Add the ability to build filters completely locally#1181
Fix #1180 Add the ability to build filters completely locally#1181
Conversation
Strip volatile metadata lines (Checksum, Diff-Path, TimeUpdated, Version) from compiled filter files after build. Reduces noise when comparing outputs between compiler versions.
There was a problem hiding this comment.
Could we use kebab-case for the new filename here?
We are gradually standardizing on this naming style across projects, so for new files it would be better to move in that direction instead of extending the underscore-based pattern.
There was a problem hiding this comment.
and custom_platforms.js, find_files.js?
There was a problem hiding this comment.
It seems not now - used in the comment in FiltersCompiler
https://github.com/AdguardTeam/FiltersCompiler/blob/baa8636e6cac9c60b239520b35c68edea1883f51/src/main/platforms-config.js#L8
- one-liner => helper function - .txt extension to the constant in strip_generated_meta.js
| "build": "tsx scripts/build/build.js", | ||
| "build:local": "tsx scripts/build/build.js --use-cache", | ||
| "build:patches": "node scripts/build/patches.js", | ||
| "strip-generated-meta": "tsx scripts/build/strip-generated-meta.ts", |
There was a problem hiding this comment.
yarn strip-generated-meta points to scripts/build/strip-generated-meta.ts, but this file only exports stripGeneratedMetaFromDir() and never invokes it.
I verified locally that the command exits successfully without touching the generated files. Please add a CLI entrypoint here, or point the npm script to a small wrapper that actually calls the function.
|
|
||
| // Strip generated metadata (Checksum, Diff-Path, TimeUpdated, Version) | ||
| // from compiled filter files so they don't pollute diff comparisons. | ||
| if (stripGeneratedMeta) { |
There was a problem hiding this comment.
This strips metadata only from the new platforms/ tree, but the old baseline may already have been copied to temp/platforms/ above.
That means a later yarn build:patches can diff stripped output against an unstripped baseline and generate patch noise from volatile headers instead of real content changes. Please either make --strip-generated-meta imply --no-patches-prepare, reject the unsafe combination, or strip both trees consistently.
| * @param rootDir - Root directory to search (e.g. `platforms/`). | ||
| * @returns Number of files actually modified. | ||
| */ | ||
| export const stripGeneratedMetaFromDir = async (rootDir: string): Promise<number> => { |
There was a problem hiding this comment.
Non-blocking: this first walks the tree to find every filters/ directory and then does another recursive walk inside each one to collect .txt files. Since we already have a shared recursive file finder, could we avoid the extra traversal and gather the relevant files in one pass?
| /** | ||
| * Compiler entry point. | ||
| */ | ||
| const buildFilters = async () => { |
There was a problem hiding this comment.
This introduces several new execution paths and flag interactions (--use-cache, --generate-cache, --no-patches-prepare, --strip-generated-meta), but the PR does not add any focused coverage for them.
Could we extract the mode selection and argument handling into smaller helpers and add tests for the new combinations? The two blocker scenarios here are exactly the kind of regressions that are easy to miss without targeted coverage.
3 participants
#1180