ci: run the integrity checks on every pull request

.github/workflows/main.yml

@@ -43,6 +43,13 @@ jobs:
           PREVIOUS_COMMIT_PR: ${{ github.event.pull_request.base.sha }}
           PREVIOUS_COMMIT: ${{ github.event.before }}
 
+      # --offline: the URI check downloads every airspace file the manifest
+      # points at, too slow and too dependent on third-party hosts to gate a
+      # PR.  check_repo_urls.yml runs it daily against the published manifest.
+      - name: "Check build"
+        run: |
+          ./check.sh ./output --offline
+
       - name: "output repofile"
         run: |
           ls -R ./output/

check.sh

@@ -1,31 +1,57 @@
 #!/bin/bash
 
-# Verify integrity.
+# Verify integrity of a build produced by build.sh.
+#
+# Usage: ./check.sh [BUILD_DIR] [--offline]
+#
+# BUILD_DIR is build.sh's output directory (default ./output).  Content sits in
+# its content/ subdirectory and the manifest beside that, so this takes the
+# build root rather than the content directory.
+#
+# --offline skips the URI check, which downloads and parses every airspace file
+# the manifest points at.  That is too slow and too dependent on third-party
+# hosts to gate a pull request; check_repo_urls.yml runs it daily against the
+# published manifest instead.
+
 ERROR=0
-OUT="${1}"
+OFFLINE=0
+ARGS=()
+
+for arg in "$@"; do
+  case "${arg}" in
+    --offline) OFFLINE=1 ;;
+    *) ARGS+=("${arg}") ;;
+  esac
+done
+
+OUT="${ARGS[0]}"
 
 # Set default to output if not specified
 if [ -z "${OUT}" ]; then
-  OUT="./output/content"
+  OUT="./output"
 fi
 
+CONTENT="${OUT}/content"
+
 # report all errors don't halt.
