Skip to content

Commit aacdefb

Browse files
claudeEtienneLescot
authored andcommitted
ci(windows-store): check tiles against the generator, not against build/appx
The verification read build/appx/ to decide which assets to look for, which makes it blind in the one case it exists for: delete a PNG there and the name drops out of the expected set too, so the loop passes while electron-builder swaps its blank placeholder in under that exact name. An empty directory reported success with zero assets checked. The generator now owns the list — `--list` prints it, derived from ASSETS and TARGET_SIZES with no filesystem involved — and CI compares that list to both the committed directory (missing, or hand-added files) and the package. Refactoring the names out of the write loop also folded the targetsize variants into the same compose() path; at size == canvas with no shift it is the same pixels, and the regenerated assets are byte-identical. Reported by CodeRabbit on #294. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZV4hzYgcpTfL15wkxyYQz
1 parent 4f33804 commit aacdefb

2 files changed

Lines changed: 90 additions & 52 deletions

File tree

.github/workflows/build.yml

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -83,13 +83,32 @@ jobs:
8383
# the package carried electron-builder's vendored placeholder tiles: it reads
8484
# them from build/appx/ and, when a name is missing there, silently substitutes
8585
# a blank SampleAppx.*.png instead of failing. Nothing in the build output says
86-
# so — the only way to know is to look inside the package. This step does that,
87-
# comparing every committed asset against the one actually packaged, so a
88-
# renamed, dropped or substituted tile fails the build instead of failing
89-
# certification days later.
86+
# so — the only way to know is to look inside the package.
87+
#
88+
# The expected filenames come from the generator (`--list`), NOT from build/appx/.
89+
# Deriving them from the directory would defeat the check at the exact moment it
90+
# matters: delete an asset there and it drops out of the expected set too, so the
91+
# loop passes while electron-builder quietly packages a placeholder under that
92+
# name. The generator is the authority; build/appx/ is the artifact being checked.
9093
- name: Verify Store tiles are in the package
9194
shell: pwsh
9295
run: |
96+
$expected = @(node scripts/generate-appx-assets.mjs --list)
97+
if ($LASTEXITCODE -ne 0 -or $expected.Count -eq 0) { throw "generator produced no asset list" }
98+
99+
$problems = @()
100+
101+
# First: the committed directory must match the generator exactly. A deleted or
102+
# hand-added PNG is caught here, before it can reach the package.
103+
$committed = @(Get-ChildItem build/appx -Filter *.png | ForEach-Object { $_.Name })
104+
foreach ($name in $expected) {
105+
if ($committed -notcontains $name) { $problems += "missing from build/appx: $name" }
106+
}
107+
foreach ($name in $committed) {
108+
if ($expected -notcontains $name) { $problems += "unexpected file in build/appx (run npm run assets:appx): $name" }
109+
}
110+
111+
# Then: every expected asset must be in the package, byte-identical.
93112
$appx = Get-ChildItem release -Recurse -Filter *.appx | Select-Object -First 1
94113
if (-not $appx) { throw "no .appx found under release/" }
95114
Add-Type -AssemblyName System.IO.Compression.FileSystem
@@ -99,25 +118,26 @@ jobs:
99118
$entries = @{}
100119
foreach ($e in $archive.Entries) { $entries[$e.FullName.Replace("\", "/")] = $e }
101120
$sha = [System.Security.Cryptography.SHA256]::Create()
102-
$problems = @()
103-
$expected = Get-ChildItem build/appx -Filter *.png
104-
foreach ($asset in $expected) {
105-
$entry = $entries["assets/$($asset.Name)"]
106-
if (-not $entry) { $problems += "missing from package: $($asset.Name)"; continue }
121+
foreach ($name in $expected) {
122+
$entry = $entries["assets/$name"]
123+
if (-not $entry) { $problems += "missing from package: $name"; continue }
107124
$stream = $entry.Open()
108125
try { $packaged = [BitConverter]::ToString($sha.ComputeHash($stream)) }
109126
finally { $stream.Dispose() }
110-
$source = [BitConverter]::ToString($sha.ComputeHash([IO.File]::ReadAllBytes($asset.FullName)))
111-
if ($packaged -ne $source) { $problems += "packaged copy differs from build/appx: $($asset.Name)" }
112-
}
113-
if ($problems) {
114-
$problems | ForEach-Object { Write-Output "::error::$_" }
115-
throw "$($problems.Count) tile asset problem(s) in $($appx.Name)"
127+
$sourceFile = "build/appx/$name"
128+
if (-not (Test-Path $sourceFile)) { continue }
129+
$source = [BitConverter]::ToString($sha.ComputeHash([IO.File]::ReadAllBytes((Resolve-Path $sourceFile))))
130+
if ($packaged -ne $source) { $problems += "packaged copy differs from build/appx: $name" }
116131
}
117-
Write-Output "$($expected.Count) tile assets present in $($appx.Name), byte-identical to build/appx/"
118132
}
119133
finally { $archive.Dispose() }
120134
135+
if ($problems) {
136+
$problems | ForEach-Object { Write-Output "::error::$_" }
137+
throw "$($problems.Count) tile asset problem(s) in $($appx.Name)"
138+
}
139+
Write-Output "$($expected.Count) tile assets present in $($appx.Name), byte-identical to build/appx/"
140+
121141
- name: Upload Windows Store package
122142
uses: actions/upload-artifact@v4
123143
with:

scripts/generate-appx-assets.mjs

Lines changed: 54 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -280,58 +280,76 @@ function compose(icon, canvasWidth, canvasHeight, size, shiftY) {
280280

281281
// --- Generation -------------------------------------------------------------------
282282

283-
async function main() {
284-
const icon = decodePng(await readFile(SOURCE_ICON));
285-
286-
// Wipe first: a renamed or dropped asset left behind in build/appx/ would still be
287-
// mapped into the package by electron-builder, which copies the directory wholesale.
288-
await rm(OUT_DIR, { recursive: true, force: true });
289-
await mkdir(OUT_DIR, { recursive: true });
290-
291-
const written = [];
292-
const write = async (name, width, height, pixels) => {
293-
const file = path.join(OUT_DIR, name);
294-
await writeFile(file, encodePng(width, height, pixels));
295-
written.push(name);
296-
};
297-
283+
/**
284+
* Every asset this generator owns, as {name, width, height, size, shiftY}, derived from
285+
* ASSETS/TARGET_SIZES alone — no filesystem involved.
286+
*
287+
* This table, not the contents of build/appx/, is the authority on what the package must
288+
* contain. `--list` exposes it so CI can tell "this asset is missing" apart from "this
289+
* asset was never expected": a check that reads build/appx/ to decide what to look for
290+
* passes happily once a file is deleted there, which is precisely when electron-builder
291+
* swaps a blank placeholder in.
292+
*
293+
* The targetsize variants come out of compose() too rather than a bare resample: at
294+
* size == width == height with no shift, the canvas is fully covered, so the two are the
295+
* same pixels — and there is then a single code path to keep honest.
296+
*/
297+
function plannedAssets() {
298+
const planned = [];
298299
for (const asset of ASSETS) {
299300
const shiftY = asset.shiftY ?? 0;
300301
for (const scale of [100, ...(asset.scales ?? [])]) {
301302
const width = Math.round((asset.width * scale) / 100);
302303
const height = Math.round((asset.height * scale) / 100);
303304
const size = Math.round(Math.min(width, height) * asset.fill);
304-
if (size > icon.width) {
305-
throw new Error(
306-
`${asset.name} at ${scale}% needs a ${size}px icon; master is ${icon.width}px`,
307-
);
308-
}
309305
// The 100% variant stays unqualified so it is also the neutral MRT candidate:
310306
// if resources.pri ever fails to resolve a scale, Windows still finds art.
311307
const suffix = scale === 100 ? "" : `.scale-${scale}`;
312-
await write(
313-
`${asset.name}${suffix}.png`,
314-
width,
315-
height,
316-
compose(icon, width, height, size, shiftY),
317-
);
308+
planned.push({ name: `${asset.name}${suffix}.png`, width, height, size, shiftY });
318309
}
319310
}
320-
321311
for (const target of TARGET_SIZES) {
322-
const pixels = resample(icon.pixels, icon.width, icon.height, target, target);
323-
await write(`Square44x44Logo.targetsize-${target}.png`, target, target, pixels);
324-
await write(
325-
`Square44x44Logo.targetsize-${target}_altform-unplated.png`,
326-
target,
327-
target,
328-
pixels,
329-
);
312+
for (const suffix of ["", "_altform-unplated"]) {
313+
planned.push({
314+
name: `Square44x44Logo.targetsize-${target}${suffix}.png`,
315+
width: target,
316+
height: target,
317+
size: target,
318+
shiftY: 0,
319+
});
320+
}
321+
}
322+
return planned;
323+
}
324+
325+
async function main() {
326+
const planned = plannedAssets();
327+
328+
if (process.argv.includes("--list")) {
329+
console.log(planned.map((it) => it.name).join("\n"));
330+
return;
331+
}
332+
333+
const icon = decodePng(await readFile(SOURCE_ICON));
334+
for (const { name, size } of planned) {
335+
if (size > icon.width) {
336+
throw new Error(`${name} needs a ${size}px icon; master is ${icon.width}px`);
337+
}
338+
}
339+
340+
// Wipe first: a renamed or dropped asset left behind in build/appx/ would still be
341+
// mapped into the package by electron-builder, which copies the directory wholesale.
342+
await rm(OUT_DIR, { recursive: true, force: true });
343+
await mkdir(OUT_DIR, { recursive: true });
344+
345+
for (const { name, width, height, size, shiftY } of planned) {
346+
const pixels = compose(icon, width, height, size, shiftY);
347+
await writeFile(path.join(OUT_DIR, name), encodePng(width, height, pixels));
330348
}
331349

332350
const total = (await readdir(OUT_DIR)).length;
333351
console.log(
334-
`${written.length} assets written to ${path.relative(ROOT, OUT_DIR)} (${total} files)`,
352+
`${planned.length} assets written to ${path.relative(ROOT, OUT_DIR)} (${total} files)`,
335353
);
336354
}
337355

0 commit comments

Comments
 (0)