sync: upstream f0e1174

.github/workflows/distro-sync.yml

@@ -158,14 +158,23 @@ jobs:
         # `--assembly` scopes the reported coverage to what actually merged: an
         # optional feature that conflicted is not in this binary, and a gate that
         # names it as covered contradicts the drop list this same job publishes.
+        # `--repo .` is the assembled checkout: each merged feature's declared
+        # `tests` directory is run there, which is the only positive-path coverage
+        # the distribution has -- byte-identity is blind to any path a feature
+        # turns ON, because those change output on purpose.
         run: |
           make -j"$(nproc)"
+          # The feature suites shell out to these probes. Built here rather than
+          # inherited from the earlier build step: a gate that depends on another
+          # step's side effect reports "probe not built" -- a build error wearing a
+          # test failure's clothes -- the first time the steps are reordered.
+          make -j"$(nproc)" -C api-test
           git worktree add /tmp/stock master
           make -C /tmp/stock -j"$(nproc)"
           python -m minibwa_dist.cli gates \
             --candidate ./minibwa --stock /tmp/stock/minibwa \
             --fixtures ./test --workdir /tmp/gates \
-            --assembly /tmp/assembly.json
+            --assembly /tmp/assembly.json --repo .
 
       - name: Re-parent dist-next onto dist
         # dist and dist-next are otherwise INDEPENDENT assemblies of the same
@@ -249,7 +258,13 @@ jobs:
           fi
 
       - name: File an issue for dropped features
-        if: steps.assemble.outputs.dropped != '[]' && steps.assemble.outputs.dropped != ''
+        # `always()`, because a REGRESSION fails the assemble step and would
+        # otherwise skip the one issue naming which features dropped and what they
+        # conflict in. That step writes its outputs before exiting precisely so
+        # this can still run. A regression therefore files this issue AND the
+        # generic `sync: run failed` one: different titles, different information,
+        # neither dedups against the other.
+        if: always() && steps.assemble.outputs.dropped != '[]' && steps.assemble.outputs.dropped != ''
         env:
           GH_TOKEN: ${{ github.token }}
         run: |
@@ -273,8 +288,13 @@ jobs:
           # the open issue instead of skipping, the way distro-reconcile.yml does,
           # and refresh the TITLE too: it names the set, so a stale one misreports
           # which features are actually missing from the build.
-          NUM=$(gh issue list --state open --search "sync: dropped in:title" --json number \
-                  --jq '.[0].number // empty')
+          # Token-matched search, then an exact client-side shape check: `gh issue
+          # edit` rewrites title AND body, so a loose match would clobber an
+          # unrelated issue that merely shares two common words. The previous form
+          # was immune only because a false hit just skipped the update.
+          NUM=$(gh issue list --state open --search "sync: dropped in:title" --json number,title \
+                  --jq '[.[] | select((.title | startswith("sync: dropped ")) and
+                                      (.title | endswith(" -- would not merge")))][0].number // empty')
           if [ -n "$NUM" ]; then
             gh issue edit "$NUM" --title "$TITLE" --body "$BODY"
           else
@@ -347,11 +367,13 @@ jobs:
           fi
           git worktree add /tmp/stock "$BASE"
           make -C /tmp/cand  -j"$(nproc)"
+          # The feature suites shell out to these; see the x86 gate step.
+          make -C /tmp/cand/api-test -j"$(nproc)"
           make -C /tmp/stock -j"$(nproc)"
           # dist-manifest.json carries this build's `merged` list, so coverage names
           # only the features actually in the binary -- same scoping the x86 gate
           # gets from --assembly, without passing an artifact between jobs.
           python -m minibwa_dist.cli gates \
             --candidate /tmp/cand/minibwa --stock /tmp/stock/minibwa \
             --fixtures /tmp/cand/test --workdir /tmp/gates \
-            --assembly /tmp/cand/dist-manifest.json
+            --assembly /tmp/cand/dist-manifest.json --repo /tmp/cand

README.md

@@ -75,8 +75,10 @@ structural changes anyway.
 
 Minibwa is designed for mapping short reads and accurate long reads. It does
 not support spliced alignment and has not been tuned for aligning long contigs.
