Skip to content

Commit 14e7ae9

Browse files
sensei-hackersensei-hacker
authored
Merge pull request #11835 from sensei-hacker/fix-size-baseline-commit-matching
Fix size-diff baseline comparison: compare against the PR's merge-base commit, not the branch tip
2 parents 463e224 + 6a0fb56 commit 14e7ae9

6 files changed

Lines changed: 433 additions & 68 deletions

File tree

‎.github/scripts/fetch-size-baseline.sh‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
#!/bin/bash
2+
#
3+
# Fetch the size baseline for a PR's TRUE base commit — the merge-base of
4+
# the PR head and base ref — rather than the base branch's latest tip, so
5+
# the size-diff delta doesn't include unrelated changes merged to the base
6+
# after the PR forked (see the +9,788 B stale-RAM regression on PR #11785).
7+
#
8+
# Strategy, in order:
9+
# 1. Exact: size-baseline-<merge-base-sha> in the companion builds repo.
10+
# 2. Nearest ancestor: walk first-parents of the merge-base (older commits
11+
# on the base branch) up to MAX_WALK steps, using the first per-commit
12+
# baseline tag found. Nightly baselines exist only for commits that were
13+
# actually pushed to the branch, so the walk finds the closest nightly
14+
# build at-or-before the fork point.
15+
# 3. Otherwise report found=false (caller emits the graceful
16+
# "no size baseline available" comment path).
17+
#
18+
# The base branch's LATEST-tip baseline is deliberately NOT used as a
19+
# fallback: comparing against it is exactly the stale-delta bug this fixes.
20+
#
21+
# Usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>
22+
# <main-repo> the firmware repo, e.g. iNavFlight/inav
23+
# <builds-repo> companion repo holding the baselines, e.g. iNavFlight/pr-test-builds
24+
# <base-ref> PR base branch, e.g. maintenance-10.x (validated)
25+
# <merge-base-sha> full 40-hex SHA of the PR's merge-base commit (validated)
26+
# <out-dir> directory to write size-report.json into
27+
#
28+
# Env: GH_TOKEN with read access to both repos (secrets.PR_BUILDS_TOKEN in
29+
# the pr-comment job has it; the companion repo is private).
30+
#
31+
# Prints key=value lines to stdout for the workflow to append to
32+
# $GITHUB_OUTPUT: found, baseline_commit (full SHA), baseline_commit_short,
33+
# baseline_exact (true when the merge-base itself had a stored baseline).
34+
35+
set -euo pipefail
36+
37+
MAIN_REPO=${1:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>}
38+
BUILDS_REPO=${2:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>}
39+
BASE_REF=${3:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>}
40+
MERGE_BASE_SHA=${4:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>}
41+
OUT_DIR=${5:?usage: fetch-size-baseline.sh <main-repo> <builds-repo> <base-ref> <merge-base-sha> <out-dir>}
42+
43+
MAX_WALK=30
44+
45+
if ! [[ "$BASE_REF" =~ ^[A-Za-z0-9._/-]{1,100}$ ]]; then
46+
echo "::error::Invalid base ref: $BASE_REF" >&2
47+
exit 1
48+
fi
49+
if ! [[ "$MERGE_BASE_SHA" =~ ^[0-9a-f]{40}$ ]]; then
50+
echo "::error::Invalid merge-base SHA: $MERGE_BASE_SHA" >&2
51+
exit 1
52+
fi
53+
54+
mkdir -p "$OUT_DIR"
55+
56+
# Set of per-commit baseline SHAs currently stored in the builds repo.
57+
# One paginated call; tags are built by publish-size-baseline.sh and match
58+
# the strict 40-hex pattern by construction.
59+
list_baseline_shas() {
60+
gh api "repos/${BUILDS_REPO}/releases?per_page=100" --paginate \
61+
--jq '.[].tag_name | select(test("^size-baseline-[0-9a-f]{40}$")) | sub("^size-baseline-"; "")'
62+
}
63+
64+
# publish-size-baseline replaces the asset in place (--clobber), which still
65+
# briefly deletes-then-uploads under the hood; a few short retries absorb
66+
# that window instead of misreporting "no baseline available".
67+
download_baseline() { # $1 = 40-hex sha
68+
local tag="size-baseline-${1}"
69+
for attempt in 1 2 3; do
70+
if gh release download "$tag" --repo "$BUILDS_REPO" \
71+
--pattern size-report.json --dir "$OUT_DIR" 2>/dev/null; then
72+
return 0
73+
fi
74+
sleep 3
75+
done
76+
return 1
77+
}
78+
79+
emit_found() { # $1 = 40-hex sha, $2 = true|false (exact)
80+
echo "found=true"
81+
echo "baseline_commit=${1}"
82+
echo "baseline_commit_short=${1:0:7}"
83+
echo "baseline_exact=${2}"
84+
}
85+
86+
BASELINE_SHAS=$(list_baseline_shas) || true
87+
88+
# Exact merge-base first, then nearest ancestors along the base branch's
89+
# first-parent chain. The loop STARTS at the merge-base itself so a
90+
# transient download failure on the exact commit is retried here instead
91+
# of silently degrading to a nearest-ancestor baseline.
92+
sha="$MERGE_BASE_SHA"
93+
first=1
94+
for _ in $(seq 1 $((MAX_WALK + 1))); do
95+
if grep -qx "$sha" <<< "$BASELINE_SHAS" && download_baseline "$sha"; then
96+
if [ "$first" = 1 ]; then
97+
emit_found "$sha" true
98+
else
99+
emit_found "$sha" false
100+
fi
101+
exit 0
102+
fi
103+
first=0
104+
parent=$(gh api "repos/${MAIN_REPO}/commits/${sha}" --jq '.parents[0].sha // empty' 2>/dev/null || true)
105+
if [ -z "$parent" ] || ! [[ "$parent" =~ ^[0-9a-f]{40}$ ]]; then
106+
break
107+
fi
108+
sha="$parent"
109+
done
110+
111+
echo "found=false"

