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-<version> 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

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-<version> 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', () => {

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-<version> 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",
     [