-For now, minibwa does not properly work with alternate contigs in the reference
-genome. Please use a version of the reference without such contigs.
+Alternate contigs are supported: when a `<idx>.alt` file sits beside the index
+it is loaded automatically and ALT hits are reconciled with their primary
+counterparts, so an ALT twin no longer collapses the primary's mapping quality.
+Pass `--no-alt` to ignore it and align exactly as if no ALT file existed.
 
 ### Installation
 
@@ -161,7 +163,6 @@ include GPL'd source code.
 
 * Minibwa does not work with noisy long reads or spliced RNA-seq reads.
 * Minibwa does not support undirectional bisulfite sequencing data.
-* Minibwa does not recognize alternate haplotypes.
 
 [apache2]: https://github.com/lh3/minibwa/tree/Apache2
 [zlib]: https://zlib.net/

dist-manifest.json

@@ -1 +1 @@
-{"base": "f0e117436c28addc359b67123d2353f0d4a1f9e8", "head": "6ca35d1650f845604e2f1e0b560ec83e9ff8f518", "version": "0.7-nh13.dev+6ca35d1", "merged": ["ops-distro", "ll-affine-reassoc", "ksw2-extension-kernels", "single-copy-parser", "index-threads-v2", "extd2-avx512", "inline-appenders", "parallel-encode", "meth-cleanups", "meth-sam-tags", "soft-clip-penalty", "submem-ablation", "alt-liftgroup"], "dropped": [], "regressed": []}
+{"base": "f0e117436c28addc359b67123d2353f0d4a1f9e8", "head": "06ab16caf2925c7f0963218293f6877127af420a", "version": "0.7-nh13.dev+06ab16c", "merged": ["ops-distro", "ll-affine-reassoc", "ksw2-extension-kernels", "single-copy-parser", "index-threads-v2", "extd2-avx512", "inline-appenders", "parallel-encode", "meth-cleanups", "meth-sam-tags", "soft-clip-penalty", "submem-ablation", "alt-liftgroup"], "dropped": [], "regressed": []}

minibwa.h

@@ -3,7 +3,7 @@
 
 #include <stdint.h>
 
-#define MB_VERSION "0.7-nh13.dev+6ca35d1"
+#define MB_VERSION "0.7-nh13.dev+06ab16c"
 
 #define MB_F_PAF              (0x1LL)       // output in the PAF format
 #define MB_F_NO_UNMAP         (0x2LL)       // output unmapped query sequences

minibwa_dist/cli.py

@@ -107,6 +107,18 @@ def _gates(args: argparse.Namespace) -> int:
     A subcommand rather than two hand-copied heredocs in two workflows: the
     aggregation (`all(...)`) is the whole point of the step, and neither copy
     was reachable by `ruff check minibwa_dist/` or `pytest minibwa_dist/tests`.