-if ! ./script/check/check_waypoints_country.py "${OUT}"/waypoint/country/*.cup; then
+if ! ./script/check/check_waypoints_country.py "${CONTENT}"/waypoint/country/*.cup; then
   ERROR=1
 fi
 
 while IFS= read -r -d '' each; do
   if ! ./script/check/check_waypoints.py "${each}"; then
     ERROR=1
   fi
-done < <(find "${OUT}/waypoint/" -type f -name "*.cup" -print0)
+done < <(find "${CONTENT}/waypoint/" -type f -name "*.cup" -print0)
 
-if ! ./script/check/check_airspaces.py "${OUT}"/airspace/; then
+if ! ./script/check/check_airspaces.py "${CONTENT}"/airspace/; then
   ERROR=1
 fi
 
-if ! ./script/check/check_urls.py "${OUT}"/repository; then
-  ERROR=1
+if [ "${OFFLINE}" = '0' ]; then
+  if ! ./script/check/check_urls.py "${OUT}"/repository; then
+    ERROR=1
+  fi
 fi
 
 if [ "${ERROR}" = '1' ]; then

requirements.txt

@@ -1,3 +1,4 @@
 aerofiles==1.5.6
 iso3166==3.0.0
+python-dateutil==2.9.0.post0
 requests==2.34.2

script/check/check_airspaces.py

@@ -1,7 +1,96 @@
 #!/usr/bin/env python3
+"""Check that local airspace files parse as OpenAir, without a single error.
+
+These are the files this repository ships, so a damaged record is ours to fix
+and tolerating one gains nothing: it costs the airspace it belongs to, and it
+costs the file its bbox, which repository.py derives from the same parser.
+Third-party files reached over a URI are judged leniently by check_urls.py
+instead, since a defect there cannot be repaired here.
+
+Takes files or directories; directories are searched for *.txt. check.sh passes
+a directory, so both have to work.
+
+Files are read as bytes and decoded leniently, because an encoding quirk in a
+comment header should not read as missing airspace.
+"""
+
+from __future__ import annotations
 
 import sys
-from aerofiles.openair.reader import Reader as OpenAirReader
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "lib"))
+
+from openair_content import describe, is_clean_openair  # noqa: E402
+
+
+def iter_airspace_files(args: list[str]) -> tuple[list[Path], list[Path]]:
+    """Expand command line arguments into (airspace files, empty directories).
+
+    A directory that holds no *.txt is reported rather than passed over: a
+    build that stopped producing airspace would otherwise be indistinguishable
+    from one with nothing to check, and check.sh hands this the whole airspace
+    tree.
+    """
+    paths: list[Path] = []
+    empty: list[Path] = []
+    for arg in args:
+        path = Path(arg)
+        if path.is_dir():
+            found = sorted(path.rglob("*.txt"))
+            if found:
+                paths.extend(found)
+            else:
+                empty.append(path)
+        else:
+            paths.append(path)
+    return paths, empty
+
+
+def check_file(path: Path) -> tuple[bool, str]:
+    """Returns (ok, message) for one airspace file."""
+    try:
+        raw = path.read_bytes()
+    except OSError as e:
+        return False, f"ERROR cannot read: {e}"
+
+    try:
+        text = raw.decode("utf-8")
+    except UnicodeDecodeError:
+        text = raw.decode("latin-1")
+
+    verdict = describe(text)
+    if not is_clean_openair(text):
+        return False, f"FAIL {verdict}"
+    return True, f"pass {verdict}"
+
+
+def main(args: list[str]) -> int:
+    paths, empty = iter_airspace_files(args)
+
+    for path in empty:
+        print(f"FAIL no *.txt in directory\t{path}")
+
+    if not paths and not empty:
+        print("No airspace files given.", file=sys.stderr)
+        return 1
+
+    failures = list(empty)
+    for path in paths:
+        ok, message = check_file(path)
+        print(f"{message}\t{path}")
+        if not ok:
+            failures.append(path)
+
+    if failures:
+        print("\nFAIL: airspace that does not parse cleanly:", file=sys.stderr)
+        for path in failures:
+            print(path, file=sys.stderr)
+        return 1
+
+    print(f"PASS: {len(paths)} airspace files parsed without errors.")
+    return 0
+
 
-openairfile = OpenAirReader()
-openairfile.read(open(str(sys.argv[1])))
+if __name__ == "__main__":
+    sys.exit(main(sys.argv[1:]))

script/check/check_urls.py

@@ -1,73 +1,166 @@
 #!/usr/bin/env python3
-"""Check if all the repository URLs are working."""
+"""Check that all the repository URIs work, and that airspace files hold airspace.
+
+A HEAD request only proves that something answered.  An airspace URI that has
+started serving an error page or an HTML redirect still passes that test, and
+the file reaches the pilot as an airspace file describing nothing -- daec.de in
+particular answers a missing file with HTTP 200 and a short error body.  Entries
+of type airspace pointing at a .txt are therefore downloaded and parsed.
+"""
+
+from __future__ import annotations
 
 import sys
-import requests
-from typing import List
+from collections.abc import Iterable, Iterator
 from pathlib import Path
+from urllib.parse import urlparse
+
+import requests
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "lib"))
+
+from openair_content import describe, looks_like_openair  # noqa: E402
+
+# Downloading airspace costs a response body rather than a header, so give it room.
+_AIRSPACE_TIMEOUT = 120
+
+# A host that refuses the runner says nothing about the file.  gliding.co.nz
+# answers 403 to every path including / and /robots.txt from a data centre, so
+# treating that as a dead link would fail the check on a file that pilots can
+# download perfectly well.  Reported separately instead of failing the run.
+_BLOCKED_STATUS = frozenset({403, 429, 451})
+
+
+def iter_records(lines: Iterable[str]) -> Iterator[dict[str, str]]:
+    """Yield the repository manifest's records as key/value dicts.
+
+    A record starts at its "name=" line and runs to the next one; comments and
+    blank lines are ignored.
+    """
+    record: dict[str, str] = {}
+    for raw in lines:
+        line = raw.strip()
+        if not line or line.startswith("#"):
+            continue
+        key, sep, value = line.partition("=")
+        if not sep:
+            continue
+        if key == "name" and record:
+            yield record
+            record = {}
+        record[key] = value
+    if record:
+        yield record
+
 
+def get_records_from_www(repo_url: str) -> list[dict[str, str]]:
+    """Read the manifest at repo_url."""
+    repo_req = requests.get(repo_url, timeout=60)
+    repo_req.raise_for_status()
+    return list(iter_records(repo_req.text.splitlines()))
 
-def get_urls_from_www(repo_url: str) -> List[str]:
-    """Extract all the URLs after "uri=" at repo_url."""
-    repo_req = requests.get(repo_url)
 
-    urls = []
-    for line in repo_req.iter_lines():
-        decoded_line = line.decode("utf-8")
-        if decoded_line.startswith("uri="):
-            urls.append(decoded_line[4:])
-    return urls
+def get_records_from_file(repo_file: Path) -> list[dict[str, str]]:
+    """Read the manifest at repo_file."""
+    with repo_file.open(encoding="utf-8") as in_file:
+        return list(iter_records(in_file))
 
 
-def get_urls_from_file(repo_file: Path) -> List[str]:
-    """Extract all the URLs after "uri=" in repo_file."""
-    urls = []
-    with repo_file.open() as in_file:
-        for line in in_file:
-            single_line = line.strip()
-            if single_line.startswith("uri="):
-                urls.append(single_line[4:].strip())
-    return urls
+def is_airspace_text(record: dict[str, str]) -> bool:
+    """Whether record is an airspace entry served as OpenAir text."""
+    if record.get("type") != "airspace":
+        return False
+    return urlparse(record.get("uri", "")).path.lower().endswith(".txt")
 
 
-def check_urls(urls: List[str]) -> (bool, List[str]):
-    """Check (by an HTTP HEAD request) the URLs in urls."""
+def check_record(record: dict[str, str], session: requests.Session) -> tuple[str, str]:
+    """Returns (outcome, message) for one manifest record.
+
+    outcome is "pass", "blocked" or "fail".
+    """
+    url = record["uri"]
+
+    if not is_airspace_text(record):
+        req = session.head(url, allow_redirects=True, timeout=60)
+        if req.status_code == requests.codes.ok:
+            return "pass", f"pass {req.status_code} {url}"
+        if req.status_code in _BLOCKED_STATUS:
+            return "blocked", f"BLOCKED {req.status_code} {url}\thost refused this client"
+        return "fail", f"FAIL {req.status_code} {url}\t!!!"
+
+    req = session.get(url, allow_redirects=True, timeout=_AIRSPACE_TIMEOUT)
+    if req.status_code in _BLOCKED_STATUS:
+        return "blocked", f"BLOCKED {req.status_code} {url}\thost refused this client"
+    if req.status_code != requests.codes.ok:
+        return "fail", f"FAIL {req.status_code} {url}\t!!!"
+
+    # Airspace files are published in whatever encoding the source uses, and the
+    # parse only needs the ASCII record keywords, so undecodable bytes are
+    # replaced rather than counted as a failure.
+    text = req.content.decode(req.encoding or "utf-8", errors="replace")
+    verdict = describe(text)
+    if not looks_like_openair(text):
+        return "fail", f"FAIL {req.status_code} {url}\tno airspace ({verdict})\t!!!"
+    return "pass", f"pass {req.status_code} {url}\t{verdict}"
+
+
+def check_urls(records: list[dict[str, str]]) -> tuple[bool, list[str], list[str]]:
+    """Check every record's URI. Returns (all_passed, failed_urls, blocked_urls)."""
     rv = True
     failed_urls = []
+    blocked_urls = []
+    session = requests.Session()
 
-    for i, url in enumerate(urls):
+    for i, record in enumerate(records):
+        url = record.get("uri")
+        if not url:
+            continue
         try:
-            req = requests.head(url, allow_redirects=True)
-            if req.status_code == requests.codes.ok:
-                print(f"{i}\tpass {req.status_code} {url}")
-            else:
-                print(f"{i}\tFAIL {req.status_code} {url}\t!!!")
-                failed_urls.append(url)
-                rv = False
+            outcome, message = check_record(record, session)
         except requests.RequestException as e:
             print(f"{i}\tERROR {url}\t{e}\t!!!")
             failed_urls.append(url)
             rv = False
+            continue
 
-    return rv, failed_urls
+        print(f"{i}\t{message}")
+        if outcome == "fail":
+            failed_urls.append(url)
+            rv = False
+        elif outcome == "blocked":
+            blocked_urls.append(url)
+
+    return rv, failed_urls, blocked_urls
 
 
 if __name__ == "__main__":
-    # allow to specify the repository as argument
+    # allow to specify the repository as argument, as a URL or as a local path
     if len(sys.argv) > 1:
-        repo_url = sys.argv[1]
+        repo = sys.argv[1]
     else:
-        repo_url = "http://download.xcsoar.org/repository"
+        repo = "http://download.xcsoar.org/repository"
 
-    url_list = get_urls_from_www(repo_url)
-    all_passed, failed_urls = check_urls(urls=url_list)
+    if urlparse(repo).scheme in ("http", "https"):
+        record_list = get_records_from_www(repo)
+    else:
+        record_list = get_records_from_file(Path(repo))
+
+    all_passed, failed, blocked = check_urls(records=record_list)
 
     if all_passed:
         print("PASS: All URIs downloaded successfully.")
     else:
         print("FAIL: Some/all URIs could not be downloaded.")
         print("Failed URLs:")
-        for url in failed_urls:
-            print(url)
+        for failed_url in failed:
+            print(failed_url)
+
+    if blocked:
+        # Not a failure: the host refused this client, which says nothing about
+        # the file.  Still worth a human look, because a removed file behind
+        # such a host looks exactly the same.
+        print("\nBlocked by the host, could not be checked:")
+        for blocked_url in blocked:
+            print(blocked_url)
 
     sys.exit(0 if all_passed else 1)