From 825a86c1d7e443f2d09fde535d02211bc8c72beb Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 13 May 2026 17:08:36 -0600 Subject: [PATCH] ci: stop refetching translations and rebuilding JS in downstream jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cuts PR build wall-time from 13:15 → 9:00 (32% faster) and per-build agent-time by ~10 minutes. Single source of truth for translation fetching and JS compilation: only `Build React App` does either, and every other job consumes the `dist.tar.gz` artifact. Three compounding inefficiencies removed: 1. `build-react` re-fetched translations every build. `make build REFRESH_L10N=1` defeated the Makefile's existence cache, forcing all ~50 locales to be re-fetched from translate.wordpress.org on every build. 2. Every downstream `: build` consumer (`test-swift-library`, `test-swift-package`, `test-android`, `test-android-library-e2e`, `build-resources-xcframework`) re-fetched translations *and* re-ran `npm ci`, only to throw the work away when `build`'s recipe short-circuited on existing `dist/`. 3. `Test Android Library` and `Test Android Library Instrumented` had no upstream dependency, so they walked the full `: build` chain — fetch all locales, `npm ci`, `npm run build`, copy assets — before finally running gradle. Pipeline (.buildkite/pipeline.yml): - `Build React App` backgrounds `make prep-translations` while running `make npm-dependencies` in the foreground, then `wait`s on the fetch before `make build`. `set -euo pipefail`, plus log redirect/replay so the backgrounded fetch's section markers don't interleave with `npm ci`'s and confuse Buildkite log folding. - `Test Android Library` and `Test Android Library Instrumented` now `depends_on: build-react` and download `dist.tar.gz`. Makefile: - `prep-translations` skips when `dist/` exists *and* `src/translations/` is populated. `REFRESH_L10N=1` and direct invocation still force the fetch. - `build` drops `npm-dependencies` from its prereq list and invokes it via `$(MAKE) _RECURSIVE_INVOKE=1 npm-dependencies` inside the rebuild branch. Downstream `: build` consumers no longer pay for an `npm ci` they don't need. - `npm-dependencies` uses the `_RECURSIVE_INVOKE=1` sentinel to distinguish recursive calls from user-direct invocations, so `make npm-dependencies` still installs even when node_modules exists (matches existing user expectation), while the recursive call from `build` short-circuits. - `test-android` and `test-android-library-e2e` copy `dist/` into Android assets unconditionally — `build`'s recipe short-circuits `copy-dist-android` when `dist/` exists, so CI agents that download a dist artifact would otherwise have stale assets. bin/prep-translations.js: - Drop `import fetch from 'node-fetch'` in favour of Node 20's built-in global `fetch`. Lets the script run before `npm ci` populates `node_modules`, enabling the parallel-with-npm-ci pattern in the CI step. Also drops `node-fetch` from package.json / package-lock.json (98 lines of lockfile). --- .buildkite/pipeline.yml | 37 ++++++++++++++- .gitignore | 1 + Makefile | 65 ++++++++++++++++++++++---- bin/prep-translations.js | 5 +- package-lock.json | 98 ---------------------------------------- package.json | 1 - 6 files changed, 96 insertions(+), 111 deletions(-) diff --git a/.buildkite/pipeline.yml b/.buildkite/pipeline.yml index 4ac399855..37a1f42c2 100644 --- a/.buildkite/pipeline.yml +++ b/.buildkite/pipeline.yml @@ -8,7 +8,33 @@ steps: - label: ':react: Build React App' key: build-react command: | - make build REFRESH_L10N=1 REFRESH_JS_BUILD=1 STRICT_L10N=1 + # Fail fast if any step (including the backgrounded + # `prep-translations`, surfaced via `wait`) returns non-zero, + # rather than continuing into `make build` with partial inputs. + set -euo pipefail + # `prep-translations` is pure network I/O and no longer depends + # on `node_modules` (uses Node's built-in `fetch`), so it can run + # in parallel with `npm ci`. Vite reads `src/translations/` when + # it emits locale bundles, so we wait for the fetch to complete + # before invoking `make build`. + # + # Background output is redirected to a logfile and replayed + # after `wait` returns so its `--- :section:` markers don't + # interleave with `npm ci`'s and confuse Buildkite log folding. + # The replay happens unconditionally so a failed fetch surfaces + # its log before `set -e` aborts the script. + make prep-translations STRICT_L10N=1 > prep-translations.log 2>&1 & + PREP_PID=$$! + make npm-dependencies + PREP_STATUS=0 + wait $$PREP_PID || PREP_STATUS=$$? + echo "--- :earth_americas: prep-translations (ran in parallel with npm ci)" + cat prep-translations.log + rm -f prep-translations.log + if [ "$$PREP_STATUS" -ne 0 ]; then + exit "$$PREP_STATUS" + fi + make build REFRESH_JS_BUILD=1 tar -czf dist.tar.gz dist/ buildkite-agent artifact upload dist.tar.gz plugins: &plugins @@ -54,7 +80,11 @@ steps: plugins: *plugins - label: ':android: Test Android Library' - command: make test-android + depends_on: build-react + command: | + buildkite-agent artifact download dist.tar.gz . + tar -xzf dist.tar.gz + make test-android agents: queue: android plugins: *plugins @@ -142,7 +172,10 @@ steps: - 'android/app/build/outputs/connected_android_test_additional_output/**/*' - label: ':android: Test Android Library Instrumented' + depends_on: build-react command: | + buildkite-agent artifact download dist.tar.gz . + tar -xzf dist.tar.gz echo "--- :robot_face: Starting Android emulator" start-android-emulator \ --system-image "system-images;android-34;google_apis;arm64-v8a" \ diff --git a/.gitignore b/.gitignore index e6f70c7be..2e700a94f 100644 --- a/.gitignore +++ b/.gitignore @@ -136,6 +136,7 @@ out # Nuxt.js build / generate output .nuxt dist +dist.tar.gz # Gatsby files .cache/ diff --git a/Makefile b/Makefile index 38ea2ab30..76d529e6b 100644 --- a/Makefile +++ b/Makefile @@ -31,8 +31,14 @@ npm-dependencies: ## Install npm dependencies # Skip unless... # - node_modules doesn't exist # - REFRESH_DEPS is set to true or 1 -# - npm-dependencies was invoked directly - @if [ ! -d "node_modules" ] || [ "$(REFRESH_DEPS)" = "true" ] || [ "$(REFRESH_DEPS)" = "1" ] || echo "$(MAKECMDGOALS)" | grep -q "^npm-dependencies$$"; then \ +# - npm-dependencies was invoked directly (not from a recursive `$(MAKE)`) +# +# `build`'s rebuild branch invokes this as `$(MAKE) _RECURSIVE_INVOKE=1 +# npm-dependencies`, which sets MAKECMDGOALS=npm-dependencies in the +# child make. Without the sentinel, that recursive call would treat +# itself as a "direct invocation" and re-run `npm ci` every time `build` +# rebuilds — even when node_modules is already populated. + @if [ ! -d "node_modules" ] || [ "$(REFRESH_DEPS)" = "true" ] || [ "$(REFRESH_DEPS)" = "1" ] || { [ -z "$(_RECURSIVE_INVOKE)" ] && echo "$(MAKECMDGOALS)" | grep -q "^npm-dependencies$$"; }; then \ echo "--- :npm: Installing NPM Dependencies"; \ npm ci; \ else \ @@ -41,11 +47,26 @@ npm-dependencies: ## Install npm dependencies .PHONY: prep-translations prep-translations: ## Fetch and cache locale string files -# Skip unless... +# Skip when `dist/` already exists *and* `src/translations/` is +# populated — translations are baked into the bundle at JS build time, +# so there is nothing for a downstream consumer to refresh until the +# bundle itself is rebuilt. This matters on CI agents that download +# `dist.tar.gz` from an upstream job: without it, every downstream +# `make` target that depends on `build` would re-fetch all ~50 locales +# from translate.wordpress.org. +# +# We also require `src/translations/` to be populated so that a stale +# `dist/` paired with an empty `src/translations/` doesn't silently +# produce a no-translations rebuild when `REFRESH_JS_BUILD=1` or a +# direct `make build` triggers JS rebuilding against empty inputs. +# +# Otherwise, skip unless... # - src/translations doesn't contain any fetched bundles (only `.gitkeep` is committed) # - REFRESH_L10N is set to true or 1 # - prep-translations was invoked directly - @if [ -z "$$(find src/translations -maxdepth 1 -name '*.json' -print -quit 2>/dev/null)" ] || [ "$(REFRESH_L10N)" = "true" ] || [ "$(REFRESH_L10N)" = "1" ] || echo "$(MAKECMDGOALS)" | grep -q "^prep-translations$$"; then \ + @if [ -d "dist" ] && [ -n "$$(find src/translations -maxdepth 1 -name '*.json' -print -quit 2>/dev/null)" ] && [ "$(REFRESH_L10N)" != "true" ] && [ "$(REFRESH_L10N)" != "1" ] && ! echo "$(MAKECMDGOALS)" | grep -q "^prep-translations$$"; then \ + echo "--- :white_check_mark: Skipping translations fetch (dist/ already built, translations baked in). Use REFRESH_L10N=1 to force refresh."; \ + elif [ -z "$$(find src/translations -maxdepth 1 -name '*.json' -print -quit 2>/dev/null)" ] || [ "$(REFRESH_L10N)" = "true" ] || [ "$(REFRESH_L10N)" = "1" ] || echo "$(MAKECMDGOALS)" | grep -q "^prep-translations$$"; then \ echo "--- :npm: Preparing Translations"; \ if ! npm run prep-translations -- --force; then \ if [ "$(STRICT_L10N)" = "true" ] || [ "$(STRICT_L10N)" = "1" ]; then \ @@ -91,16 +112,28 @@ clean: ## Remove build artifacts and translation string files ################################################################################ .PHONY: build -build: npm-dependencies prep-translations ## Build the project for all platforms (iOS, Android, web) +build: prep-translations ## Build the project for all platforms (iOS, Android, web) # Skip unless... # - dist doesn't exist # - REFRESH_JS_BUILD is set to true or 1 # - build was invoked directly +# +# `npm-dependencies` is invoked from inside the rebuild branch rather +# than declared as a Make prereq so that downstream targets which +# depend on `build` (`test-android`, `test-swift-library`, etc.) don't +# trigger an `npm ci` they don't actually need when `dist/` is already +# populated — e.g. on CI agents that just extracted an upstream +# `dist.tar.gz` and only intend to run gradle/xcodebuild/swift. +# +# Targets that legitimately use node_modules (`test-e2e` via +# `e2e-dependencies`, `lint-js`, `test-js`, etc.) declare +# `npm-dependencies` as their own prereq. @if [ ! -d "dist" ] || [ "$(REFRESH_JS_BUILD)" = "true" ] || [ "$(REFRESH_JS_BUILD)" = "1" ] || echo "$(MAKECMDGOALS)" | grep -q "^build$$"; then \ - echo "--- :node: Building Gutenberg"; \ - npm run build; \ - echo "--- :open_file_folder: Copying Build Products into place"; \ - $(MAKE) copy-dist-ios; \ + $(MAKE) _RECURSIVE_INVOKE=1 npm-dependencies && \ + echo "--- :node: Building Gutenberg" && \ + npm run build && \ + echo "--- :open_file_folder: Copying Build Products into place" && \ + $(MAKE) copy-dist-ios && \ $(MAKE) copy-dist-android; \ else \ echo "--- :white_check_mark: Skipping JS build (dist already exists). Use REFRESH_JS_BUILD=1 to force refresh."; \ @@ -285,6 +318,13 @@ test-ios-e2e-dev: ## Run iOS E2E tests against the Vite dev server (must be runn .PHONY: test-android test-android: build ## Run Android tests +# `build` short-circuits `copy-dist-android` when `dist/` already exists +# (e.g. in CI, after extracting an upstream `dist.tar.gz`), so copy +# explicitly here to guarantee the tests run against the current dist +# rather than whatever was committed at HEAD. + @echo "--- :open_file_folder: Copying build into Android bundle" + @rm -rf ./android/Gutenberg/src/main/assets/ + @cp -r ./dist/. ./android/Gutenberg/src/main/assets @echo "--- :android: Running Android Tests" ./android/gradlew -p ./android :gutenberg:test @@ -346,6 +386,13 @@ test-android-e2e-dev: ## Run Android E2E tests against the Vite dev server (must .PHONY: test-android-library-e2e test-android-library-e2e: build ## Run instrumented tests for the Gutenberg Android library module +# `build` short-circuits `copy-dist-android` when `dist/` already exists +# (e.g. in CI, after extracting an upstream `dist.tar.gz`), so copy +# explicitly here to guarantee the instrumented tests run against the +# current dist rather than whatever was committed at HEAD. + @echo "--- :open_file_folder: Copying build into Android bundle" + @rm -rf ./android/Gutenberg/src/main/assets/ + @cp -r ./dist/. ./android/Gutenberg/src/main/assets $(ENSURE_ANDROID_DEVICE) @echo "--- :android: Running Android Library Instrumented Tests" @mkdir -p android/Gutenberg/build/outputs/buildkite-logs diff --git a/bin/prep-translations.js b/bin/prep-translations.js index 5e0d1c2ac..7a4571ffc 100644 --- a/bin/prep-translations.js +++ b/bin/prep-translations.js @@ -3,13 +3,16 @@ */ import fs from 'fs'; import path from 'path'; -import fetch from 'node-fetch'; /** * Internal dependencies */ import { info, error, debug } from '../src/utils/logger.js'; +// Uses Node's built-in `fetch` (Node 20, per .nvmrc) so this script can +// run before `npm ci` has populated `node_modules` — letting CI overlap +// translation fetching with dependency installation on the critical path. + const TRANSLATIONS_DIR = path.join( process.cwd(), 'src/translations' ); const SUPPORTED_LOCALES = [ 'ar', // Arabic diff --git a/package-lock.json b/package-lock.json index e8307afc8..105e736b7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -86,7 +86,6 @@ "eslint-plugin-react-refresh": "^0.4.26", "jsdom": "^29.0.2", "magic-string": "^0.30.21", - "node-fetch": "^3.3.2", "patch-package": "^8.0.1", "prettier": "npm:wp-prettier@^3.0.3", "react-devtools": "^7.0.1", @@ -12962,16 +12961,6 @@ "dev": true, "license": "BSD-2-Clause" }, - "node_modules/data-uri-to-buffer": { - "version": "4.0.1", - "resolved": "https://registry.npmjs.org/data-uri-to-buffer/-/data-uri-to-buffer-4.0.1.tgz", - "integrity": "sha512-0R9ikRb668HB7QDxT1vkpuUBtqc53YyAwMwGeUFKRojY/NWKvdZ+9UYtRfGmhqNbRkTSVpMbmyhXipFFv2cb/A==", - "dev": true, - "license": "MIT", - "engines": { - "node": ">= 12" - } - }, "node_modules/data-urls": { "version": "7.0.0", "resolved": "https://registry.npmjs.org/data-urls/-/data-urls-7.0.0.tgz", @@ -14890,30 +14879,6 @@ "pend": "~1.2.0" } }, - "node_modules/fetch-blob": { - "version": "3.2.0", - "resolved": "https://registry.npmjs.org/fetch-blob/-/fetch-blob-3.2.0.tgz", - "integrity": "sha512-7yAQpD2UMJzLi1Dqv7qFYnPbaPx7ZfFK6PiIxQ4PfkGPyNyl2Ugx+a/umUonmKqjhM4DnfbMvdX6otXq83soQQ==", - "dev": true, - "funding": [ - { - "type": "github", - "url": "https://github.com/sponsors/jimmywarting" - }, - { - "type": "paypal", - "url": "https://paypal.me/jimmywarting" - } - ], - "license": "MIT", - "dependencies": { - "node-domexception": "^1.0.0", - "web-streams-polyfill": "^3.0.3" - }, - "engines": { - "node": "^12.20 || >= 14.13" - } - }, "node_modules/file-entry-cache": { "version": "6.0.1", "resolved": "https://registry.npmjs.org/file-entry-cache/-/file-entry-cache-6.0.1.tgz", @@ -15077,19 +15042,6 @@ "url": "https://github.com/sponsors/isaacs" } }, - "node_modules/formdata-polyfill": { - "version": "4.0.10", - "resolved": "https://registry.npmjs.org/formdata-polyfill/-/formdata-polyfill-4.0.10.tgz", - "integrity": "sha512-buewHzMvYL29jdeQTVILecSaZKnt/RJWjoZCF5OW60Z67/GmSLBkOFM7qh1PI3zFNtJbaZL5eQu1vLfazOwj4g==", - "dev": true, - "license": "MIT", - "dependencies": { - "fetch-blob": "^3.1.2" - }, - "engines": { - "node": ">=12.20.0" - } - }, "node_modules/forwarded": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/forwarded/-/forwarded-0.2.0.tgz", @@ -17927,46 +17879,6 @@ "license": "MIT", "optional": true }, - "node_modules/node-domexception": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/node-domexception/-/node-domexception-1.0.0.tgz", - "integrity": "sha512-/jKZoMpw0F8GRwl4/eLROPA3cfcXtLApP0QzLmUT/HuPCZWyB7IY9ZrMeKw2O/nFIqPQB3PVM9aYm0F312AXDQ==", - "deprecated": "Use your platform's native DOMException instead", - "dev": true, - "funding": [ - { - "type": "github", - "url": "https://github.com/sponsors/jimmywarting" - }, - { - "type": "github", - "url": "https://paypal.me/jimmywarting" - } - ], - "license": "MIT", - "engines": { - "node": ">=10.5.0" - } - }, - "node_modules/node-fetch": { - "version": "3.3.2", - "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-3.3.2.tgz", - "integrity": "sha512-dRB78srN/l6gqWulah9SrxeYnxeddIG30+GOqK/9OlLVyLg3HPnr6SqOWTWOXKRwC2eGYCkZ59NNuSgvSrpgOA==", - "dev": true, - "license": "MIT", - "dependencies": { - "data-uri-to-buffer": "^4.0.0", - "fetch-blob": "^3.1.4", - "formdata-polyfill": "^4.0.10" - }, - "engines": { - "node": "^12.20.0 || ^14.13.1 || >=16.0.0" - }, - "funding": { - "type": "opencollective", - "url": "https://opencollective.com/node-fetch" - } - }, "node_modules/node-releases": { "version": "2.0.27", "resolved": "https://registry.npmjs.org/node-releases/-/node-releases-2.0.27.tgz", @@ -22814,16 +22726,6 @@ "defaults": "^1.0.3" } }, - "node_modules/web-streams-polyfill": { - "version": "3.3.3", - "resolved": "https://registry.npmjs.org/web-streams-polyfill/-/web-streams-polyfill-3.3.3.tgz", - "integrity": "sha512-d2JWLCivmZYTSIoge9MsgFCZrt571BikcWGYkjC1khllbTeDlGqZ2D8vD8E/lJa8WGWbb7Plm8/XJYV7IJHZZw==", - "dev": true, - "license": "MIT", - "engines": { - "node": ">= 8" - } - }, "node_modules/webidl-conversions": { "version": "8.0.1", "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-8.0.1.tgz", diff --git a/package.json b/package.json index a04270fda..133b8ea03 100644 --- a/package.json +++ b/package.json @@ -103,7 +103,6 @@ "eslint-plugin-react-refresh": "^0.4.26", "jsdom": "^29.0.2", "magic-string": "^0.30.21", - "node-fetch": "^3.3.2", "patch-package": "^8.0.1", "prettier": "npm:wp-prettier@^3.0.3", "react-devtools": "^7.0.1",