+
+    `--repo` is where each merged feature's declared test suites are run. It has
+    no default -- it is not always the working directory, and guessing wrong
+    points the suites at a tree that does not carry them -- but it is optional
+    rather than required, and its absence is reported instead of raising.
+
+    That last part is not politeness. The engine and the workflow file update on
+    different schedules: every sync checks out the freshly assembled `dist-next`
+    and runs THAT `minibwa_dist`, while the YAML executing comes from `dist` and
+    changes only when a build is shipped. A newly required argument would be
+    demanded by the engine a full sync before the workflow could supply it, and
+    the gates step would die on argparse before the ship that would fix it.
     """
     manifest = load_manifest(Path(args.manifest))
     merged: tuple[str, ...] | None = None
@@ -119,9 +131,16 @@ def _gates(args: argparse.Namespace) -> int:
         manifest,
         Path(args.workdir),
         merged=merged,
+        repo=Path(args.repo) if args.repo else None,
     )
     for result in results:
         print(f"{'PASS' if result.passed else 'FAIL'}  {result.name}: {result.detail}")
+    unrun = [f.name for f in manifest.features if f.tests and (merged is None or f.name in merged)]
+    if args.repo is None and unrun:
+        # Not a gate result: it did not run, so it neither passed nor failed.
+        # Said out loud all the same -- unrun coverage that looks like absent
+        # coverage is how a gate quietly stops meaning anything.
+        print(f"NOTE  feature suites not run (--repo not given): {', '.join(unrun)}")
     return 0 if all(result.passed for result in results) else 1
 
 
@@ -241,6 +260,16 @@ def main(argv: list[str] | None = None) -> int:
         default=None,
         help="assembly JSON, so coverage names only the features this binary contains",
     )
+    # No default: the two gate jobs lay their workspaces out differently -- one
+    # runs from the assembled checkout, the other from the tooling checkout with
+    # the candidate in a worktree -- so any default is wrong for one of them. A
+    # "." default aimed the arm64 gate at the tooling checkout, which carries no
+    # suites. Optional rather than required, though: see _gates().
+    p.add_argument(
+        "--repo",
+        default=None,
+        help="the assembled checkout; each merged feature's declared test suites are run in it",
+    )
     p.set_defaults(func=_gates)
 
     p = sub.add_parser("stamp", help="set MB_VERSION to an exact version string")

minibwa_dist/features.toml

@@ -157,6 +157,10 @@ required = false
 output = "conditional"
 condition = "the index has ALT contigs"
 negative = "align the chrM fixture (no ALT contigs); SAM must be byte-identical to stock"
+# The negative case above is all byte-identity can prove: with no ALT contigs the
+# feature is inert. Its ~1,350 active lines only run when a .alt is present, and
+# these suites are the only thing that exercises them.
+tests = "test/altlg"
 summary = "ALT-aware mapping via post-extension liftover groups"
 upstream = { status = "unsubmitted" }
 

minibwa_dist/gates.py

@@ -54,6 +54,26 @@
 )
 
 
+# Bounded by the job, not by generosity: both gate jobs in distro-sync.yml cap at
+# `timeout-minutes: 60`, and this budget applies to each of a feature's suites in
+# turn. A value the whole set can outlast is a guard that never fires -- the
+# runner kills the job first and prints nothing at all. 120s x 20 suites is 40
+# minutes, leaving the builds and the byte-identity comparison the rest; the
+# in-tree ALT suites finish in seconds, so this is still enormous slack per
+# suite. `test_the_suite_timeout_fits_inside_the_job_timeout` holds the bound.
+_SUITE_TIMEOUT_S = 120
+
+# Enough failures to see the pattern, not so many that one gate line is a log
+# dump: 15 failing suites at 800 characters each is a 12KB single line.
+_MAX_REPORTED_FAILURES = 3
+
+
+def _tail(text: str, limit: int = 800) -> str:
+    """The last `limit` characters, which is where a shell suite says why it failed."""
+    text = text.strip()
+    return text if len(text) <= limit else "..." + text[-limit:]
+
+
 def _gate_name(label: str) -> str:
     """Gate name for a mode label.
 
@@ -155,13 +175,96 @@ def _modes_are_distinct(stock_digests: dict[str, str]) -> GateResult:
     )
 
 
