Skip to content

Commit 8adc99c

Browse files
committed
ci: harden parallel-fetch error surfacing and tighten skip clauses
Follow-ups to the parallel-fetch and npm-skip changes: **Build React App step** - `set -euo pipefail` so any step (including the backgrounded fetch surfaced via `wait`) aborts cleanly instead of continuing into `make build` with partial inputs. - Redirect `prep-translations` output to a logfile and replay it after `wait` returns, so its `--- :section:` markers don't interleave with `npm ci`'s and confuse Buildkite log folding. Replay happens unconditionally so a failed fetch surfaces its log before `set -e` aborts. **Makefile** - `npm-dependencies` switches from "drop direct-invocation-force" to an explicit `_RECURSIVE_INVOKE=1` sentinel passed by `build`'s recipe. Preserves the user-facing "make npm-dependencies installs even if node_modules exists" behaviour while still letting the recursive call short-circuit. - `prep-translations` skip clause now requires both `dist/` *and* `src/translations/` populated. A stale `dist/` paired with an empty `src/translations/` would otherwise silently produce a no-translations rebuild on `REFRESH_JS_BUILD=1`. **Housekeeping** - `.gitignore`: add `dist.tar.gz` (created locally when testing the artifact pack/upload step). - `bin/prep-translations.js`: comment now references `.nvmrc` for the Node version it relies on.
1 parent 0f29d64 commit 8adc99c

4 files changed

Lines changed: 44 additions & 20 deletions

File tree

.buildkite/pipeline.yml

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,32 @@ steps:
88
- label: ':react: Build React App'
99
key: build-react
1010
command: |
11+
# Fail fast if any step (including the backgrounded
12+
# `prep-translations`, surfaced via `wait`) returns non-zero,
13+
# rather than continuing into `make build` with partial inputs.
14+
set -euo pipefail
1115
# `prep-translations` is pure network I/O and no longer depends
1216
# on `node_modules` (uses Node's built-in `fetch`), so it can run
1317
# in parallel with `npm ci`. Vite reads `src/translations/` when
1418
# it emits locale bundles, so we wait for the fetch to complete
1519
# before invoking `make build`.
16-
make prep-translations STRICT_L10N=1 &
20+
#
21+
# Background output is redirected to a logfile and replayed
22+
# after `wait` returns so its `--- :section:` markers don't
23+
# interleave with `npm ci`'s and confuse Buildkite log folding.
24+
# The replay happens unconditionally so a failed fetch surfaces
25+
# its log before `set -e` aborts the script.
26+
make prep-translations STRICT_L10N=1 > prep-translations.log 2>&1 &
1727
PREP_PID=$$!
1828
make npm-dependencies
19-
wait $$PREP_PID
29+
PREP_STATUS=0
30+
wait $$PREP_PID || PREP_STATUS=$$?
31+
echo "--- :earth_americas: prep-translations (ran in parallel with npm ci)"
32+
cat prep-translations.log
33+
rm -f prep-translations.log
34+
if [ "$$PREP_STATUS" -ne 0 ]; then
35+
exit "$$PREP_STATUS"
36+
fi
2037
make build REFRESH_JS_BUILD=1
2138
tar -czf dist.tar.gz dist/
2239
buildkite-agent artifact upload dist.tar.gz

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ out
136136
# Nuxt.js build / generate output
137137
.nuxt
138138
dist
139+
dist.tar.gz
139140

140141
# Gatsby files
141142
.cache/

Makefile

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,14 @@ npm-dependencies: ## Install npm dependencies
3131
# Skip unless...
3232
# - node_modules doesn't exist
3333
# - REFRESH_DEPS is set to true or 1
34+
# - npm-dependencies was invoked directly (not from a recursive `$(MAKE)`)
3435
#
35-
# Direct invocation no longer force-installs: `build` invokes this via
36-
# `$(MAKE) npm-dependencies` from inside its rebuild branch, which sets
37-
# MAKECMDGOALS=npm-dependencies in the child make. Without this change,
38-
# that recursive call would re-run `npm ci` even when node_modules is
39-
# already populated. Users who want to force a reinstall can pass
40-
# REFRESH_DEPS=1.
41-
@if [ ! -d "node_modules" ] || [ "$(REFRESH_DEPS)" = "true" ] || [ "$(REFRESH_DEPS)" = "1" ]; then \
36+
# `build`'s rebuild branch invokes this as `$(MAKE) _RECURSIVE_INVOKE=1
37+
# npm-dependencies`, which sets MAKECMDGOALS=npm-dependencies in the
38+
# child make. Without the sentinel, that recursive call would treat
39+
# itself as a "direct invocation" and re-run `npm ci` every time `build`
40+
# rebuilds — even when node_modules is already populated.
41+
@if [ ! -d "node_modules" ] || [ "$(REFRESH_DEPS)" = "true" ] || [ "$(REFRESH_DEPS)" = "1" ] || { [ -z "$(_RECURSIVE_INVOKE)" ] && echo "$(MAKECMDGOALS)" | grep -q "^npm-dependencies$$"; }; then \
4242
echo "--- :npm: Installing NPM Dependencies"; \
4343
npm ci; \
4444
else \
@@ -47,18 +47,24 @@ npm-dependencies: ## Install npm dependencies
4747

