diff --git a/extension/scripts/packaging.sh b/extension/scripts/packaging.sh index a8aa969..399aa43 100755 --- a/extension/scripts/packaging.sh +++ b/extension/scripts/packaging.sh @@ -6,7 +6,9 @@ # keeping three copies of one fact in sync by hand is how issue #2397 happened: # # 1. web-ext's --ignore-files (extension/package.json's four scripts) -# 2. the :(exclude) pathspec (ci.yml's extension-version guard) +# 2. the :(exclude) pathspec (what moves the version — a WIDER +# set than the ignore list; see +# NOT_VERSION_RELEVANT) # 3. the git-log pathspec (the derived version, below) # # They now all read from here. POSIX sh only — CI's run shell is busybox. @@ -35,6 +37,28 @@ set -euf NOT_PACKAGED_TRACKED='package.json package-lock.json README.md .gitignore vitest.config.js scripts scripts/** test test/**' NOT_PACKAGED_BUILD='web-ext-artifacts node_modules' +# Paths under extension/ that cannot change the SHIPPED BYTES, and so must not +# move the derived version. +# +# Deliberately NOT the same list as NOT_PACKAGED_TRACKED, and the whole +# difference is `scripts/`. packaging.sh is not packaged into the XPI — but it +# DECIDES the version string, and build.yml stamps that string into the +# manifest.json that is packaged. A change to how the version is computed is +# therefore a change to the shipped bytes. +# +# Excluding it was harmless only while every push rebuilt the web image. +# Milestone 313 step 4 made the rebuild conditional on the derived revision +# moving, which turned it into a silent failure: a packaging.sh change gives a +# NEW version, so sign-extension misses its ext- cache and signs — +# while build-web sees an unmoved revision, reuses the published image, and +# ships the OLD XPI. An orphaned AMO signature, and an instance quietly serving +# code the registry says is current. +# +# The two directions are not symmetric, which is why this list is the narrower +# one. Too wide costs a re-sign and a rebuild for a change that ships nothing +# new. Too narrow serves stale bytes and says nothing. +NOT_VERSION_RELEVANT='package.json package-lock.json README.md .gitignore vitest.config.js test test/**' + usage() { echo "usage: packaging.sh {ignore|pathspec|version|major-minor|patch}" >&2 exit 2 @@ -49,11 +73,14 @@ cmd_ignore() { echo "$NOT_PACKAGED_TRACKED $NOT_PACKAGED_BUILD" } -# git pathspec excluding the non-packaged tracked files, e.g. -# :(exclude)extension/package.json :(exclude)extension/test/** +# git pathspec excluding the tracked files that cannot change the shipped +# bytes, e.g. :(exclude)extension/package.json :(exclude)extension/test/** +# +# This answers "what moves the version?", NOT "what goes in the XPI?" — see +# NOT_VERSION_RELEVANT for why those differ. cmd_ignore answers the other one. # Same `set -f` requirement as above. cmd_pathspec() { - for entry in $NOT_PACKAGED_TRACKED; do + for entry in $NOT_VERSION_RELEVANT; do printf ':(exclude)extension/%s ' "$entry" done echo diff --git a/extension/test/version.spec.js b/extension/test/version.spec.js index 4e69a69..3b75088 100644 --- a/extension/test/version.spec.js +++ b/extension/test/version.spec.js @@ -44,9 +44,39 @@ describe('packaging.sh — the single definition of what ships', () => { // covering anything added later. const pathspec = packaging('pathspec') expect(pathspec).toContain(':(exclude)extension/test/**') - expect(pathspec).toContain(':(exclude)extension/scripts/**') expect(pathspec.some((e) => e.includes('.spec.js'))).toBe(false) expect(pathspec.some((e) => e.includes('helpers'))).toBe(false) + + const ignore = packaging('ignore') + expect(ignore).toContain('test/**') + expect(ignore).toContain('scripts/**') + expect(ignore.some((e) => e.includes('.spec.js'))).toBe(false) + }) + + it('lets packaging.sh move the version, though it never ships in the XPI', () => { + // The two lists answer different questions and this is the one place they + // disagree. scripts/ is ignored by web-ext — it is repo tooling, not addon + // code — but packaging.sh DECIDES the version string, and build.yml stamps + // that string into the manifest.json that does ship. So changing how the + // version is computed changes the shipped bytes. + // + // Excluding it from the pathspec was invisible while every push rebuilt the + // web image. Milestone 313 step 4 made that rebuild conditional on the + // derived revision moving, and the omission turned into a silent failure: + // a new version means sign-extension misses its ext- cache and + // signs, while build-web sees an unmoved revision, reuses the published + // image and ships the OLD XPI. An orphaned signature, and an instance + // serving code the registry calls current. + const pathspec = packaging('pathspec') + expect( + pathspec.some((e) => e.startsWith(':(exclude)extension/scripts')), + 'the pathspec excludes scripts/, so a change to how the version is ' + + 'derived would not move the version it derives', + ).toBe(false) + + // ...and it is still kept out of the package itself. Both must hold: the + // tempting "fix" for either half is to make the two lists one again. + expect(packaging('ignore')).toContain('scripts') }) it('keeps its own scripts and specs out of the XPI', () => { diff --git a/tests/test_artifact_paths.py b/tests/test_artifact_paths.py index c226f5b..8063517 100644 --- a/tests/test_artifact_paths.py +++ b/tests/test_artifact_paths.py @@ -110,6 +110,36 @@ def test_the_web_image_versions_on_an_extension_change(): ) +def test_the_web_image_versions_on_a_version_derivation_change(): + """packaging.sh ships in no image, yet it belongs in the sets that bundle + the XPI — because it decides the version string build.yml stamps into the + packaged manifest.json. Changing the derivation changes the shipped bytes. + + Left out, milestone 313 step 4 turns it silent: the new version misses the + ext- cache and gets signed, while web's revision has not moved, so + the reuse path republishes the old image and the fresh signature is + orphaned. Guarded for web and the extension both, since web bundles what + the extension produces. + """ + for artifact in ("extension", "web"): + inc = includes(artifact) + excluded = [ + p[len(":(exclude)"):] for p in declared_paths(artifact) + if p.startswith(":(exclude)") + ] + path = "extension/scripts/packaging.sh" + assert any(covered_by(path, i) for i in inc), ( + f"{path} is not in the {artifact} path set" + ) + assert not any( + covered_by(path, e.rstrip("*").rstrip("/")) for e in excluded + ), ( + f"{path} is excluded from the {artifact} path set, so a change to " + f"how the version is derived would not move the version — and " + f"step 4 would reuse the image that carries the old one" + ) + + @pytest.mark.parametrize( "artifact, path", [