+def run_feature_suites(
+    repo: Path, manifest: Manifest, merged: tuple[str, ...] | None
+) -> list[GateResult]:
+    """Run the `test-*.sh` suites of every merged feature that declares `tests`.
+
+    This is the positive-path coverage `run_gates` deliberately leaves out. Its
+    byte-identity comparison proves the merge did not disturb stock behaviour,
+    which is the property rerere can silently break -- but it is blind to every
+    path a feature turns ON, because those change output on purpose. A feature's
+    own suites are the only thing that looks at them, and until this existed
+    nothing ran them: `distro-test` covers the Python engine, and the sync
+    compiled the aligner without ever invoking `make test`.
+
+    Scoped to `merged` for the same reason the identity coverage line is: a
+    dropped feature's scripts are not in the tree, so a bare `make test` step
+    would fail with "no rule to make target" rather than a test failure, and
+    reporting it as covered would overstate what this build proved.
+
+    Suites run with `repo` as both the working directory and their sole
+    argument, matching the `[<minibwa-dir>]` convention the in-tree suites
+    already use to locate the binary they exercise.
+    """
+    results: list[GateResult] = []
+    for feature in manifest.features:
+        if feature.tests is None:
+            continue
+        if merged is not None and feature.name not in merged:
+            continue
+        name = f"tests:{feature.name}"
+        directory = repo / feature.tests
+        if not directory.is_dir():
+            results.append(
+                GateResult(
+                    name=name,
+                    passed=False,
+                    detail=f"declared suite directory '{feature.tests}' is not in the build",
+                )
+            )
+            continue
+        scripts = sorted(directory.glob("test-*.sh"))
+        if not scripts:
+            # Zero scripts is a pass over nothing, which is worse than no gate:
+            # it reports coverage that does not exist.
+            results.append(
+                GateResult(name=name, passed=False, detail=f"no test-*.sh in '{feature.tests}'")
+            )
+            continue
+        failures: list[str] = []
+        for script in scripts:
+            try:
+                proc = subprocess.run(
+                    ["sh", str(script), str(repo)],
+                    cwd=repo,
+                    capture_output=True,
+                    text=True,
+                    timeout=_SUITE_TIMEOUT_S,
+                )
+                ok, output = proc.returncode == 0, proc.stdout + proc.stderr
+            except subprocess.TimeoutExpired:
+                ok, output = False, f"timed out after {_SUITE_TIMEOUT_S}s"
+            if not ok:
+                failures.append(f"{script.name}: {_tail(output)}")
+        results.append(
+            GateResult(
+                name=name,
+                passed=not failures,
+                detail=(
+                    f"{len(scripts)} suite(s) passed"
+                    if not failures
+                    else f"{len(failures)} of {len(scripts)} failed -- "
+                    + " | ".join(failures[:_MAX_REPORTED_FAILURES])
+                    + (
+                        f" (+{len(failures) - _MAX_REPORTED_FAILURES} more)"
+                        if len(failures) > _MAX_REPORTED_FAILURES
+                        else ""
+                    )
+                ),
+            )
+        )
+    return results
+
+
 def run_gates(
     candidate_bin: Path,
     stock_bin: Path,
     fixture_dir: Path,
     manifest: Manifest,
     workdir: Path,
     merged: tuple[str, ...] | None = None,
+    repo: Path | None = None,
 ) -> list[GateResult]:
     """Run every gate implied by the manifest.
 
@@ -229,6 +332,9 @@ def in_build(name: str) -> bool:
                 )
             )
 
+    if repo is not None:
+        results.extend(run_feature_suites(repo, manifest, merged))
+
     for feature in manifest.features:
         # A dropped changes-output feature is not in the binary, so it has
         # nothing to block: failing the release for code this build does not

minibwa_dist/manifest.py

@@ -42,6 +42,11 @@ class Feature:
     upstream: Upstream
     condition: str | None = None
     negative: str | None = None
+    # Directory of `test-*.sh` suites this feature ships, run by the output gates
+    # when -- and only when -- the feature is in the build. This is the positive-
+    # path coverage the byte-identity gate deliberately does not provide: those
+    # paths change output on purpose, so identity says nothing about them.
+    tests: str | None = None
     # False for work that is ours to carry and never upstream's to take -- the
     # distribution's own tooling. Defaults true: a feature is a candidate for
     # upstreaming unless the manifest says otherwise.
@@ -125,6 +130,7 @@ def _parse_feature(table: dict[str, object], index: int) -> Feature:
 
     condition = table.get("condition")
     negative = table.get("negative")
+    tests = table.get("tests")
     if output == "conditional" and not (condition and negative):
         # Documentation, and required as such: the gate runs one default-flags
         # comparison that stands in for every conditional feature's negative
@@ -146,6 +152,7 @@ def _parse_feature(table: dict[str, object], index: int) -> Feature:
         upstream=Upstream(status=status, pr=int(pr_raw) if pr_raw is not None else None),
         condition=str(condition) if condition else None,
         negative=str(negative) if negative else None,
+        tests=str(tests) if tests else None,
         upstreamable=_optional_bool(table, "upstreamable", where, default=True),
     )