‎.github/scripts/publish-size-baseline.sh‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
#!/bin/bash
2+
#
3+
# Publish the nightly size report as release assets in the companion
4+
# pr-test-builds repo, keyed by BRANCH (latest-tip pointer, kept for
5+
# backward compatibility) AND by COMMIT SHA (primary — lets PR size-diff
6+
# comparisons use the PR's exact base commit instead of the branch tip),
7+
# then prune old per-commit baselines so the companion repo doesn't grow
8+
# unbounded.
9+
#
10+
# Usage: publish-size-baseline.sh <builds-repo> <branch> <commit-sha> <report-json> [--dry-run]
11+
# <builds-repo> companion repo, e.g. iNavFlight/pr-test-builds
12+
# <branch> branch the nightly was pushed to (e.g. maintenance-10.x)
13+
# <commit-sha> full 40-hex SHA of the pushed commit (workflow_run.head_sha)
14+
# <report-json> merged size-report.json artifact
15+
# --dry-run print what would be published/pruned without touching GitHub
16+
#
17+
# Env: GH_TOKEN with write access to <builds-repo> (Contents: write).
18+
#
19+
# Per-commit baseline releases carry a machine-readable `branch: <name>`
20+
# first line in their notes so pruning can group them per branch. Tags are
21+
# `size-baseline-<40-hex-sha>`; branch-tip tags are `size-baseline-<branch>`.
22+
#
23+
# Pruning policy: keep the newest KEEP_PER_BRANCH per-commit baselines per
24+
# branch, plus a GLOBAL_CAP safety net for orphaned baselines (deleted
25+
# branches, notes parse failures) so the repo can never grow unbounded.
26+
27+
set -euo pipefail
28+
29+
BUILDS_REPO=${1:?usage: publish-size-baseline.sh <builds-repo> <branch> <commit-sha> <report-json> [--dry-run]}
30+
BRANCH=${2:?usage: publish-size-baseline.sh <builds-repo> <branch> <commit-sha> <report-json> [--dry-run]}
31+
COMMIT_SHA=${3:?usage: publish-size-baseline.sh <builds-repo> <branch> <commit-sha> <report-json> [--dry-run]}
32+
REPORT_JSON=${4:?usage: publish-size-baseline.sh <builds-repo> <branch> <commit-sha> <report-json> [--dry-run]}
33+
DRY_RUN=${5:-}
34+
35+
KEEP_PER_BRANCH=50
36+
GLOBAL_CAP=300
37+
38+
# --- Validate inputs: never trust artifact content or context values before
39+
# they are used to build shell commands or release tags.
40+
if ! [[ "$BRANCH" =~ ^[A-Za-z0-9._/-]{1,100}$ ]]; then
41+
echo "::error::Invalid branch name in artifact: $BRANCH" >&2
42+
exit 1
43+
fi
44+
if ! [[ "$COMMIT_SHA" =~ ^[0-9a-f]{40}$ ]]; then
45+
echo "::error::Invalid commit SHA: $COMMIT_SHA" >&2
46+
exit 1
47+
fi
48+
if [ ! -f "$REPORT_JSON" ]; then
49+
echo "::error::Size report not found: $REPORT_JSON" >&2
50+
exit 1
51+
fi
52+
53+
# publish one baseline release: create once, then only ever replace the
54+
# asset in place (--clobber) so the release/tag stays continuously
55+
# resolvable for concurrent PR comparisons.
56+
publish_asset() {
57+
local tag="$1" title="$2" notes="$3"
58+
if gh release view "$tag" --repo "$BUILDS_REPO" >/dev/null 2>&1; then
59+
gh release upload "$tag" "$REPORT_JSON" --repo "$BUILDS_REPO" --clobber
60+
else
61+
gh release create "$tag" "$REPORT_JSON" --repo "$BUILDS_REPO" --prerelease \
62+
--title "$title" --notes "$notes"
63+
fi
64+
}
65+
66+
BRANCH_TAG="size-baseline-${BRANCH}"
67+
COMMIT_TAG="size-baseline-${COMMIT_SHA}"
68+
69+
if [ "$DRY_RUN" = "--dry-run" ]; then
70+
echo "[dry-run] would publish ${BRANCH_TAG} and ${COMMIT_TAG} in ${BUILDS_REPO}"
71+
else
72+
publish_asset "$BRANCH_TAG" "Size baseline: ${BRANCH}" \
73+
"Latest per-target flash/RAM size report for ${BRANCH}. Auto-updated on every push. Not for human consumption."
74+
publish_asset "$COMMIT_TAG" "Size baseline: ${COMMIT_SHA}" \
75+
"branch: ${BRANCH}
76+
Per-commit per-target flash/RAM size report for ${COMMIT_SHA}. Consumed by PR size-diff comparisons; pruned to the newest ${KEEP_PER_BRANCH} per branch."
77+
fi
78+
79+
# ---------------------------------------------------------------------------
80+
# Prune per-commit baselines
81+
# ---------------------------------------------------------------------------
82+
# Emit <created_at>\t<tag>\t<branch> for every per-commit baseline release,
83+
# newest first (GitHub release listing is newest-first). Branch comes from
84+
# the notes' `branch: <name>` FIRST LINE; the (?m) flag is required — the
85+
# notes are multi-line, and without it ^/$ anchor to the whole string so
86+
# the capture never matches and every baseline collapses into the '?'
87+
# bucket, which would make pruning treat all branches as one.
88+
list_per_commit_baselines() {
89+
gh api "repos/${BUILDS_REPO}/releases?per_page=100" --paginate \
90+
--jq '.[] | select(.tag_name | test("^size-baseline-[0-9a-f]{40}$")) |
91+
[.created_at, .tag_name,
92+
((.body // "") | capture("(?m)^branch: (?<b>[A-Za-z0-9._/-]+)$") | .b // "?")] | @tsv'
93+
}
94+
95+
prune() {
96+
local tmp
97+
tmp=$(mktemp)
98+
trap 'rm -f "$tmp"' RETURN
99+
100+
# Pass 1 (NR==FNR): record every tag in `all`, and the per-branch
101+
# keep-set — newest KEEP_PER_BRANCH tags per branch — in `keepTag`.
102+
# Pass 2: apply the global cap to the kept set in input order.
103+
# END: delete every tag in `all` that did not survive to keepFinal.
104+
# (Explicit `all` set so pruning never depends on awk's create-on-
105+
# reference semantics of reading keepTag[$2] in pass 2.)
106+
list_per_commit_baselines | sort -r > "$tmp"
107+
108+
awk -F '\t' -v keep="$KEEP_PER_BRANCH" -v cap="$GLOBAL_CAP" '
109+
NR == FNR {
110+
all[$2] = 1
111+
if (count[$3] < keep) { count[$3]++; keepTag[$2] = 1 }
112+
next
113+
}
114+
{
115+
if (keepTag[$2]) {
116+
if (kept < cap) { kept++; keepFinal[$2] = 1 }
117+
}
118+
}
119+
END {
120+
for (t in all) if (!keepFinal[t]) print t
121+
}
122+
' "$tmp" "$tmp" | while read -r tag; do
123+
if [ "$DRY_RUN" = "--dry-run" ]; then
124+
echo "[dry-run] would prune ${tag}"
125+
else
126+
gh release delete "$tag" --repo "$BUILDS_REPO" --yes --cleanup-tag \
127+
|| echo "::warning::failed to prune ${tag}" >&2
128+
fi
129+
done
130+
}
131+
132+
# Pruning is housekeeping: a failure here must not fail the publish (the
133+
# baseline itself already landed above), or the nightly would look broken
134+
# for a cosmetic reason. Warn loudly instead.
135+
prune || echo "::warning::per-commit baseline pruning failed (see stderr)" >&2

‎.github/scripts/size-diff-comment.js‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,15 +60,31 @@ function diffSizeReports(prReport, baselineReport) {
6060
}
6161

6262
// docLink: string URL to link, or null/undefined to omit the doc-link line.
63-
function renderComment({ prReport, baselineReport, shortSha, docLink, marker }) {
63+
// baselineCommit: short SHA of the baseline commit the delta was computed
64+
// against, or null/undefined to fall back to the generic "base branch"
65+
// wording. baselineIsNearest: true when the exact base commit had no
66+
// stored baseline and a nearest-ancestor baseline was used instead.
67+
function renderComment({ prReport, baselineReport, shortSha, baselineCommit, baselineIsNearest, docLink, marker }) {
6468
const rows = diffSizeReports(prReport, baselineReport);
65-
const lines = [marker, '**RAM / Flash usage vs. base branch** — commit `' + shortSha + '`', ''];
69+
// Only name the baseline commit when there is actually a baseline to
70+
// compare against (the workflow only sets baselineCommit in that case,
71+
// but the renderer must not emit a contradictory header otherwise).
72+
const vs = (baselineReport && baselineCommit)
73+
? `vs. base commit \`${baselineCommit}\`` : 'vs. base branch';
74+
const lines = [marker, `**RAM / Flash usage ${vs}** — commit \`${shortSha}\``, ''];
6675

6776
if (!baselineReport) {
6877
lines.push(
69-
'> No size baseline is available yet for this PR\'s base branch ' +
70-
'(first run after this feature shipped, or a new branch). ' +
71-
'This comment will show deltas once a baseline exists.',
78+
'> No size baseline is available yet for this PR\'s base commit ' +
79+
'(no per-commit baseline has been published for it). This comment ' +
80+
'will show deltas once one exists — rebasing the PR refreshes its ' +
81+
'base commit.',
82+
''
83+
);
84+
} else if (baselineIsNearest) {
85+
lines.push(
86+
'> Using the nearest available size baseline — the PR\'s exact base ' +
87+
'commit has no stored baseline yet.',
7288
''
7389
);
7490
}

‎.github/scripts/size-diff-comment.test.js‎

Lines changed: 76 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,12 @@ test('diffSizeReports: notable is gated at exactly NOISE_THRESHOLD_BYTES', () =>
8484

8585
test('diffSizeReports: notable also triggers from ramDelta alone, and honors negative deltas via Math.abs', () => {
8686
const base = { MATEKF405: { flash: 100000, ram: 50000 } };
87-
const pr = { MATEKF405: { flash: 100000, ram: 50000 - 40 } }; // flash unchanged, ram shrank by 40
87+
const pr = { MATEKF405: { flash: 100000, ram: 50000 - 300 } }; // flash unchanged, ram shrank by 300
8888
const row = diffSizeReports(pr, base).find((r) => r.target === 'MATEKF405');
8989

9090
assert.equal(row.flashDelta, 0);
91-
assert.equal(row.ramDelta, -40);
92-
assert.equal(row.notable, true, 'a -40 ram delta exceeds the 32-byte threshold in magnitude');
91+
assert.equal(row.ramDelta, -300);
92+
assert.equal(row.notable, true, 'a -300 ram delta exceeds NOISE_THRESHOLD_BYTES in magnitude');
9393
});
9494

9595
test('diffSizeReports: target missing from PR report but present in baseline -> missing-from-pr', () => {
@@ -155,9 +155,9 @@ test('renderComment: baseline present, all 4 targets compared, mix of notable/no
155155
MATEKH743: [530000, 63000],
156156
});
157157
const prReport = fullReport({
158-
MATEKF405: [500100, 60000], // +100 flash -> notable
158+
MATEKF405: [500300, 60000], // +300 flash -> notable
159159
MATEKF722: [510010, 61000], // +10 flash -> not notable
160-
MATEKF765: [519960, 62000], // -40 flash -> notable
160+
MATEKF765: [519700, 62000], // -300 flash -> notable
161161
MATEKH743: [530000, 63000], // no change -> not notable
162162
});
163163

@@ -179,9 +179,9 @@ test('renderComment: baseline present, all 4 targets compared, mix of notable/no
179179
const matekf765Line = lines.find((l) => l.startsWith('| MATEKF765'));
180180
const matekh743Line = lines.find((l) => l.startsWith('| MATEKH743'));
181181

182-
assert.ok(matekf405Line.includes('⚠️'), 'MATEKF405 (+100 flash) should be flagged notable');
182+
assert.ok(matekf405Line.includes('⚠️'), 'MATEKF405 (+300 flash) should be flagged notable');
183183
assert.ok(!matekf722Line.includes('⚠️'), 'MATEKF722 (+10 flash) should NOT be flagged notable');
184-
assert.ok(matekf765Line.includes('⚠️'), 'MATEKF765 (-40 flash) should be flagged notable');
184+
assert.ok(matekf765Line.includes('⚠️'), 'MATEKF765 (-300 flash) should be flagged notable');
185185
assert.ok(!matekh743Line.includes('⚠️'), 'MATEKH743 (no change) should NOT be flagged notable');
186186

187187
// No "no baseline available" note when a baseline was supplied.
@@ -286,3 +286,72 @@ test('renderComment: result always ends with exactly one trailing newline', () =
286286
assert.ok(body.endsWith('\n'));
287287
assert.ok(!body.endsWith('\n\n'));
288288
});
289+
290+
// ---------------------------------------------------------------------------
291+
// renderComment: baseline-commit header
292+
// ---------------------------------------------------------------------------
293+
294+
test('renderComment: baselineCommit supplied -> header names the baseline commit', () => {
295+
const body = renderComment({
296+
prReport: fullReport({ MATEKF405: [500000, 60000] }),
297+
baselineReport: fullReport({ MATEKF405: [499000, 60000] }),
298+
shortSha: 'abc1234',
299+
baselineCommit: '9e932ba',
300+
baselineIsNearest: false,
301+
docLink: null,
302+
marker: '<!-- marker -->',
303+
});
304+
305+
assert.ok(body.includes('vs. base commit `9e932ba`'), 'header should name the baseline commit');
306+
assert.ok(body.includes('commit `abc1234`'), 'header should still name the PR head commit');
307+
assert.ok(!body.includes('vs. base branch'), 'exact baseline should not use the generic "base branch" wording');
308+
assert.ok(!body.includes('nearest available size baseline'), 'exact baseline should not show the fallback note');
309+
assert.ok(!body.includes('No size baseline is available yet'), 'baseline present means no "no baseline" note');
310+
});
311+
312+
test('renderComment: baselineCommit + baselineIsNearest -> fallback note shown', () => {
313+
const body = renderComment({
314+
prReport: fullReport({ MATEKF405: [500000, 60000] }),
315+
baselineReport: fullReport({ MATEKF405: [499000, 60000] }),
316+
shortSha: 'abc1234',
317+
baselineCommit: '7f133b3',
318+
baselineIsNearest: true,
319+
docLink: null,
320+
marker: '<!-- marker -->',
321+
});
322+
323+
assert.ok(body.includes('vs. base commit `7f133b3`'), 'header should name the nearest baseline commit');
324+
assert.ok(body.includes('nearest available size baseline'), 'fallback note should explain the nearest-baseline choice');
325+
});
326+
327+
test('renderComment: baselineCommit omitted -> generic "vs. base branch" wording kept', () => {
328+
const body = renderComment({
329+
prReport: fullReport({ MATEKF405: [500000, 60000] }),
330+
baselineReport: fullReport({ MATEKF405: [499000, 60000] }),
331+
shortSha: 'abc1234',
332+
docLink: null,
333+
marker: '<!-- marker -->',
334+
});
335+
336+
assert.ok(body.includes('vs. base branch'), 'legacy callers without baselineCommit keep the old wording');
337+
assert.ok(!body.includes('base commit `'), 'no baseline commit rendered when none supplied');
338+
});
339+
340+
test('renderComment: baselineCommit supplied but no baseline report -> graceful note wins, no contradictory header', () => {
341+
// Defensive: the workflow only sets baselineCommit when a baseline was
342+
// found, so this combination should not occur; if it ever does, the
343+
// header must NOT claim a base commit was compared (no "vs. base
344+
// commit") and the "no baseline available" note must be present.
345+
const body = renderComment({
346+
prReport: fullReport({ MATEKF405: [500000, 60000] }),
347+
baselineReport: null,
348+
shortSha: 'abc1234',
349+
baselineCommit: '9e932ba',
350+
docLink: null,
351+
marker: '<!-- marker -->',
352+
});
353+
354+
assert.ok(body.includes('No size baseline is available yet'));
355+
assert.ok(!body.includes('vs. base commit `9e932ba`'), 'header must not name a baseline commit when no baseline exists');
356+
assert.ok(body.includes('vs. base branch'), 'header should fall back to the generic wording');
357+
});

0 commit comments

Comments
 (0)