4848
.PHONY: prep-translations
4949
prep-translations: ## Fetch and cache locale string files
50-
# Skip when `dist/` already exists — translations are baked into the
51-
# bundle at JS build time, so there is nothing for a downstream consumer
52-
# to refresh until the bundle itself is rebuilt. This matters on CI
53-
# agents that download `dist.tar.gz` from an upstream job: without it,
54-
# every downstream `make` target that depends on `build` would re-fetch
55-
# all ~50 locales from translate.wordpress.org.
50+
# Skip when `dist/` already exists *and* `src/translations/` is
51+
# populated — translations are baked into the bundle at JS build time,
52+
# so there is nothing for a downstream consumer to refresh until the
53+
# bundle itself is rebuilt. This matters on CI agents that download
54+
# `dist.tar.gz` from an upstream job: without it, every downstream
55+
# `make` target that depends on `build` would re-fetch all ~50 locales
56+
# from translate.wordpress.org.
57+
#
58+
# We also require `src/translations/` to be populated so that a stale
59+
# `dist/` paired with an empty `src/translations/` doesn't silently
60+
# produce a no-translations rebuild when `REFRESH_JS_BUILD=1` or a
61+
# direct `make build` triggers JS rebuilding against empty inputs.
5662
#
5763
# Otherwise, skip unless...
5864
# - src/translations doesn't contain any fetched bundles (only `.gitkeep` is committed)
5965
# - REFRESH_L10N is set to true or 1
6066
# - prep-translations was invoked directly
61-
@if [ -d "dist" ] && [ "$(REFRESH_L10N)" != "true" ] && [ "$(REFRESH_L10N)" != "1" ] && ! echo "$(MAKECMDGOALS)" | grep -q "^prep-translations$$"; then \
67+
@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 \
6268
echo "--- :white_check_mark: Skipping translations fetch (dist/ already built, translations baked in). Use REFRESH_L10N=1 to force refresh."; \
6369
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 \
6470
echo "--- :npm: Preparing Translations"; \
@@ -123,7 +129,7 @@ build: prep-translations ## Build the project for all platforms (iOS, Android, w
123129
# `e2e-dependencies`, `lint-js`, `test-js`, etc.) declare
124130
# `npm-dependencies` as their own prereq.
125131
@if [ ! -d "dist" ] || [ "$(REFRESH_JS_BUILD)" = "true" ] || [ "$(REFRESH_JS_BUILD)" = "1" ] || echo "$(MAKECMDGOALS)" | grep -q "^build$$"; then \
126-
$(MAKE) npm-dependencies && \
132+
$(MAKE) _RECURSIVE_INVOKE=1 npm-dependencies && \
127133
echo "--- :node: Building Gutenberg" && \
128134
npm run build && \
129135
echo "--- :open_file_folder: Copying Build Products into place" && \

bin/prep-translations.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ import path from 'path';
99
*/
1010
import { info, error, debug } from '../src/utils/logger.js';
1111

12-
// Uses Node's built-in `fetch` (Node 18+) so this script can run before
13-
// `npm ci` has populated `node_modules` — letting CI overlap translation
14-
// fetching with dependency installation on the critical path.
12+
// Uses Node's built-in `fetch` (Node 20, per .nvmrc) so this script can
13+
// run before `npm ci` has populated `node_modules` — letting CI overlap
14+
// translation fetching with dependency installation on the critical path.
1515

1616
const TRANSLATIONS_DIR = path.join( process.cwd(), 'src/translations' );
1717
const SUPPORTED_LOCALES = [

0 commit comments

Comments
 (0)