Benchmark: ignored root file + single package change - #9489
Closed
Mrtenz wants to merge 31 commits into
Closed
Conversation
Mrtenz
force-pushed
the
mrtenz/benchmark-mixed
branch
2 times, most recently
from
July 15, 2026 08:45
01c4c42 to
6dadbcd
Compare
4 tasks
Instead of rebuilding all 89 packages on every CI run, use `yarn workspaces foreach --since` to build only the packages that changed relative to the target branch, plus their transitive dependants. Falls back to a full `yarn build` on push-to-main events where no base branch ref is available.
`--depth=1` on the base branch fetch doesn't give git enough history to find the common ancestor with the PR branch, causing yarn's `--since` to fail. Replace with `--no-tags` (full history, no tags).
The `action-checkout-and-setup` does a `fetch-depth: 1` shallow clone, so the PR branch history only goes back one commit. Git cannot find the merge base against `origin/$BASE_REF` from such a shallow history, causing yarn's `--since` to fail with "No ancestor could be found". Unshallow the checkout first, so the full history is available for the merge-base computation.
Instead of heuristic depth fetches, call the GitHub Compare API to get the exact merge base SHA, then fetch only that single commit. This avoids fetching large chunks of history while being precise regardless of how old the branch is.
The previous approach fetched the merge base commit but git still couldn't confirm it was an ancestor of HEAD because the PR branch checkout is depth=1. Fix by unshallowing the checkout using `--filter=blob:none`, which fetches full commit and tree history without downloading file content — sufficient for `git diff --name-only`.
Without an explicit refspec, `git fetch --unshallow` deepens all refs that were fetched during checkout. Passing `origin HEAD` limits it to the currently checked-out ref.
Instead of running `yarn workspaces foreach --since` sequentially, generate a temporary tsconfig that lists only changed packages and their transitive dependants as project references. This lets ts-bridge use TypeScript's project-references to parallelise the build.
mktemp creates the file in /tmp by default, causing ts-bridge to resolve relative package paths against /tmp instead of the repo root. Using --tmpdir="$GITHUB_WORKSPACE" and cleaning up with rm -f after the build keeps relative paths correct without leaving a dirty tree.
…dencies Three bugs: - actions/checkout checks out the merge commit (PR + base), so `git diff $MERGE_BASE...HEAD` included all of main's changes. Fix: pass the explicit PR head SHA and diff against that instead. - Packages included as dependants of changed packages couldn't build because their own dependencies had no dist files. Fix: expand the build set to include transitive dependencies as well. - ts-bridge rejects a tsconfig with `files: []` and no references. Fix: skip ts-bridge entirely when no packages need building.
Extract shared workspace logic into scripts/lib/workspaces.mts and add scripts/get-changed-workspaces.mts. A new get-changed-packages CI job fetches the merge base once and computes which packages changed; test-18/20/22, validate-changelog, and build all consume its outputs rather than computing independently. The build job no longer calls the GitHub Compare API itself — it reads the merge base from get-changed-packages and only needs to unshallow its own checkout.
CI-only PRs (e.g. changes only to workflow files) would previously fall
back to running tests and changelog validation for all packages. This was
meant to ensure required status checks were not skipped, but the only
required check is at the workflow level ("all jobs pass"), not at the
individual job level. An empty matrix safely skips those jobs without
blocking merges.
GitHub Actions errors with "Matrix vector does not contain any values" when a matrix input resolves to an empty array. Add an `if` condition to `validate-changelog`, `test-18`, `test-20`, and `test-22` so they are skipped entirely when `package-names` is `[]`.
This job builds the full wallet-cli dependency subtree, so it can't use the package matrix. Instead, skip it entirely when a merge base is available and `@metamask/wallet-cli` is not in the changed packages list.
If any changed file lives outside all package directories, rebuild and test everything. Root-level configs, workflow files, and scripts can all affect every package, so a full run is the safe default. A set of known-safe root files (yarn.lock, README.md, .gitignore, etc.) are excluded from this check since they don't affect package builds or tests.
getAllWorkspaces was using `yarn workspaces list` without `--no-private`, so private packages (e.g. wallet-framework-docs) were included in the test matrix. The prepare job always used `--no-private`, so these were never tested before and have no working test scripts.
Extract a `checkRootChange` helper and an optional `changedFiles` parameter in `computeChangedWorkspaces` to avoid a double `git diff` call. Rewrite `get-changed-workspaces.mts` with Yargs and change its output to a single JSON object (`names`, `locations`, `hasRootChange`) so one script invocation covers all three fields. In CI, move `lint:eslint` out of the matrix into a dedicated `lint-eslint` job. When a PR only touches packages, pass their paths directly to `yarn lint:eslint`; when root files change, fall back to a full run.
Avoids the unquoted subshell (shellcheck SC2046) and keeps the invocation to a single eslint process. With ~93 packages averaging ~30 chars each, the argument list is well under ARG_MAX.
`jq` pretty-prints arrays across multiple lines by default, which causes "Invalid format" errors when writing to GITHUB_OUTPUT. Adding `-c` keeps the JSON on a single line.
- Fix dead `== ''` guard in `test-wallet-cli-e2e` (should be `== '[]'` since `package-names` is always a JSON array string, never empty) - Remove `eslint-suppressions.json` from `IGNORED_ROOT_FILES` so a PR that only adds suppressions still runs ESLint - Pass `getAllWorkspaces()` to `computeChangedWorkspaces` in `generate-partial-build-tsconfig.mts` so that JS-only package changes are correctly attributed and don't trigger a spurious full TS rebuild - Parallelise `package.json` reads in `getWorkspaceDependencies` with `Promise.all`
Removes the duplicate `analyse-code` job from `main.yml` and moves it into `lint-build-test.yml` where it can consume the `scan-paths` output from `get-changed-packages` directly. When only specific packages changed, the scanner receives their locations; when root files changed or there is no merge base, it scans everything. Uses a temporary commit SHA for the scanner action until `paths` support is released as a stable version.
Aligns `scanner-ref` with the action SHA so the internal scanner also picks up the `paths` input implementation.
Paths scoping in CodeQL doesn't reduce runtime — most time is spent compiling .qls query files regardless of the analysis scope. Revert the analyse-code job back to main.yml at @v2.
Saves one sequential job hop (~30s) by running the merge base fetch and changed package detection directly in the 24.x matrix run of prepare, guarded with `if: matrix.node-version == '24.x'`.
Makes it clear at a glance whether CI is running a full or partial check, and which packages are included.
Print each package on its own line with a "- " prefix, and add a blank line after the list to visually separate it from the command output.
Renames eslint-paths to changed-paths and uses it in the build step too. Previously, a root change would set MERGE_BASE and trigger a partial build even though everything should be rebuilt. Now both steps check CHANGED_PATHS == "full" as the authoritative signal.
The partial-run branches already print a blank line after the package list. Add the same blank line to the full-run branches.
Declares the minimum permissions required by the workflow explicitly.
Mrtenz
force-pushed
the
mrtenz/ci-incremental-build
branch
from
July 23, 2026 14:43
664902c to
a508a37
Compare
Mrtenz
force-pushed
the
mrtenz/benchmark-mixed
branch
2 times, most recently
from
July 23, 2026 16:22
e9ef9bc to
fe898a1
Compare
pull Bot
pushed a commit
to Reality2byte/core
that referenced
this pull request
Jul 23, 2026
…9373) ## Explanation Currently, every CI run rebuilds all packages from scratch and runs tests, changelog validation, and ESLint across the entire monorepo, regardless of how many packages actually changed. This PR scopes TypeScript builds, tests, changelog validation, and ESLint to only the packages that changed plus their transitive dependants, by: - Using the GitHub Compare API to find the exact merge base between the PR head and the target branch. - Running `git diff --name-only` against that merge base to find changed files. - Expanding the changed set to transitive dependants (so type-correctness across package boundaries is preserved), and also to transitive dependencies when building (so `ts-bridge` has all referenced `dist/` outputs available). - Falling back to a full run when any file outside a package directory changes (e.g., root config files or workflow files), or when there is no merge base (push to `main`). A new `scripts/get-changed-workspaces.mts` script computes the changed package set and is called from the `prepare` job (24.x matrix run only). Downstream jobs read the outputs from `prepare` directly — the separate `get-changed-packages` job has been removed, saving one sequential job hop. The `changed-paths` output gates both ESLint scope and TypeScript build scope: when it is `"full"`, both run against all packages; when it is a JSON array of paths, each runs only against that subset. ### Benchmark PRs Four benchmark PRs target this branch to verify the expected behaviour: | PR | Change | Expected | |---|---|---| | [MetaMask#9486](MetaMask#9486) | CI-only change (workflow file comment) | Full run — workflow files are not in `IGNORED_ROOT_FILES` | | [MetaMask#9487](MetaMask#9487) | Single package (`@metamask/logging-controller`) | 3 packages (`logging-controller` + 2 transitive dependants) | | [MetaMask#9488](MetaMask#9488) | Three packages (`accounts-`, `gas-fee-`, `network-controller`) | 38 packages (those 3 + transitive dependants) | | [MetaMask#9489](MetaMask#9489) | `README.md` (ignored) + `logging-controller` | 3 packages — `README.md` does not trigger a full run | ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how every PR is validated; incorrect workspace expansion or root-change detection could skip builds or tests, though impact is limited to CI, not shipped product code. > > **Overview** > **CI runs only what the PR touches** instead of rebuilding and testing the whole monorepo on every run. > > The `prepare` job (Node 24.x) resolves the merge base via the GitHub Compare API, runs `get-changed-workspaces.mts`, and exports `package-names`, `changed-paths`, and `merge-base`. Downstream jobs use those outputs: changelog validation and per-Node test matrices iterate only affected workspace names; `wallet-cli` e2e runs only when that package is in the set. > > **ESLint** moves to a dedicated `lint-eslint` job that runs `yarn lint:eslint` on all packages when `changed-paths` is `full`, otherwise only on the listed workspace paths. > > **Build** uses `generate-partial-build-tsconfig.mts` plus `ts-bridge` for a partial TypeScript reference graph (changed packages plus transitive dependants and dependencies) when a merge base exists and there is no root-level change; otherwise it keeps `yarn build`. Root or workflow changes outside ignored files (e.g. not `README.md`) still force a full run; pushes without a merge base fall back to all packages. > > New shared logic lives in `scripts/lib/workspaces.mts` (git diff, dependency graph, root-change detection); `tsconfig.scripts.json` now includes `*.mts` for those scripts. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit b910c31. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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
1 participant
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.
Changes
README.md(inIGNORED_ROOT_FILES) and@metamask/logging-controller. Expected: onlylogging-controller+ its 2 transitive dependants (shield-controller,signature-controller) are tested —README.mdshould not trigger a full run.