Merge pull request 'Document the APK filename/versionCode naming contract' (#14) from apk-versioncode-naming-contract into main

Reviewed-on: #14

README.md

@@ -48,6 +48,7 @@ here.
 | `with-nixpkgs.sh` | run a command with one or more nixpkgs packages on PATH, pinned to the revision in `nixpkgs-pin` |
 | `check_zip_alignment.py` | the alignment check `sign-apk.sh` runs before signing (see "Align before sign" below) |
 | `next_version_code.py` | the stdlib-only implementation `next-version-code.sh` wraps (index read, arithmetic) |
+| `preserve_added.py` | stage last publish's `index-v2.json` so `fdroid update` reuses its per-build `added` dates, and check afterwards that it did (see "Per-build `added` timestamps" below) |
 | `action.yml` | the composite action a consumer includes to get all of the above |
 
 Deliberately **not** in scope: fetching the APK. That needs an upstream
@@ -82,6 +83,10 @@ only the repo index key, in the `fdroid-update.sh` step.
     FDROID_REPO_URL: https://fdroid-weblib.s3w.dev.chaosbit.de/repo
     FDROID_REPO_NAME: weblib
     FDROID_REPO_DESCRIPTION: "weblib's F-Droid repository"
+    # Last publish's index, fetched by the consumer (only it knows the
+    # bucket). Leave UNSET for a channel's first-ever publish; see
+    # "Per-build `added` timestamps" below.
+    FDROID_PREVIOUS_INDEX: /tmp/previous-index-v2.json
   run: |
     bash "${{ steps.ci.outputs.path }}/fdroid-update.sh" \
       "$FDROID_REPO_ROOT" "$INDEX_KEYSTORE" repo-key INDEX_KEYSTORE_PASSWORD
@@ -151,6 +156,79 @@ serves (and a client's "Add to F-Droid" 404s on `index-v2.json`) -- the
 `/repo` prefix is not optional decoration, it is what makes `repo_url`
 and the published tree agree at all.
 
+### Per-build `added` timestamps (`FDROID_PREVIOUS_INDEX`, weblib/weblib-viewer#95)
+
+**A repo that rebuilds `repo/` from scratch on every run must hand
+`fdroid-update.sh` the index it published last time, or every build in it
+gets re-stamped with the current wall clock.** That is
+weblib/weblib-viewer#95: an F-Droid client's version list showed five
+separate builds all "added" within seconds of each other, all at the moment
+of the most recent publish.
+
+It is not an mtime problem and `--use-date-from-apk` does not fix it.
+fdroidserver takes the date from `common.KnownApks`, which loads it out of
+an *already existing index*:
+
+```python
+for part in ('repo', 'archive'):
+    path = os.path.join(part, 'index-v2.json')
+    if os.path.isfile(path):
+        ...
+        filename = version["file"]["name"][1:]
+        date = datetime.fromtimestamp(version["added"] // 1000, tz=timezone.utc)
+        self.apks[filename] = date
+```
+
+and falls back to the wall clock for any filename it has not seen:
+
+```python
+if apkName not in self.apks:
+    if default_date is None:
+        default_date = datetime.now(timezone.utc)
+    self.apks[apkName] = default_date
+return self.apks[apkName]
+```
+
+`KnownApks`' own docstring says why there is nowhere else to look: the index
+is "currently the only way to permanently store the 'updated' date of APKs".
+A CI job on a fresh runner starts with no `repo/index-v2.json`, so
+`KnownApks` starts empty and *everything*, including APKs re-downloaded by a
+retention scheme, counts as new. (`--use-date-from-apk` only changes the
+fallback from `now()` to the local file's mtime, which for a re-downloaded
+file is the time it was downloaded -- no better.)
+
+So: fetch the previously published `index-v2.json` and point
+`FDROID_PREVIOUS_INDEX` at the local copy. `fdroid-update.sh` stages it
+where `KnownApks` looks, runs the update, and then **checks** that every
+build present in both indexes really did keep its date, failing the job if
+not -- fdroidserver's own `read_added_date_from_all_apks` carries a TODO
+about reading these dates differently, so this is verified rather than
+assumed.
+
+- **Unset it for a first-ever publish.** A path that is set but does not
+  exist is a hard failure, deliberately: a consumer whose fetch silently
+  produced nothing would otherwise flatten its entire history on every
+  publish and look perfectly healthy doing it.
+- A file that is not a parseable `index-v2.json` (a truncated download, an
+  S3 `NoSuchKey` error document) is refused **before** `fdroid update`
+  starts, naming the path -- `KnownApks` has no error handling of its own
+  and would abort mid-library with a traceback that never mentions the file.
+- A genuinely new build is untouched and still gets "now"; that is the one
+  date that *should* be the current time.
+- `lastUpdated` keeps meaning the real last update: fdroidserver derives it
+  as the newest `added` across the app's builds.
+- Fetching it is the consumer's job, not this repo's, for the same reason
+  fetching the APK is: only the consumer knows the bucket, and per
+  fdroid-repo-ci#1 this repo must not come to know. `fdroid-update.sh`
+  itself still needs no network.
+
+One caveat worth stating rather than discovering: the *app-level*
+`metadata.added` is derived by fdroidserver as the oldest `added` among the
+builds currently in the index, so it moves forward as old APKs age out of a
+retention window. That is inherent to retention -- the index cannot
+reference an APK that is no longer published -- and is not what #95 is
+about.
+
 ### The exception: a consumer whose build does not already sign
 
 `sign-apk.sh` is **not part of the flow above** and `action.yml` does not
@@ -272,6 +350,28 @@ collision; it prevents the collision from silently corrupting the repo.
 Skipped entirely for a plain `--index-only` invocation (below), since that
 pass can never upload a `*.apk` in the first place.
 
+### APK filenames must carry the versionCode (fdroid-repo-ci#13)
+
+That guard is correct -- a published versionCode's artifact has to stay
+immutable -- but it has a consequence nothing else here states: **the
+filename has to carry the versionCode**, or the guard permits exactly one
+publish into a prefix, ever. `publish.sh` checks the destination *key*, not
+the artifact's contents, so a build that always emits the same filename
+collides with itself on its second run regardless of what actually changed
+inside the APK.
+
+Name the APK `<applicationId>_<versionCode>.apk` -- F-Droid's own
+convention, and what `fdroid update` expects -- before handing it to
+`fdroid-update.sh`/`publish.sh`. Concretely: Flutter's
+`flutter build apk` always emits `app-<flavor>-release.apk`, independent of
+`--build-number`, so a consumer that publishes that path unchanged gets a
+successful first publish and then a permanent refusal on every publish
+after it -- exactly what happened to weblib-viewer (weblib-viewer#29) before
+it started renaming the artifact to `<applicationId>_<versionCode>.apk`
+ahead of this step. The failure only shows up on the *second* publish, long
+after the integration looks finished, and reads like a bucket-state problem
+(see the error text below) rather than the naming contract it actually is.
+
 ### `--files-only` and `--index-only`
 
 The two passes can also be run as two separate invocations:
@@ -438,7 +538,7 @@ here needs, resolved through the same pin.
 bash run_tests.sh
 ```
 
-98 assertions across seven suites offline (~20s on a warm nix store, 4
+123 assertions across eight suites offline (~20s on a warm nix store, 4
 deliberately skipped -- see below), plus 13 more that only run against a
 real bucket:
 
@@ -465,6 +565,25 @@ real bucket:
   the forwards clamp) are pinned independently, the latter via an
   undocumented env var seam, so every expected number is fixed rather than
   depending on whatever minute the suite happens to run in.
+* `test_preserve_added.py` -- standard library too, running
+  `preserve_added.py` as a subprocess against synthetic index-v2.json
+  fixtures: staging puts the file at exactly `repo/index-v2.json` (the only
+  path `common.KnownApks` looks at), a missing/truncated/not-an-index
+  previous index is refused with the path named and nothing staged, and
+  verification passes when every shared build kept its date while **failing**
+  when one drifted -- the negative control, since a checker that only ever
+  passes is worth nothing. Also: a build that aged out of the retention
+  window is not drift, a leading `/` on `file.name` (present in the index,
+  stripped by `KnownApks`) must not make every build look new, a version
+  entry with no `added` at all is skipped rather than defaulted, and the
+  sub-second tolerance is exactly one second wide -- fdroidserver only ever
+  emits whole seconds, but a whole second of drift still fails.
+  `test_fdroid_update.sh` covers the same feature's shell-level contract
+  (`FDROID_PREVIOUS_INDEX` set but missing, or set to garbage, aborts before
+  `fdroid update` runs); the preservation *itself*, end to end against real
+  APKs, was measured against a copy of the live dev index rather than in the
+  suite, because this repo's fixture APK is deliberately not parseable by
+  androguard and so never reaches the index at all.
 * `test_with_nixpkgs.sh` -- offline; every way the pinning wrapper is
   supposed to refuse rather than silently degrade (missing pin, malformed
   pin, too few arguments). Does not exercise the happy path of actually

action.yml

@@ -37,8 +37,8 @@ outputs:
   path:
     description: >-
       Directory holding with-nixpkgs.sh, nixpkgs-pin, sign-apk.sh,
-      fdroid-update.sh, next-version-code.sh, next_version_code.py and
+      fdroid-update.sh, next-version-code.sh, next_version_code.py,
-      publish.sh.
+      preserve_added.py and publish.sh.
     value: ${{ github.action_path }}
 
 runs:
@@ -48,6 +48,7 @@ runs:
       run: |
         echo "fdroid-repo-ci tools at ${{ github.action_path }}"
         for f in with-nixpkgs.sh nixpkgs-pin sign-apk.sh fdroid-update.sh \
-                 next-version-code.sh next_version_code.py publish.sh; do
+                 next-version-code.sh next_version_code.py preserve_added.py \
+                 publish.sh; do
           test -f "${{ github.action_path }}/$f"
         done

fdroid-update.sh

@@ -45,7 +45,36 @@
 # argument -- or the value embedded here (verbatim, as repo.address in
 # index-v2.json) will not be where the index actually is.
 #
+# FDROID_PREVIOUS_INDEX (optional) is a **local path** to the
+# `index-v2.json` this repo published last time. Give it and every build
+# that is still present keeps the date it was first published; omit it and
+# `fdroid update` stamps *every* APK in repo/ with the current wall clock,
+# which is weblib/weblib-viewer#95 -- a version list where five builds all
+# claim to have been added within seconds of each other.
+#
+# The dates live in the published index and nowhere else. fdroidserver's
+# `common.KnownApks` loads them from `repo/index-v2.json` (and
+# `archive/index-v2.json`) relative to the repo root, and falls back to
+# `datetime.now()` for any APK filename it has not seen -- its own docstring
+# calls the index "currently the only way to permanently store the 'updated'
+# date of APKs". A CI job that rebuilds repo/ on a fresh runner, as a
+# retention scheme re-downloading the previous APKs does, therefore starts
+# with an empty KnownApks. `preserve_added.py` (next to this script, and
+# where the full citation lives) puts the previous index back before the
+# run and checks afterwards that the dates really did survive; a build that
+# is genuinely new is untouched and still gets "now". Nothing here invents
+# or writes a date.
+#
+# A path that is set but does not exist is a hard failure, not a shrug: a
+# consumer whose download silently produced nothing would otherwise flatten
+# its whole history on every publish and look fine doing it. A genuine
+# first-ever publish has no previous index, and expresses that by leaving
+# the variable unset.
+#
 # Needs no network (fdroid-repo-ci#1) -- it only reads repo/ and signs.
+# FDROID_PREVIOUS_INDEX is a local file for that reason: fetching last
+# publish's index is the caller's job, since only the caller knows the
+# bucket (and, per fdroid-repo-ci#1, must remain the only one that does).
 # Pins fdroidserver, the JDK (fdroidserver shells out to jarsigner/keytool
 # for the index signature) and apksigner (for the per-entry v1 signature in
 # newer index formats) via with-nixpkgs.sh/nixpkgs-pin, next to this script.
@@ -59,7 +88,11 @@ usage: fdroid-update.sh <repo-root> <index-keystore> <key-alias> \
                          <keystore-pass-env> [<key-pass-env>] [extra fdroid update args...]
 
 required environment variables: FDROID_REPO_URL, FDROID_REPO_NAME
-optional environment variable:  FDROID_REPO_DESCRIPTION
+optional environment variables: FDROID_REPO_DESCRIPTION,
+                                FDROID_PREVIOUS_INDEX (local path to the
+                                previously published index-v2.json, so
+                                already-published builds keep the date they
+                                were first added)
 USAGE
   exit 2
 fi
@@ -90,6 +123,13 @@ extra_args=("$@")
 : "${FDROID_REPO_URL:?fdroid-update.sh: environment variable FDROID_REPO_URL (canonical repo URL, no trailing slash) is not set}"
 : "${FDROID_REPO_NAME:?fdroid-update.sh: environment variable FDROID_REPO_NAME (repo display name) is not set}"
 
+previous_index=${FDROID_PREVIOUS_INDEX:-}
+if [ -n "$previous_index" ] && [ ! -f "$previous_index" ]; then
+  echo "fdroid-update.sh: FDROID_PREVIOUS_INDEX is set to '$previous_index', which does not exist." >&2
+  echo "fdroid-update.sh: leave it unset for a first-ever publish; do not point it at a file that was never fetched." >&2
+  exit 2
+fi
+
 # YAML single-quoted scalar: doubling an embedded single quote is the only
 # escape it needs, and (unlike double-quoted) it does not interpret
 # backslashes -- so this is safe for an arbitrary URL/name/description
@@ -124,16 +164,44 @@ EOF
 } > "$root/config.yml"
 chmod 600 "$root/config.yml"
 
+# Stash the previous index somewhere `fdroid update` cannot reach. install
+# copies it *into* repo/, where the run then overwrites it -- so the "before"
+# side of the verification has to be read from a copy taken beforehand. It
+# also makes FDROID_PREVIOUS_INDEX=<root>/repo/index-v2.json (a caller
+# re-running in place) work rather than being a copy onto itself.
+previous_stash=
+if [ -n "$previous_index" ]; then
+  previous_stash=$(mktemp "${TMPDIR:-/tmp}/fdroid-previous-index.XXXXXX")
+  trap 'rm -f "$previous_stash"' EXIT
+  cp "$previous_index" "$previous_stash"
+fi
+
 echo "fdroid-update.sh: running fdroid update in $root"
 # fdroidserver locates a JDK by globbing standard system install locations
 # (or $JAVA_HOME) -- a nix store path matches neither, so JAVA_HOME has to be
 # set explicitly from whichever jarsigner this nix shell actually put on
 # PATH, rather than assumed.
-bash "$here/with-nixpkgs.sh" "fdroidserver jdk21 apksigner" bash -c '
+#
+# python3 is on the list for preserve_added.py: fdroidserver is a python
+# application but its nix output only exposes `fdroid` on PATH, not the
+# interpreter it runs on.
+bash "$here/with-nixpkgs.sh" "fdroidserver jdk21 apksigner python3" bash -c '
   set -euo pipefail
   export JAVA_HOME="$(dirname "$(dirname "$(command -v jarsigner)")")"
+  tools=$1; prev=$2; shift 2
   cd "$1"; shift
-  exec fdroid update "$@"
+  if [ -n "$prev" ]; then
-' bash "$root" "${extra_args[@]}"
+    python3 "$tools/preserve_added.py" install "$prev" repo
+  fi
+  fdroid update "$@"
+  # After, not instead of: install only makes the dates *available* to
+  # fdroidserver. This measures that they actually came out the other side,
+  # so a future fdroidserver that stops consulting the old index fails the
+  # publish here rather than silently flattening the history again
+  # (weblib/weblib-viewer#95).
+  if [ -n "$prev" ]; then
+    python3 "$tools/preserve_added.py" verify "$prev" repo/index-v2.json
+  fi
+' bash "$here" "$previous_stash" "$root" "${extra_args[@]}"
 
 echo "fdroid-update.sh: done; index at $root/repo/index-v2.json"

preserve_added.py

@@ -0,0 +1,208 @@
+#!/usr/bin/env python3
+"""Carry an F-Droid repo's per-build "added" timestamps across a rebuild.
+
+weblib/weblib-viewer#95: every version in the published index claimed it was
+added at the moment of the *last* publish, so a client's version list showed
+five builds all "added" within seconds of each other.
+
+## Why that happens (fdroidserver 2.4.3, verified in the source)
+
+`fdroid update` does not read an APK's mtime, and does not invent the date
+either. It asks `common.KnownApks`, whose `__init__` loads the dates out of
+an *already existing* index:
+
+    for part in ('repo', 'archive'):
+        path = os.path.join(part, 'index-v2.json')
+        if os.path.isfile(path):
+            ...
+            filename = version["file"]["name"][1:]
+            date = datetime.fromtimestamp(version["added"] // 1000, tz=timezone.utc)
+            self.apks[filename] = date
+
+and whose `recordapk` falls back to the wall clock for anything it has not
+seen before:
+
+    if apkName not in self.apks:
+        if default_date is None:
+            default_date = datetime.now(timezone.utc)
+        self.apks[apkName] = default_date
+    return self.apks[apkName]
+
+(`default_date` is only non-None under `--use-date-from-apk`, which uses the
+local file's mtime -- no better here, since a re-downloaded file's mtime is
+the time it was downloaded.)
+
+So the per-build `added` date is stored *in the published index and nowhere
+else* -- `KnownApks`' own docstring says as much: "This is currently the only
+way to permanently store the 'updated' date of APKs."
+
+A CI job that rebuilds `repo/` from scratch on a fresh runner -- which is
+exactly what a retention scheme re-downloading the previous APKs does --
+starts with no `repo/index-v2.json`, so `KnownApks` starts empty and *every*
+build, old ones included, gets `datetime.now()`. Hence "all apps show the
+date of the last update".
+
+## The fix
+
+Put last publish's `index-v2.json` back into `repo/` before `fdroid update`
+runs, and let fdroidserver's own mechanism do the rest. Nothing here writes
+a date itself; `install` stages the previous index and `verify` afterwards
+*measures* that the dates actually survived, so a future fdroidserver that
+drops this behaviour (its own `read_added_date_from_all_apks` carries a TODO
+about reading the dates differently) fails the publish loudly instead of
+silently flattening the history again.
+
+    preserve_added.py install <previous-index-v2.json> <repo-dir>
+    preserve_added.py verify  <previous-index-v2.json> <new-index-v2.json>
+
+Builds are keyed on the index's own `file.name`, because that is the key
+`KnownApks` itself uses -- not the versionCode, which is one level deeper in
+the JSON and is not what fdroidserver matches on.
+"""
+
+import argparse
+import json
+import os
+import shutil
+import sys
+
+
+def load_index(path, what):
+    """Parse an index-v2.json, rejecting anything KnownApks would choke on.
+
+    KnownApks indexes straight into `index["packages"]` and into each
+    version's `file.name`/`added` with no guards at all, so a truncated or
+    wrong-shaped file does not degrade -- it aborts `fdroid update` from
+    inside a library, with a traceback that says nothing about where the
+    file came from. Checking the shape here costs nothing and turns that
+    into one sentence naming the actual path.
+    """
+    try:
+        with open(path, "r", encoding="utf-8") as f:
+            index = json.load(f)
+    except FileNotFoundError:
+        sys.exit(f"preserve_added.py: no such {what}: {path}")
+    except json.JSONDecodeError as e:
+        sys.exit(f"preserve_added.py: {what} {path} is not valid JSON: {e}")
+
+    if not isinstance(index, dict) or not isinstance(index.get("packages"), dict):
+        sys.exit(
+            f"preserve_added.py: {what} {path} has no top-level 'packages' object "
+            "-- it does not look like an index-v2.json"
+        )
+    return index
+
+
+def added_by_file(index):
+    """Map {apk file name -> added (ms)} exactly as KnownApks keys it.
+
+    Entries with no `added`, or no `file.name`, are skipped rather than
+    defaulted: a missing date is not a date, and inventing one here would be
+    the very bug this script exists to stop.
+    """
+    out = {}
+    for pkg in index["packages"].values():
+        if not isinstance(pkg, dict):
+            continue
+        for version in (pkg.get("versions") or {}).values():
+            if not isinstance(version, dict):
+                continue
+            name = (version.get("file") or {}).get("name")
+            added = version.get("added")
+            if isinstance(name, str) and isinstance(added, int):
+                out[name.lstrip("/")] = added
+    return out
+
+
+def cmd_install(args):
+    previous = load_index(args.previous_index, "previous index")
+    dates = added_by_file(previous)
+
+    if not os.path.isdir(args.repo_dir):
+        sys.exit(f"preserve_added.py: no such repo directory: {args.repo_dir}")
+
+    target = os.path.join(args.repo_dir, "index-v2.json")
+    shutil.copyfile(args.previous_index, target)
+    print(
+        f"preserve_added.py: staged {args.previous_index} at {target}; "
+        f"fdroid update will carry the 'added' date of {len(dates)} known build(s)"
+    )
+    return 0
+
+
+def cmd_verify(args):
+    previous = added_by_file(load_index(args.previous_index, "previous index"))
+    new = added_by_file(load_index(args.new_index, "new index"))
+
+    carried, drifted = [], []
+    for name, was in previous.items():
+        if name not in new:
+            # Aged out of the retention window, or otherwise no longer
+            # published. Not this script's business.
+            continue
+        # Whole seconds, deliberately. fdroidserver stores `added` in
+        # milliseconds but only ever *produces* whole seconds
+        # (index.py: `int(calendar.timegm(obj.timetuple()) * 1000)`), and
+        # KnownApks reads it back as `version["added"] // 1000`. A
+        # hand-made previous index carrying a sub-second value would
+        # therefore come back rounded, through no fault of the mechanism
+        # being checked -- failing a publish over that would be a false
+        # alarm, while every real regression here is seconds to hours.
+        if new[name] // 1000 == was // 1000:
+            carried.append(name)
+        else:
+            drifted.append((name, was, new[name]))
+
+    for name, was, now in sorted(drifted):
+        print(
+            f"preserve_added.py: {name} was added at {was}, "
+            f"the new index says {now}",
+            file=sys.stderr,
+        )
+    fresh = sorted(set(new) - set(previous))
+    for name in fresh:
+        print(f"preserve_added.py: {name} is new in this index (added {new[name]})")
+
+    if drifted:
+        print(
+            f"preserve_added.py: {len(drifted)} already-published build(s) lost their "
+            "original 'added' timestamp (weblib/weblib-viewer#95)",
+            file=sys.stderr,
+        )
+        return 1
+
+    print(
+        f"preserve_added.py: {len(carried)} already-published build(s) kept their "
+        f"original 'added' timestamp, {len(fresh)} new"
+    )
+    return 0
+
+
+def main(argv=None):
+    parser = argparse.ArgumentParser(description=__doc__.splitlines()[0])
+    sub = parser.add_subparsers(dest="command", required=True)
+
+    p_install = sub.add_parser(
+        "install",
+        help="stage a previously published index-v2.json in repo/ so "
+        "`fdroid update` reuses its per-build 'added' dates",
+    )
+    p_install.add_argument("previous_index")
+    p_install.add_argument("repo_dir")
+    p_install.set_defaults(func=cmd_install)
+
+    p_verify = sub.add_parser(
+        "verify",
+        help="fail if any build present in both indexes changed its "
+        "'added' timestamp",
+    )
+    p_verify.add_argument("previous_index")
+    p_verify.add_argument("new_index")
+    p_verify.set_defaults(func=cmd_verify)
+
+    args = parser.parse_args(argv)
+    return args.func(args)
+
+
+if __name__ == "__main__":
+    sys.exit(main())

publish.sh

@@ -267,6 +267,18 @@ if [ "$run_files" = true ]; then
     fi
     if head_out=$("${aws_base[@]}" s3api head-object --bucket "$bucket" --key "$key" 2>&1); then
       echo "publish.sh: refusing to publish -- $key already exists in s3://$bucket (would overwrite an existing APK, fdroid-repo-ci#6)" >&2
+      # Hint only, never a control-flow change (fdroid-repo-ci#13): a
+      # colliding filename with no digits at all cannot be carrying a
+      # versionCode, which is the single most common way to trip this
+      # guard on every publish after the first -- Flutter's
+      # app-<flavor>-release.apk being the concrete case that motivated
+      # this. A filename that does contain digits still gets refused with
+      # just the message above, exactly as before; it may yet be a genuine
+      # collision (fdroid-repo-ci#6) and this heuristic cannot tell.
+      base=$(basename "$key")
+      if [[ $base != *[0-9]* ]]; then
+        echo "publish.sh: $base does not appear to carry a versionCode -- F-Droid's convention is <applicationId>_<versionCode>.apk; a constant filename plus this guard means only the first publish into a prefix can ever succeed" >&2
+      fi
       exit 1
     elif echo "$head_out" | grep -qi '404\|not found\|NoSuchKey'; then
       : # does not exist yet -- fine, this is the common case

run_tests.sh

@@ -7,10 +7,10 @@
 #
 #     bash run_tests.sh
 #
-# check_zip_alignment.py's and next_version_code.py's own suites need only
+# check_zip_alignment.py's, next_version_code.py's and preserve_added.py's
-# python3 (via with-nixpkgs.sh, since none is assumed to be on PATH);
+# own suites need only python3 (via with-nixpkgs.sh, since none is assumed
-# everything else additionally needs apksigner/fdroidserver/jdk21/awscli2,
+# to be on PATH); everything else additionally needs
-# resolved through the same pin. All of it stays on loopback/local-store --
+# apksigner/fdroidserver/jdk21/awscli2, resolved through the same pin. All of it stays on loopback/local-store --
 # nothing here reaches a real host, bucket or key, except the opt-in live
 # section inside test_publish.sh, which stays skipped unless
 # FDROID_CI_LIVE_BUCKET/FDROID_CI_LIVE_ENDPOINT and real credentials are set.
@@ -64,6 +64,7 @@ run_shell_suite() {
 
 run_python_suite test_check_zip_alignment.py
 run_python_suite test_next_version_code.py
+run_python_suite test_preserve_added.py
 run_shell_suite test_with_nixpkgs.sh
 run_shell_suite test_sign_apk.sh
 run_shell_suite test_fdroid_update.sh

test_fdroid_update.sh

@@ -131,4 +131,59 @@ assert repo['name']['en-US'] == \"bit's test repo\", repo['name']
 "
 export FDROID_REPO_NAME="fdroid-repo-ci test repo"
 
+# --- FDROID_PREVIOUS_INDEX (weblib/weblib-viewer#95) ---
+#
+# The preservation itself is measured by test_preserve_added.py (unit) and,
+# end to end, against a copy of the live dev index -- not here: this suite's
+# fixture APK is deliberately not parseable by androguard (see the top of
+# this file), so `fdroid update` indexes zero packages and there is no
+# `added` date to preserve. What IS testable here is the part that runs
+# before fdroidserver is ever reached: the contract that a set-but-wrong
+# FDROID_PREVIOUS_INDEX aborts loudly rather than silently flattening every
+# already-published build's history on the next publish.
+
+echo "--- fault: FDROID_PREVIOUS_INDEX pointing at a file that does not exist is refused ---"
+assert_failure "fdroid-update.sh refuses a FDROID_PREVIOUS_INDEX that does not exist" \
+  env FDROID_PREVIOUS_INDEX="$work/never-fetched.json" \
+    bash "$here/fdroid-update.sh" "$work" index.jks indexkey IDX_KS_PASS
+
+echo "--- fault: a truncated/garbage previous index is refused before fdroid update runs ---"
+printf '%s' '{"packages": {"de.example' > "$work/truncated.json"
+assert_failure "fdroid-update.sh refuses a previous index that is not valid JSON" \
+  env FDROID_PREVIOUS_INDEX="$work/truncated.json" \
+    bash "$here/fdroid-update.sh" "$work" index.jks indexkey IDX_KS_PASS
+
+echo "--- fault: valid JSON that is not an index (e.g. an S3 error document) is refused ---"
+printf '%s' '{"error": "NoSuchKey"}' > "$work/notanindex.json"
+assert_failure "fdroid-update.sh refuses a previous index with no top-level packages" \
+  env FDROID_PREVIOUS_INDEX="$work/notanindex.json" \
+    bash "$here/fdroid-update.sh" "$work" index.jks indexkey IDX_KS_PASS
+
+echo "--- a valid previous index is staged, survives the run, and is verified afterwards ---"
+# An empty-but-well-formed index is exactly the shape a channel's
+# first-ever publish would leave behind, and it proves the staged file does
+# not confuse `fdroid update` -- which overwrites it -- while still being
+# read back for the after-the-fact check.
+printf '%s' '{"repo": {}, "packages": {}}' > "$work/prev-empty.json"
+out="$work/prev-empty.log"
+if env FDROID_PREVIOUS_INDEX="$work/prev-empty.json" \
+     bash "$here/fdroid-update.sh" "$work" index.jks indexkey IDX_KS_PASS >"$out" 2>&1; then
+  pass "fdroid-update.sh runs with FDROID_PREVIOUS_INDEX set"
+else
+  fail "fdroid-update.sh runs with FDROID_PREVIOUS_INDEX set"
+  cat "$out" >&2
+fi
+assert_contains "the previous index was staged for fdroidserver to read" "$out" \
+  "fdroid update will carry the 'added' date of 0 known build(s)"
+assert_contains "the result was checked, not merely hoped for" "$out" \
+  "already-published build(s) kept their original 'added' timestamp"
+
+echo "--- and the staged copy never replaces the freshly generated index ---"
+assert_success "index-v2.json after the run is the new one, not the staged previous one" \
+  bash "$here/with-nixpkgs.sh" python3 python3 -c "
+import json
+index = json.load(open('repo/index-v2.json'))
+assert index['repo']['address'] == 'https://fdroid-repo-ci-test.example.invalid/repo', index['repo']
+"
+
 report_and_exit

test_preserve_added.py

@@ -0,0 +1,228 @@
+#!/usr/bin/env python3
+"""Offline tests for preserve_added.py (weblib/weblib-viewer#95).
+
+Standard library only, no network, no nix -- like test_next_version_code.py
+and test_check_zip_alignment.py, this is the piece of the change that needs
+none of the pinned toolchain. `fdroid update` itself is exercised by
+test_fdroid_update.sh, which needs fdroidserver; what is tested here is the
+part that has to be right *before* fdroidserver is ever reached and the part
+that judges what fdroidserver produced.
+
+Run as a *subprocess*, same reasoning the neighbouring suites give: the
+contract includes the exit code (fdroid-update.sh aborts a publish on a
+non-zero `verify`) and the messages, not just the return value of a function.
+
+The one property that matters, and the one a refactor could quietly break
+while still looking correct: `verify` must FAIL when a build that exists in
+both indexes changed its date. A checker that only ever passes is worth
+nothing, so every "carried" assertion below is paired with a drift case.
+"""
+import json
+import os
+import subprocess
+import sys
+import tempfile
+import unittest
+
+HERE = os.path.dirname(os.path.abspath(__file__))
+SCRIPT = os.path.join(HERE, "preserve_added.py")
+APP_ID = "de.example.app"
+
+
+def index(entries, app_id=APP_ID):
+    """Build a minimal index-v2.json shaped exactly like fdroidserver's.
+
+    `entries` is {versionCode: added_ms}. The map key under `versions` is a
+    sha256 in a real index; nothing here reads it, and using the versionCode
+    keeps the fixtures readable -- what both sides key on is `file.name`,
+    which is what `common.KnownApks` itself keys on.
+    """
+    versions = {}
+    for code, added in entries.items():
+        versions["%064x" % code] = {
+            "added": added,
+            "file": {"name": "/%s_%d.apk" % (app_id, code), "size": 1},
+            "manifest": {"versionCode": code, "versionName": "1.0"},
+        }
+    return {
+        "repo": {"timestamp": 0, "address": "https://example.invalid/repo"},
+        "packages": {app_id: {"metadata": {"added": 0}, "versions": versions}},
+    }
+
+
+class PreserveAddedTestCase(unittest.TestCase):
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self.tmp.cleanup)
+
+    def write(self, name, obj):
+        path = os.path.join(self.tmp.name, name)
+        with open(path, "w", encoding="utf-8") as f:
+            if isinstance(obj, str):
+                f.write(obj)
+            else:
+                json.dump(obj, f)
+        return path
+
+    def run_script(self, *args):
+        return subprocess.run(
+            [sys.executable, SCRIPT, *args],
+            capture_output=True,
+            text=True,
+        )
+
+    # --- install ---------------------------------------------------------
+
+    def test_install_puts_the_index_where_knownapks_looks_for_it(self):
+        """fdroidserver reads repo/index-v2.json and nothing else.
+
+        common.KnownApks.__init__ globs exactly os.path.join(part,
+        'index-v2.json') for part in ('repo', 'archive'), relative to the
+        repo root. A file staged anywhere else, or under any other name, is
+        invisible to it -- so the filename is the whole contract here.
+        """
+        prev = self.write("prev.json", index({1: 1000, 2: 2000}))
+        repo = os.path.join(self.tmp.name, "repo")
+        os.mkdir(repo)
+
+        result = self.run_script("install", prev, repo)
+        self.assertEqual(result.returncode, 0, result.stderr)
+
+        staged = os.path.join(repo, "index-v2.json")
+        self.assertTrue(os.path.isfile(staged))
+        with open(staged, encoding="utf-8") as f:
+            self.assertEqual(json.load(f), index({1: 1000, 2: 2000}))
+        self.assertIn("2 known build(s)", result.stdout)
+
+    def test_install_refuses_a_missing_repo_directory(self):
+        prev = self.write("prev.json", index({1: 1000}))
+        result = self.run_script("install", prev, os.path.join(self.tmp.name, "nope"))
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("no such repo directory", result.stderr)
+
+    def test_install_refuses_a_missing_previous_index(self):
+        repo = os.path.join(self.tmp.name, "repo")
+        os.mkdir(repo)
+        result = self.run_script(
+            "install", os.path.join(self.tmp.name, "gone.json"), repo
+        )
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("no such previous index", result.stderr)
+
+    def test_install_refuses_a_truncated_index(self):
+        """KnownApks has no error handling at all around json.load().
+
+        A half-written download would abort `fdroid update` from inside a
+        library, with a traceback that never names the file. Refuse it here,
+        naming the path, before the run starts.
+        """
+        prev = self.write("prev.json", '{"packages": {"a":')
+        repo = os.path.join(self.tmp.name, "repo")
+        os.mkdir(repo)
+        result = self.run_script("install", prev, repo)
+        self.assertNotEqual(result.returncode, 0)
+        self.assertIn("not valid JSON", result.stderr)
+        self.assertFalse(os.path.exists(os.path.join(repo, "index-v2.json")))
+
+    def test_install_refuses_valid_json_that_is_not_an_index(self):
+        """An S3 error document, or entry.json fetched by mistake, parses fine."""
+        for payload in ('{"error": "NoSuchKey"}', "[]", '{"packages": []}'):
+            with self.subTest(payload=payload):
+                prev = self.write("prev.json", payload)
+                repo = os.path.join(self.tmp.name, "repo")
+                os.makedirs(repo, exist_ok=True)
+                result = self.run_script("install", prev, repo)
+                self.assertNotEqual(result.returncode, 0)
+                self.assertIn("does not look like an index-v2.json", result.stderr)
+
+    # --- verify ----------------------------------------------------------
+
+    def test_verify_passes_when_every_shared_build_kept_its_date(self):
+        prev = self.write("prev.json", index({1: 1000, 2: 2000}))
+        new = self.write("new.json", index({1: 1000, 2: 2000, 3: 9000}))
+        result = self.run_script("verify", prev, new)
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("2 already-published build(s) kept", result.stdout)
+        self.assertIn("1 new", result.stdout)
+        self.assertIn("_3.apk is new", result.stdout)
+
+    def test_verify_fails_when_a_shared_build_drifted(self):
+        """The negative control: this is exactly weblib-viewer#95.
+
+        Both builds re-stamped with the current wall clock, which is what a
+        run with no previous index produces.
+        """
+        prev = self.write("prev.json", index({1: 1000, 2: 2000}))
+        new = self.write("new.json", index({1: 1789194194000, 2: 1789194195000}))
+        result = self.run_script("verify", prev, new)
+        self.assertEqual(result.returncode, 1)
+        self.assertIn("lost their original 'added' timestamp", result.stderr)
+        self.assertIn("_1.apk was added at 1000", result.stderr)
+        self.assertIn("_2.apk was added at 2000", result.stderr)
+
+    def test_verify_ignores_a_build_that_aged_out_of_retention(self):
+        """Retention deletes old APKs on purpose; that is not drift.
+
+        Only builds present in BOTH indexes are judged.
+        """
+        prev = self.write("prev.json", index({1: 1000, 2: 2000, 3: 3000}))
+        new = self.write("new.json", index({3: 3000, 4: 4000}))
+        result = self.run_script("verify", prev, new)
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("1 already-published build(s) kept", result.stdout)
+
+    def test_verify_accepts_a_first_publish_shaped_empty_index(self):
+        prev = self.write("prev.json", {"packages": {}})
+        new = self.write("new.json", index({1: 1000}))
+        result = self.run_script("verify", prev, new)
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("0 already-published build(s) kept", result.stdout)
+
+    def test_verify_tolerates_sub_second_rounding_only(self):
+        """fdroidserver stores ms but only ever emits whole seconds.
+
+        index.py serialises a datetime as
+        `int(calendar.timegm(obj.timetuple()) * 1000)` and KnownApks reads it
+        back as `version["added"] // 1000`, so a hand-made previous index
+        carrying a sub-second value comes back rounded through no fault of
+        the mechanism. That must not fail a publish -- but one whole second
+        of drift still must.
+        """
+        prev = self.write("prev.json", index({1: 1789194194678}))
+        result = self.run_script(
+            "verify", prev, self.write("new.json", index({1: 1789194194000}))
+        )
+        self.assertEqual(result.returncode, 0, result.stderr)
+
+        result = self.run_script(
+            "verify", prev, self.write("new2.json", index({1: 1789194195000}))
+        )
+        self.assertEqual(result.returncode, 1)
+
+    def test_verify_ignores_a_version_entry_with_no_added(self):
+        """A missing date is not a date; do not default it to anything."""
+        broken = index({1: 1000})
+        del list(broken["packages"][APP_ID]["versions"].values())[0]["added"]
+        prev = self.write("prev.json", broken)
+        new = self.write("new.json", index({1: 5000}))
+        result = self.run_script("verify", prev, new)
+        self.assertEqual(result.returncode, 0, result.stderr)
+
+    def test_leading_slash_on_file_name_is_stripped_consistently(self):
+        """index-v2 writes "/name.apk"; KnownApks strips the slash ([1:]).
+
+        Both sides of verify have to agree, or every build looks new.
+        """
+        prev_obj = index({1: 1000})
+        for v in prev_obj["packages"][APP_ID]["versions"].values():
+            v["file"]["name"] = v["file"]["name"].lstrip("/")  # no leading slash
+        prev = self.write("prev.json", prev_obj)
+        new = self.write("new.json", index({1: 1000}))  # with leading slash
+        result = self.run_script("verify", prev, new)
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("1 already-published build(s) kept", result.stdout)
+        self.assertNotIn("is new", result.stdout)
+
+
+if __name__ == "__main__":
+    unittest.main()

test_publish.sh

@@ -213,6 +213,45 @@ if [ "$have_python" = true ]; then
   else
     pass "publish.sh never starts pass 1 after refusing a colliding APK"
   fi
+  # de.example.app_1.apk carries a digit (the versionCode "1"), so the
+  # no-digits hint below (fdroid-repo-ci#13) must not fire for it -- this is
+  # the negative control for that hint, proving it is not just always on.
+  if echo "$out" | grep -q "does not appear to carry a versionCode"; then
+    fail "publish.sh must not hint about a missing versionCode when the colliding filename has digits (it did!)"
+  else
+    pass "publish.sh does not add the no-versionCode hint when the colliding filename has digits"
+  fi
+
+  echo "--- a colliding filename with no digits gets the versionCode hint (fdroid-repo-ci#13) ---"
+  # The Flutter case the issue is about: a build that always emits the same
+  # filename regardless of version, e.g. app-dev-release.apk. The guard
+  # still refuses exactly as above; only the message grows a line pointing
+  # at the naming contract, since a bucket-state-shaped error is what took a
+  # full CI cycle to diagnose in weblib-viewer#29.
+  nodigits_dir="$work/nodigits"
+  mkdir -p "$nodigits_dir"
+  echo '{}' > "$nodigits_dir/index-v2.json"
+  echo 'unsigned bytes, same name every build' > "$nodigits_dir/app-dev-release.apk"
+  start_stub mybucket "myprefix/app-dev-release.apk"
+  out=$(bash "$here/publish.sh" "$nodigits_dir" mybucket "http://127.0.0.1:$stub_port" myprefix 2>&1) && rc=0 || rc=$?
+  kill "$stub_pid" 2>/dev/null || true
+  wait "$stub_pid" 2>/dev/null || true
+  if [ "${rc:-0}" -ne 0 ]; then
+    pass "publish.sh refuses to publish a colliding APK filename with no digits"
+  else
+    fail "publish.sh refuses to publish a colliding APK filename with no digits (got exit 0)"
+  fi
+  assert_contains "the refusal names the colliding key" "$out" "myprefix/app-dev-release.apk"
+  assert_contains "the refusal explains why" "$out" "already exists"
+  assert_contains "the refusal hints that the filename carries no versionCode" "$out" \
+    "does not appear to carry a versionCode"
+  assert_contains "the hint states F-Droid's naming convention" "$out" \
+    "<applicationId>_<versionCode>.apk"
+  if echo "$out" | grep -q "pass 1/2"; then
+    fail "publish.sh must not start pass 1 after refusing a colliding no-digits APK (it did!)"
+  else
+    pass "publish.sh never starts pass 1 after refusing a colliding no-digits APK"
+  fi
 
   echo "--- negative control: a non-colliding APK does not trip the same check ---"
   start_stub mybucket   # nothing pre-exists
@@ -244,6 +283,7 @@ if [ "$have_python" = true ]; then
   fi
 else
   skip "credential-check-is-real / APK-collision-refusal (no python3 available, even via with-nixpkgs.sh)"
+  skip "APK-collision refusal includes the no-versionCode hint for a digit-free filename"
   skip "negative control: non-colliding APK does not trip the check"
   skip "--index-only never runs the APK-collision check"
 fi