diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cda6fa43..4698ec02 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -134,6 +134,15 @@ jobs: python3 -m pip install --quiet PyYAML python3 test/tooling/make_flatpak_smoke_manifest_test.py + # The Flathub linter itself needs a Flatpak and, for two of its three + # modes, a 90-minute build, so it runs in the Flatpak workflow. What + # runs here is the judgement wrapped around it: that a missing linter is + # not a pass, that a finding needs a written reason, and that a reason + # for a finding the linter no longer reports is itself a failure (#449). + # Local twin: `python3 test/tooling/flathub_builder_lint_test.py`. + - name: Test the Flathub lint runner + run: python3 test/tooling/flathub_builder_lint_test.py + release-bump-guard: name: Release bump touches only version files runs-on: ubuntu-latest diff --git a/.github/workflows/flatpak-build.yml b/.github/workflows/flatpak-build.yml index 2fb6da50..b551c144 100644 --- a/.github/workflows/flatpak-build.yml +++ b/.github/workflows/flatpak-build.yml @@ -19,6 +19,7 @@ on: - 'pubspec.lock' - 'scripts/check_linux_runner.py' - 'scripts/flatpak_audio_smoke.sh' + - 'scripts/flathub_builder_lint.py' - 'scripts/flatpak_launch_smoke.sh' - 'scripts/flatpak_local_library_smoke.sh' - 'scripts/make_flatpak_smoke_manifest.py' @@ -51,6 +52,7 @@ on: - 'pubspec.lock' - 'scripts/check_linux_runner.py' - 'scripts/flatpak_audio_smoke.sh' + - 'scripts/flathub_builder_lint.py' - 'scripts/flatpak_launch_smoke.sh' - 'scripts/flatpak_local_library_smoke.sh' - 'scripts/make_flatpak_smoke_manifest.py' @@ -150,6 +152,26 @@ jobs: flatpak remote-add --user --if-not-exists \ flathub https://dl.flathub.org/repo/flathub.flatpakrepo + # Flathub's own submission linter (#449), which is a different question + # from the appstreamcli/desktop-file validation above: those check that + # two files are well-formed, this checks whether a submission would be + # accepted. It ships inside org.flatpak.Builder, which is how Flathub's + # documentation runs it, so this is the same tool a reviewer uses. + - name: Install Flathub's builder linter + run: | + set -euo pipefail + flatpak install --user -y flathub org.flatpak.Builder + + # Before the build, because a manifest finding does not need one. The + # runner script fails on any finding without a written reason in + # flatpak/flathub-lint-exceptions.json, and equally on a reason for a + # finding the linter no longer reports. + - name: Lint the submission manifest + run: | + set -euo pipefail + python3 scripts/flathub_builder_lint.py \ + --manifest flatpak/io.github.thezupzup.linthra.yml + - name: Install manifest-declared SDK and runtime prerequisites working-directory: flatpak run: | @@ -224,10 +246,39 @@ jobs: flatpak build-update-repo repo-ci echo 'Flatpak build and repository export completed successfully.' + # Before the lint below, deliberately. A failed step ends the job, so a + # lint finding sitting ahead of this would stop the package being + # installed and launched at all, which trades real coverage for a red + # badge. - name: Install and launch packaged Flatpak working-directory: flatpak run: bash ../scripts/flatpak_launch_smoke.sh repo-ci + # The AppStream catalogue appstreamcli compose produced from the metainfo + # during the build. This is what a software centre actually reads, and it + # is not the file the pre-build validation step checked. + - name: Lint the AppStream catalogue + run: | + set -euo pipefail + python3 scripts/flathub_builder_lint.py \ + --builddir flatpak/flatpak-builder-ci + + # The third mode, `repo` (the exported OSTree, what Flathub would + # actually publish), is deliberately NOT wired up yet. It reports + # metainfo-missing-screenshots and + # appstream-screenshots-not-mirrored-in-ostree, which are real submission + # blockers fixed by taking screenshots (#437), not by anything in the + # tooling. + # + # Turning it on now would mean either a permanently red job or an + # exception recording "not done yet", and the exceptions file exists for + # findings a human judged wrong or unavoidable, not for unfinished work. + # It arrives with the screenshots that let it pass (#628). + # + # `scripts/flathub_builder_lint.py --repo ...` runs today and is + # documented in docs/flathub-builder-lint.md; it is the CI wiring that + # waits, not the capability. + # A second build, on its own runner, because the sandbox smokes need binaries # the shipped package must not contain (#446, #447). Nobody wants a test # harness in a published Flathub app, so the submission manifest stays free of diff --git a/docs/README.md b/docs/README.md index 6063b8de..aba8b563 100644 --- a/docs/README.md +++ b/docs/README.md @@ -55,6 +55,7 @@ The full index of Linthra's docs. New to the project? Start with the | Flatpak audio playback smoke | [flatpak-audio-smoke.md](./flatpak-audio-smoke.md) | | Flatpak local-library sandbox smoke | [flatpak-local-library-smoke.md](./flatpak-local-library-smoke.md) | | Flatpak sandbox filesystem audit | [flatpak-filesystem-audit.md](./flatpak-filesystem-audit.md) | +| Flathub builder lint | [flathub-builder-lint.md](./flathub-builder-lint.md) | | Flathub updates (release to published Flatpak) | [flathub-update-process.md](./flathub-update-process.md) | ## Licensing & legal diff --git a/docs/flathub-builder-lint.md b/docs/flathub-builder-lint.md new file mode 100644 index 00000000..2092af97 --- /dev/null +++ b/docs/flathub-builder-lint.md @@ -0,0 +1,235 @@ +# Flathub builder lint + +Issue: [#449](https://github.com/TheZupZup/Linthra/issues/449) +Parent: #376 + +CI has validated Linthra's desktop entry and metainfo for a while, with +`desktop-file-validate` and `appstreamcli validate`. Those answer a different +question: whether two files are well-formed. + +Flathub's own linter answers the one that matters for a submission: whether it +would be *accepted*. It checks whether the sources are reproducible, whether the +permissions are ones Flathub grants, whether the metainfo carries what a +software centre needs to show the app at all, and a long list of listing-quality +rules that no XML validator knows about. + +## The exact commands + +The linter ships inside the `org.flatpak.Builder` Flatpak, which is how +Flathub's own documentation runs it, the same tool a reviewer uses. + +```sh +flatpak install -y flathub org.flatpak.Builder + +# The manifest, before anything is built. +flatpak run --command=flatpak-builder-lint org.flatpak.Builder \ + manifest flatpak/io.github.thezupzup.linthra.yml + +# The exported repository, what Flathub would publish. +flatpak run --command=flatpak-builder-lint org.flatpak.Builder \ + repo flatpak/repo-ci + +# The AppStream catalogue appstreamcli compose produced during the build. This +# is what a software centre actually reads, and is not the file +# `appstreamcli validate` checked. +flatpak run --command=flatpak-builder-lint org.flatpak.Builder \ + appstream flatpak/flatpak-builder-ci/files/share/app-info/xmls/io.github.thezupzup.linthra.xml.gz +``` + +Two of the three need a completed build, so the practical way to run all of +them is the sequence in [flatpak-ci.md](./flatpak-ci.md) followed by: + +```sh +# That sequence ends inside flatpak/, and the paths below are relative to the +# repository root. +cd .. + +python3 scripts/flathub_builder_lint.py \ + --manifest flatpak/io.github.thezupzup.linthra.yml +python3 scripts/flathub_builder_lint.py \ + --repo flatpak/repo-ci \ + --builddir flatpak/flatpak-builder-ci +``` + +`scripts/flathub_builder_lint.py` runs exactly the commands above. It exists for +what it refuses to call a pass, not to wrap them. + +## What the runner refuses + +**A missing linter is not a pass.** If `flatpak` or `org.flatpak.Builder` is +absent, the script exits 2 and says nothing was linted. A check that silently +succeeds when its tool is missing is worse than no check, because it is +reported as evidence. + +Silence is read carefully, in both directions. flatpak-builder-lint 3.x prints +*nothing at all* when a mode finds nothing, and exits 0. That is a clean +report, and it cannot be confused with a missing tool because the linter's +presence is established before any mode runs. Silence with a **non-zero** exit +explains nothing and is still refused. + +Nor does every mode speak JSON: `appstream` hands the catalogue to +appstreamcli and prints its verdict as text. There the exit code is the +verdict, and the text is printed either way. + +**A finding needs a written reason.** Any error or warning that is not listed +in `flatpak/flathub-lint-exceptions.json` fails the run. Warnings count: +reviewers read them, and #449 is about reaching a submission-quality result +rather than an exit code. + +**A reason cannot outlive its problem.** An exception the linter no longer +reports also fails the run, so the file cannot quietly accumulate justifications +for things that were fixed years ago. + +**Exception keys carry their mode**, as `manifest/`, `repo/` or `appstream/`, +for example `repo/appstream-screenshots-not-mirrored-in-ostree`. CI does not +run every mode in one invocation: the manifest is linted before the build and +the repo and catalogue after it. Without the mode, the staleness check above +could not tell an exception whose problem was fixed from one whose mode simply +did not run in that invocation, and would fail the build on a live exception. +Qualifying the key also stops a reason written for one mode accepting a +same-named finding from another. + +**An exception with no reason is rejected outright**: an empty string there is +a suppression wearing a different hat. + +**A `-lint-failed` finding cannot be excepted at all.** That name means +the linter itself failed rather than reporting something about the submission, +and it is the same name whatever the failure was. A reason attached to it would +go on matching after the original problem was fixed and a different one +appeared, which is the one thing the staleness rule exists to prevent. + +## Tool and version assumptions + +- The linter's rules change with the tool, so "clean" is only meaningful next to + the version that said so. The runner prints `flatpak --version`, the + `org.flatpak.Builder` ref and commit, and the linter's own version before any + result. +- `org.flatpak.Builder` is deliberately **not pinned**. Flathub reviews + submissions with whatever is current, so pinning would make this check green + against a linter nobody uses. The cost is that a new linter release can turn + the build red for a reason the PR did not cause; the version line in the log + is what makes that obvious, and the fix is to fix the finding. +- The `repo` and `appstream` modes need a completed `flatpak-builder` run. Only + `manifest` is cheap. + +## What CI gates on today + +| Mode | Gates CI | Result today | +| --- | --- | --- | +| `manifest` | yes, before the build | clean | +| `appstream` | yes, after the build | clean | +| `repo` | not yet, see below | two screenshot findings | + +`manifest` runs before the build, so a manifest finding fails in about a second +rather than after ninety minutes. `appstream` needs the build, and runs *after* +the launch smoke: a failed step ends the job, so a lint ahead of the smoke would +stop the package being installed and launched at all, which trades real +coverage for a red badge. + +`repo` is the one that waits. It reports `metainfo-missing-screenshots` and +`appstream-screenshots-not-mirrored-in-ostree`, both real submission blockers +whose fix is to take screenshots +([#437](https://github.com/TheZupZup/Linthra/issues/437), +[flathub-screenshots.md](./flathub-screenshots.md)) rather than to change +anything here. Wiring it in before then would mean one of two things, and both +are worse than waiting: + +- a permanently red job, which trains everyone to ignore it and buries any + *new* finding under the one everybody already knows about; +- an exception, which the rules above forbid for a finding that is simply not + fixed yet. + +It is turned on in +[#628](https://github.com/TheZupZup/Linthra/issues/628), together with the +screenshots that let it pass. A guardrail in +`test/tooling/flathub_metadata_guardrails_test.dart` fails if it is wired in +without that, so switching it on is a decision someone makes rather than +something that drifts in. + +Note that `appstream` and `repo` are different checks despite both mentioning +screenshots: `appstream` reads the catalogue `appstreamcli compose` generated, +and `repo` reads the exported OSTree that Flathub would publish. Only the second +currently reports anything, which is why only the second waits. + +## Exceptions + +`flatpak/flathub-lint-exceptions.json` is **empty**, and that is the state +[#456](https://github.com/TheZupZup/Linthra/issues/456) requires before +submission. + +An entry there is not a way to make the check green. It is a claim that a person +looked at a finding and decided the linter is wrong or the problem is +unavoidable for Linthra, and the reason has to say which. A finding that is +simply *not fixed yet* belongs in its issue, not here. + +## Where it stands + +First real run, against `flatpak-builder-lint 3.0.0.post798.dev0+5181352`: + +| Mode | Result | +| --- | --- | +| `manifest` | **clean** | +| `appstream` | **clean**, "Validation was successful." | +| `repo` | two errors, both the same missing screenshots | + +The `repo` findings: + +```text +appstream-screenshots-not-mirrored-in-ostree (error) +metainfo-missing-screenshots (error) +info: metainfo-missing-screenshots: The metainfo file is missing screenshots + or it is not present under the screenshots/screenshot/image tag +``` + +Notably *not* reported: the manifest's `type: dir` source. The development +manifest builds the working checkout, which is right for testing a PR and wrong +for a submission, but that is a Flathub *submission* requirement enforced +elsewhere, not something this linter flags, so the submission manifest +([#451](https://github.com/TheZupZup/Linthra/issues/451)) is still needed and +this check will not tell you so. + +### Why the screenshots are not excepted + +Flathub requires at least one screenshot, and it is right to. Inventing them +from the Android set would misrepresent the desktop window, so the fix is real +Linux captures ([#437](https://github.com/TheZupZup/Linthra/issues/437)) landed +through the metadata pass +([#450](https://github.com/TheZupZup/Linthra/issues/450)). + +Until then the `repo` mode is red, and that is the honest state: the linter has +found something real that Linthra has not fixed yet. An entry in the exceptions +file would only record that we would rather not see it. + +## Cheap checks that do not need a build + +`test/tooling/flathub_metadata_guardrails_test.dart` holds the handful of +Flathub quality rules that are stable, textual and free to check: the 35- +character summary cap, no trailing period, no leading article, no repeated app +name, the 20-character name cap, the required metainfo elements, and the desktop +entry's visibility and icon name. + +It is not a reimplementation of the linter: everything else is the linter's +job. It exists so a regression is caught on the PR that causes it rather than in +a submission review. The 35-character cap is the one that already bit: the +summary was 36. + +## Where this runs + +The `Build and launch Flatpak` job in `.github/workflows/flatpak-build.yml` +lints the manifest before the build and the AppStream catalogue after the +export. It does **not** currently lint the exported repository: that mode is +deferred, per the table above, so CI passing is not evidence that the +publishable OSTree was checked. Run it locally, or wait for +[#628](https://github.com/TheZupZup/Linthra/issues/628). + +`.github/workflows/ci.yml` runs the runner's own unit tests on every PR, +because the linter needs a Flatpak and a long build but the judgement around it +does not. + +## Related + +- [flatpak-ci.md](./flatpak-ci.md), the build this lints the output of +- [flathub-update-process.md](./flathub-update-process.md), where a lint result + sits in a release +- [flatpak-filesystem-audit.md](./flatpak-filesystem-audit.md), the permission + policy the manifest mode also checks diff --git a/docs/flathub-update-process.md b/docs/flathub-update-process.md index 5377dd83..5ab9e301 100644 --- a/docs/flathub-update-process.md +++ b/docs/flathub-update-process.md @@ -18,6 +18,8 @@ Related pages, each with a different job: debugging the Flatpak on your own machine. * [flatpak-ci.md](./flatpak-ci.md): what the automated Flatpak build proves on every packaging PR, and how to reproduce it locally. +* [flathub-builder-lint.md](./flathub-builder-lint.md): Flathub's own submission + linter, the exact commands, and the exceptions policy. * [flatpak-offline-build.md](./flatpak-offline-build.md): why the sandboxed build needs no network, and how that is audited. * [`flatpak/README.md`](../flatpak/README.md): what each packaging file is and diff --git a/flatpak/flathub-lint-exceptions.json b/flatpak/flathub-lint-exceptions.json new file mode 100644 index 00000000..20ee3a45 --- /dev/null +++ b/flatpak/flathub-lint-exceptions.json @@ -0,0 +1,29 @@ +{ + "_policy": [ + "Findings from Flathub's builder linter that Linthra accepts, each with a", + "written reason (#449). scripts/flathub_builder_lint.py reads this file.", + "", + "An entry here is not a way to make the check green. It is a claim that a", + "human looked at the finding and decided it is wrong or unavoidable for", + "Linthra, and the reason has to say which. A finding that is simply not", + "fixed yet belongs in its issue, not here.", + "", + "Keys are qualified by the linter mode they belong to, one of", + "\"manifest/\", \"repo/\" or \"appstream/\", for example", + "\"repo/appstream-screenshots-not-mirrored-in-ostree\". CI lints the", + "manifest before the build and the repo and catalogue after it, in separate", + "runs, so an unqualified reason could not be told from one whose mode simply", + "did not run that time.", + "", + "The file is also checked in the other direction: an exception the linter no", + "longer reports fails the run, so a reason can never outlive the problem it", + "explains. That check covers only the modes a given run linted.", + "", + "A \"-lint-failed\" finding cannot be excepted at all. It means the", + "linter itself failed, and it carries the same name whatever the failure was,", + "so a reason for it would go on matching a different problem later.", + "", + "Empty is the goal, and the state #456 requires before submission." + ], + "exceptions": {} +} diff --git a/linux/packaging/io.github.thezupzup.linthra.metainfo.xml b/linux/packaging/io.github.thezupzup.linthra.metainfo.xml index 34303959..222c116f 100644 --- a/linux/packaging/io.github.thezupzup.linthra.metainfo.xml +++ b/linux/packaging/io.github.thezupzup.linthra.metainfo.xml @@ -28,6 +28,15 @@ (#452). Newest entry first; a pre-release carries type="development" so a software centre does not offer an alpha as the current stable build. + The is held to Flathub's quality guidelines, which are stricter + than AppStream's: at most 35 characters, no trailing period, no leading + article, and it may not repeat the app's own name. "Play your own music from + this device" was 36, one character over, and the kind of finding that only + turns up when a submission is already open. test/tooling/ + flathub_metadata_guardrails_test.dart holds the rules that can be checked + without building anything; scripts/flathub_builder_lint.py runs the real + linter (#449). + Validate after editing: appstreamcli validate linux/packaging/io.github.thezupzup.linthra.metainfo.xml python3 scripts/check_release_metadata_sync.py @@ -37,7 +46,7 @@ CC0-1.0 AGPL-3.0-or-later Linthra - Play your own music from this device + Play your own music collection

Linthra is an open-source music player for people who keep their music diff --git a/scripts/flathub_builder_lint.py b/scripts/flathub_builder_lint.py new file mode 100644 index 00000000..fb8588a7 --- /dev/null +++ b/scripts/flathub_builder_lint.py @@ -0,0 +1,394 @@ +#!/usr/bin/env python3 +"""Run Flathub's own submission linter against Linthra's packaging (#449). + +`appstreamcli validate` and `desktop-file-validate`, which CI already runs, are +not this. They check that two files are well-formed. Flathub's linter checks +whether a *submission* would be accepted: whether the sources are reproducible, +whether the permissions are ones Flathub grants, whether the metainfo carries +what a software centre needs to show the app at all. + +This script is the repeatable way to run it and the honest way to read the +result. In particular it refuses two comfortable outcomes: + + * A missing linter is not a pass. If `org.flatpak.Builder` is not installed + the script exits 2 and says so, rather than reporting a clean run of + nothing. + * A finding is only tolerated when it has a written reason in the exceptions + file, and an exception that the linter no longer reports is itself a + failure, so the file cannot quietly accumulate reasons for problems that + were fixed years ago. + +Usage: + python3 scripts/flathub_builder_lint.py --manifest flatpak/.yml + python3 scripts/flathub_builder_lint.py --repo flatpak/repo-ci + python3 scripts/flathub_builder_lint.py --builddir flatpak/flatpak-builder-ci + +Exit codes: + 0 every finding is a documented exception, and every documented exception + was reported + 1 an undocumented finding, or a stale exception + 2 the linter could not be run at all + +Not every mode returns JSON: `appstream` delegates to appstreamcli and prints +its verdict as text. There the exit code is the verdict, and the text is +printed either way. +""" + +from __future__ import annotations + +import argparse +import json +import shutil +import subprocess +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +DEFAULT_EXCEPTIONS = REPO_ROOT / "flatpak" / "flathub-lint-exceptions.json" + +# The linter modes this script drives. Exception keys are qualified with one of +# these, so a reason states which run it belongs to. +MODES = ("manifest", "repo", "appstream") + +#: The Flatpak that ships flatpak-builder-lint. Flathub's own documentation +#: runs the linter this way, so this is the tool a reviewer will use. +LINTER_APP = "org.flatpak.Builder" +LINTER_COMMAND = "flatpak-builder-lint" + +#: Where the exported AppStream catalogue lands inside a flatpak-builder build +#: directory. Linting the source metainfo is not the same check: this is the +#: file a software centre actually reads, after appstreamcli compose has run. +APPSTREAM_IN_BUILDDIR = "files/share/app-info/xmls/{app_id}.xml.gz" + +APP_ID = "io.github.thezupzup.linthra" + + +class LinterUnavailable(RuntimeError): + """The linter could not be run, so nothing was checked.""" + + +def linter_versions() -> dict[str, str]: + """Version facts worth printing beside a result. + + The linter's rules change with the tool, so "clean" is only meaningful + alongside the version that said so. + """ + versions: dict[str, str] = {} + for label, command in ( + ("flatpak", ["flatpak", "--version"]), + ("org.flatpak.Builder", ["flatpak", "info", "--show-ref", LINTER_APP]), + ( + "org.flatpak.Builder commit", + ["flatpak", "info", "--show-commit", LINTER_APP], + ), + ( + LINTER_COMMAND, + [ + "flatpak", + "run", + f"--command={LINTER_COMMAND}", + LINTER_APP, + "--version", + ], + ), + ): + try: + result = subprocess.run( + command, capture_output=True, text=True, check=False + ) + except OSError as error: # flatpak itself is missing + raise LinterUnavailable( + "flatpak is not installed: {}".format(error) + ) from error + if result.returncode == 0: + versions[label] = result.stdout.strip() + return versions + + +def run_linter(mode: str, target: Path) -> dict: + """Run one linter mode and return its parsed report. + + The linter exits non-zero when it finds something, which is not an error + here: the findings are the output. Anything that stops it from producing a + report at all is. + """ + command = [ + "flatpak", + "run", + f"--command={LINTER_COMMAND}", + LINTER_APP, + mode, + str(target), + ] + print("$ {}".format(" ".join(command))) + try: + result = subprocess.run(command, capture_output=True, text=True, check=False) + except OSError as error: + raise LinterUnavailable("could not run the linter: {}".format(error)) from error + + if result.stderr.strip(): + print(result.stderr.strip(), file=sys.stderr) + + output = result.stdout.strip() + if not output: + # flatpak-builder-lint 3.x prints nothing at all when a mode finds + # nothing, and exits 0. That is a clean report, not a missing tool, + # and it cannot be confused with one here, because main() has already + # established that org.flatpak.Builder is installed before any mode + # runs. A *non-zero* exit with no output is still unexplained, and is + # still refused. + if result.returncode == 0: + return {} + raise LinterUnavailable( + "the linter produced no report for {} {} and exited {}. Nothing " + "explains that, so it cannot be read as a clean run.".format( + mode, target, result.returncode + ) + ) + try: + report = json.loads(output) + except json.JSONDecodeError: + # Not every mode speaks JSON. `appstream` hands the catalogue to + # appstreamcli and prints its human-readable verdict + # ("Validation was successful."), which the first version of this + # script refused as unreadable output and turned into a hard failure. + # + # For those, the exit code is the verdict and the text is the detail: + # both are surfaced, so a failure still has to be looked at and cannot + # pass as clean. + if result.returncode == 0: + return {"message": output} + # The linter itself failed rather than reporting a submission finding, + # and every such failure in this mode collapses to one name. That name + # must never be exceptable: a written reason attached to it would go on + # matching after the original problem was fixed and a different one + # appeared, which is exactly the guarantee the exceptions file makes. + return { + "errors": ["{}-lint-failed".format(mode)], + "message": output, + } + if not isinstance(report, dict): + raise LinterUnavailable( + "the linter's report for {} {} was not an object.".format(mode, target) + ) + # A non-zero exit has to be explained by something this script will treat as + # a finding. A structured report that carries neither errors nor warnings + # alongside a failure exit would otherwise read as clean and print PASS, + # which is the one outcome a non-zero linter run must never produce. The + # text and empty-output paths above already refuse this; so does JSON. + if result.returncode != 0 and not (report.get("errors") or report.get("warnings")): + report = dict(report) + report["errors"] = ["{}-lint-failed".format(mode)] + return report + + +def findings(report: dict) -> dict[str, list[str]]: + """Errors and warnings from a report, both treated as findings. + + Flathub's reviewers read the warnings too, and #449 is explicitly about + reaching a submission-quality result rather than an exit code. + """ + return { + "errors": sorted(str(item) for item in report.get("errors", []) or []), + "warnings": sorted(str(item) for item in report.get("warnings", []) or []), + } + + +def load_exceptions(path: Path) -> dict[str, str]: + """The documented exceptions, as {"mode/finding": reason}. + + A missing file means no exceptions, which is the state #449 wants to end + in. + + Keys are qualified by the linter mode they belong to, because CI does not + run every mode in one invocation: the manifest is linted before the build + and the repo and catalogue after it. An unqualified key could not be told + apart from one whose mode simply did not run this time, so the staleness + check would have to either miss real stale entries or reject live ones. + """ + if not path.exists(): + return {} + data = json.loads(path.read_text(encoding="utf-8")) + raw = data.get("exceptions", {}) + if not isinstance(raw, dict): + raise ValueError("{}: `exceptions` must be an object.".format(path)) + exceptions: dict[str, str] = {} + for finding, reason in raw.items(): + key = str(finding) + mode, sep, name = key.partition("/") + if not sep or mode not in MODES or not name: + raise ValueError( + "{}: exception {!r} must be qualified with the linter mode it " + "belongs to, as one of {}. CI lints the manifest and the built " + "repo in separate runs, so an unqualified reason cannot be " + "told from one whose mode did not run.".format( + path, key, "/, ".join(sorted(MODES)) + "/" + ) + ) + if name.endswith("-lint-failed"): + raise ValueError( + "{}: {!r} cannot be excepted. That name means the linter itself " + "failed, and it is the same name whatever the failure was, so a " + "reason for it would outlive the problem it explains.".format(path, key) + ) + text = reason if isinstance(reason, str) else reason.get("reason", "") + if not text.strip(): + raise ValueError( + "{}: exception {!r} has no written reason. An exception without " + "one is a suppression.".format(path, key) + ) + exceptions[key] = text.strip() + return exceptions + + +def report_mode(mode: str, target: Path, exceptions: dict[str, str]) -> tuple[int, set]: + """Print one mode's result. Returns (undocumented count, seen keys). + + Findings are looked up as "mode/finding", so a reason written for the repo + run cannot silently accept a same-named finding from the manifest run. + """ + report = run_linter(mode, target) + found = findings(report) + seen: set[str] = set() + undocumented = 0 + + print("\n--- {} {} ---".format(mode, target)) + for kind in ("errors", "warnings"): + for finding in found[kind]: + key = "{}/{}".format(mode, finding) + seen.add(key) + # load_exceptions refuses to record one of these, so this is belt + # and braces: a linter failure is never accepted, whatever the file + # says. + if key in exceptions and not finding.endswith("-lint-failed"): + print(" {} (accepted): {}".format(finding, kind[:-1])) + print(" reason: {}".format(exceptions[key])) + else: + undocumented += 1 + print(" {} ({})".format(finding, kind[:-1])) + if not found["errors"] and not found["warnings"]: + print(" no findings") + + # Anything else the linter said, verbatim, so a message that is not an + # error or a warning is not lost. + for key, value in sorted(report.items()): + if key in {"errors", "warnings"}: + continue + print(" {}: {}".format(key, value)) + return undocumented, seen + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--manifest", type=Path, help="manifest to lint") + parser.add_argument("--repo", type=Path, help="exported OSTree repo to lint") + parser.add_argument( + "--builddir", + type=Path, + help="flatpak-builder build directory, whose exported AppStream " + "catalogue is linted", + ) + parser.add_argument( + "--exceptions", + type=Path, + default=DEFAULT_EXCEPTIONS, + help="documented exceptions (default: {})".format( + DEFAULT_EXCEPTIONS.relative_to(REPO_ROOT) + ), + ) + args = parser.parse_args(argv) + + targets: list[tuple[str, Path]] = [] + if args.manifest: + targets.append(("manifest", args.manifest)) + if args.repo: + targets.append(("repo", args.repo)) + if args.builddir: + targets.append( + ( + "appstream", + args.builddir / APPSTREAM_IN_BUILDDIR.format(app_id=APP_ID), + ) + ) + if not targets: + parser.error("pass at least one of --manifest, --repo or --builddir") + + for mode, target in targets: + if not target.exists(): + print("ERROR: {} does not exist ({})".format(target, mode), file=sys.stderr) + return 2 + + if shutil.which("flatpak") is None: + print( + "ERROR: flatpak is not installed, so nothing was linted. This is " + "not a pass.", + file=sys.stderr, + ) + return 2 + + try: + exceptions = load_exceptions(args.exceptions) + except (ValueError, json.JSONDecodeError) as error: + print("ERROR: {}".format(error), file=sys.stderr) + return 1 + + try: + versions = linter_versions() + print("Flathub builder lint") + for label, value in versions.items(): + print(" {}: {}".format(label, value)) + if LINTER_APP not in versions: + raise LinterUnavailable( + "{} is not installed: `flatpak install -y flathub {}`.".format( + LINTER_APP, LINTER_APP + ) + ) + + undocumented = 0 + seen: set[str] = set() + for mode, target in targets: + mode_undocumented, mode_seen = report_mode(mode, target, exceptions) + undocumented += mode_undocumented + seen |= mode_seen + except LinterUnavailable as error: + print("\nERROR: {}".format(error), file=sys.stderr) + print("Nothing was linted, so this is not a pass.", file=sys.stderr) + return 2 + + print() + # Only the modes this run actually linted. CI lints the manifest before the + # build and the repo and catalogue after it, in separate invocations, so + # comparing every exception against one run's findings would report the + # other run's live exceptions as stale and fail the build. + ran = {mode for mode, _ in targets} + scoped = {key for key in exceptions if key.split("/", 1)[0] in ran} + stale = sorted(scoped - seen) + if stale: + print( + "FAIL: {} documented exception(s) were not reported by the linter. " + "Remove them rather than leaving a written reason for a problem " + "that no longer exists:".format(len(stale)) + ) + for finding in stale: + print(" {}".format(finding)) + if undocumented: + print( + "FAIL: {} finding(s) with no written reason. Fix them, or record " + "the reason in {}.".format(undocumented, args.exceptions.name) + ) + if stale or undocumented: + return 1 + + if scoped: + print( + "PASS: every finding has a documented reason ({} accepted).".format( + len(scoped) + ) + ) + else: + print("PASS: the linter reported nothing.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) diff --git a/test/tooling/flathub_builder_lint_test.py b/test/tooling/flathub_builder_lint_test.py new file mode 100644 index 00000000..1628eb6d --- /dev/null +++ b/test/tooling/flathub_builder_lint_test.py @@ -0,0 +1,373 @@ +#!/usr/bin/env python3 +"""Unit tests for scripts/flathub_builder_lint.py (#449). + + python3 test/tooling/flathub_builder_lint_test.py + +The script's value is entirely in what it refuses to call a pass, so that is +what these tests are about: a missing linter, a finding nobody wrote a reason +for, an exception with an empty reason, and an exception the linter no longer +reports. The linter itself is stubbed: it lives in a Flatpak, and the point +here is the judgement around it, not its rules. +""" + +from __future__ import annotations + +import contextlib +import importlib.util +import io +import json +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[2] +SCRIPTS = ROOT / "scripts" + + +def _load(name: str, filename: str): + spec = importlib.util.spec_from_file_location(name, SCRIPTS / filename) + module = importlib.util.module_from_spec(spec) + assert spec.loader is not None + spec.loader.exec_module(module) + return module + + +lint = _load("flathub_builder_lint", "flathub_builder_lint.py") + + +@contextlib.contextmanager +def stub_linter(reports): + """Replace the linter with canned reports, keyed by mode.""" + original = lint.run_linter + lint.run_linter = lambda mode, target: reports[mode] + try: + yield + finally: + lint.run_linter = original + + +@contextlib.contextmanager +def exceptions_file(payload): + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "flathub-lint-exceptions.json" + path.write_text(json.dumps(payload), encoding="utf-8") + yield path + + +def run_mode(mode, report, exceptions): + with stub_linter({mode: report}): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + undocumented, seen = lint.report_mode(mode, Path("x"), exceptions) + return undocumented, seen, buffer.getvalue() + + +class FindingsTest(unittest.TestCase): + def test_reads_errors_and_warnings(self) -> None: + found = lint.findings( + {"errors": ["b", "a"], "warnings": ["c"], "message": "hi"} + ) + self.assertEqual(found["errors"], ["a", "b"]) + self.assertEqual(found["warnings"], ["c"]) + + def test_tolerates_a_report_with_neither(self) -> None: + self.assertEqual(lint.findings({}), {"errors": [], "warnings": []}) + + def test_tolerates_explicit_nulls(self) -> None: + self.assertEqual( + lint.findings({"errors": None, "warnings": None}), + {"errors": [], "warnings": []}, + ) + + +class ExceptionsFileTest(unittest.TestCase): + def test_missing_file_means_no_exceptions(self) -> None: + self.assertEqual(lint.load_exceptions(Path("/nope/absent.json")), {}) + + def test_reads_written_reasons(self) -> None: + with exceptions_file({"exceptions": {"repo/a-finding": "because"}}) as path: + self.assertEqual(lint.load_exceptions(path), {"repo/a-finding": "because"}) + + def test_accepts_an_object_with_a_reason_field(self) -> None: + with exceptions_file( + {"exceptions": {"repo/a-finding": {"reason": "because", "issue": 449}}} + ) as path: + self.assertEqual(lint.load_exceptions(path), {"repo/a-finding": "because"}) + + # An exception without a reason is exactly the suppression #449 forbids. + def test_rejects_an_exception_with_no_reason(self) -> None: + with exceptions_file({"exceptions": {"repo/a-finding": " "}}) as path: + with self.assertRaises(ValueError): + lint.load_exceptions(path) + + # CI lints the manifest and the built repo in separate invocations, so an + # unqualified key cannot be told from one whose mode did not run. + def test_rejects_an_unqualified_key(self) -> None: + with exceptions_file({"exceptions": {"a-finding": "because"}}) as path: + with self.assertRaises(ValueError): + lint.load_exceptions(path) + + def test_rejects_an_unknown_mode(self) -> None: + with exceptions_file({"exceptions": {"nonsense/a-finding": "because"}}) as path: + with self.assertRaises(ValueError): + lint.load_exceptions(path) + + # "-lint-failed" is the same name whatever went wrong, so a reason + # for it would go on matching a different failure later. + def test_refuses_to_except_a_linter_failure(self) -> None: + for key in ("appstream/appstream-lint-failed", "repo/repo-lint-failed"): + with exceptions_file({"exceptions": {key: "known bad"}}) as path: + with self.assertRaises(ValueError): + lint.load_exceptions(path) + + def test_rejects_a_malformed_exceptions_block(self) -> None: + with exceptions_file({"exceptions": ["a-finding"]}) as path: + with self.assertRaises(ValueError): + lint.load_exceptions(path) + + def test_the_committed_file_has_no_exceptions_yet(self) -> None: + # #456 requires this to be empty at submission, so a PR that adds one + # should have to change this test and explain itself. + self.assertEqual(lint.load_exceptions(lint.DEFAULT_EXCEPTIONS), {}) + + +class ReportModeTest(unittest.TestCase): + def test_a_clean_report_has_nothing_undocumented(self) -> None: + undocumented, seen, output = run_mode( + "manifest", {"errors": [], "warnings": []}, {} + ) + self.assertEqual(undocumented, 0) + self.assertEqual(seen, set()) + self.assertIn("no findings", output) + + def test_warnings_count_as_findings(self) -> None: + undocumented, seen, _ = run_mode( + "manifest", {"warnings": ["appstream-missing-screenshots"]}, {} + ) + self.assertEqual(undocumented, 1) + self.assertEqual(seen, {"manifest/appstream-missing-screenshots"}) + + def test_a_documented_finding_is_accepted_and_quotes_its_reason(self) -> None: + undocumented, seen, output = run_mode( + "manifest", + {"errors": ["some-finding"]}, + {"manifest/some-finding": "the linter is wrong about X"}, + ) + self.assertEqual(undocumented, 0) + self.assertEqual(seen, {"manifest/some-finding"}) + self.assertIn("the linter is wrong about X", output) + + # A reason written for one mode must not accept a same-named finding from + # another, which is the whole point of qualifying the key. + def test_a_reason_for_another_mode_does_not_accept_it(self) -> None: + undocumented, seen, _ = run_mode( + "manifest", + {"errors": ["some-finding"]}, + {"repo/some-finding": "documented for the repo run only"}, + ) + self.assertEqual(undocumented, 1) + self.assertEqual(seen, {"manifest/some-finding"}) + + # Belt and braces: load_exceptions refuses to record one of these, so this + # covers a hand-built dict reaching report_mode anyway. + def test_a_linter_failure_is_never_accepted(self) -> None: + undocumented, _, _ = run_mode( + "appstream", + {"errors": ["appstream-lint-failed"]}, + {"appstream/appstream-lint-failed": "a reason that should not count"}, + ) + self.assertEqual(undocumented, 1) + + # A message the linter emits outside errors/warnings still has to reach the + # log, or a reviewer reads a summary that quietly dropped it. + def test_other_report_keys_are_printed(self) -> None: + _, _, output = run_mode( + "repo", {"errors": [], "warnings": [], "message": "checked 1 ref"}, {} + ) + self.assertIn("checked 1 ref", output) + + +class RunLinterTest(unittest.TestCase): + """How the linter's own output is read. + + flatpak-builder-lint 3.x prints nothing when a mode finds nothing. The + first version of this script read that as "the tool is missing" and turned + a clean manifest into a hard failure, which is why the distinction below is + a test rather than a comment. + """ + + @contextlib.contextmanager + def _subprocess(self, stdout, returncode, stderr=""): + original = lint.subprocess.run + + class Result: + pass + + result = Result() + result.stdout, result.stderr, result.returncode = stdout, stderr, returncode + lint.subprocess.run = lambda *a, **k: result + try: + yield + finally: + lint.subprocess.run = original + + def test_no_output_and_exit_zero_is_a_clean_report(self) -> None: + with self._subprocess("", 0): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + self.assertEqual(lint.run_linter("manifest", Path("x")), {}) + + # Silence with a failure exit explains nothing, so it cannot be read as + # either a pass or a finding. + def test_no_output_and_a_failure_exit_is_refused(self) -> None: + with self._subprocess("", 2): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + with self.assertRaises(lint.LinterUnavailable): + lint.run_linter("manifest", Path("x")) + + def test_findings_are_parsed(self) -> None: + with self._subprocess('{"errors": ["a"]}', 1): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + self.assertEqual( + lint.run_linter("manifest", Path("x")), {"errors": ["a"]} + ) + + # The `appstream` mode delegates to appstreamcli and prints its verdict as + # text. Refusing that as unreadable output turned a successful validation + # into a hard failure. + def test_text_output_and_exit_zero_is_clean_and_still_printed(self) -> None: + with self._subprocess("Validation was successful.", 0): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + report = lint.run_linter("appstream", Path("x")) + self.assertEqual(lint.findings(report), {"errors": [], "warnings": []}) + self.assertIn("Validation was successful.", report["message"]) + + # A structured report with neither errors nor warnings alongside a failure + # exit would otherwise read as clean and print PASS, which is the one thing + # a non-zero linter run must never produce. + def test_json_output_and_an_unexplained_failure_exit_is_a_finding( + self, + ) -> None: + with self._subprocess('{"message": "internal failure"}', 1): + report = lint.run_linter("repo", Path("x")) + self.assertEqual(report["errors"], ["repo-lint-failed"]) + self.assertEqual(report["message"], "internal failure") + + # ...but a report that does explain itself keeps its own findings. + def test_json_findings_explain_a_failure_exit_on_their_own(self) -> None: + with self._subprocess('{"errors": ["a-real-finding"]}', 1): + report = lint.run_linter("repo", Path("x")) + self.assertEqual(report["errors"], ["a-real-finding"]) + + def test_text_output_and_a_failure_exit_is_a_finding(self) -> None: + with self._subprocess("E: something is wrong", 1): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + report = lint.run_linter("appstream", Path("x")) + self.assertEqual(lint.findings(report)["errors"], ["appstream-lint-failed"]) + self.assertIn("something is wrong", report["message"]) + + +class MainTest(unittest.TestCase): + def setUp(self) -> None: + self.manifest = ROOT / "flatpak" / "io.github.thezupzup.linthra.yml" + + def _main(self, argv): + buffer = io.StringIO() + errors = io.StringIO() + with contextlib.redirect_stdout(buffer), contextlib.redirect_stderr(errors): + code = lint.main(argv) + return code, buffer.getvalue() + errors.getvalue() + + def test_a_missing_target_is_not_a_pass(self) -> None: + code, output = self._main(["--manifest", "/nope/absent.yml"]) + self.assertEqual(code, 2) + self.assertIn("does not exist", output) + + # The failure mode this script exists to prevent: a CI runner without the + # linter reporting a clean run of nothing. + def test_a_missing_linter_is_not_a_pass(self) -> None: + original = lint.shutil.which + lint.shutil.which = lambda _: None + try: + code, output = self._main(["--manifest", str(self.manifest)]) + finally: + lint.shutil.which = original + self.assertEqual(code, 2) + self.assertIn("not a pass", output) + + def test_an_undocumented_finding_fails(self) -> None: + with stub_linter({"manifest": {"errors": ["a-finding"], "warnings": []}}): + original_which, original_versions = lint.shutil.which, lint.linter_versions + lint.shutil.which = lambda _: "/usr/bin/flatpak" + lint.linter_versions = lambda: {lint.LINTER_APP: "test"} + try: + code, output = self._main(["--manifest", str(self.manifest)]) + finally: + lint.shutil.which = original_which + lint.linter_versions = original_versions + self.assertEqual(code, 1) + self.assertIn("no written reason", output) + + # A reason that outlives the problem it explains is how an exceptions file + # rots into a suppression list. + def test_a_stale_exception_fails(self) -> None: + with exceptions_file( + {"exceptions": {"manifest/fixed-long-ago": "it used to do this"}} + ) as path: + with stub_linter({"manifest": {"errors": [], "warnings": []}}): + original_which = lint.shutil.which + original_versions = lint.linter_versions + lint.shutil.which = lambda _: "/usr/bin/flatpak" + lint.linter_versions = lambda: {lint.LINTER_APP: "test"} + try: + code, output = self._main( + [ + "--manifest", + str(self.manifest), + "--exceptions", + str(path), + ] + ) + finally: + lint.shutil.which = original_which + lint.linter_versions = original_versions + self.assertEqual(code, 1) + self.assertIn("were not reported", output) + + # The failure Codex caught on #614. CI lints the manifest before the build + # and the repo and catalogue after it, in two invocations, each loading the + # whole file. Comparing every exception against one run's findings reported + # the other run's live exceptions as stale, which made the mechanism + # unusable the moment anyone recorded a real one. + def test_an_exception_for_a_mode_that_did_not_run_is_not_stale(self) -> None: + with exceptions_file( + {"exceptions": {"repo/some-repo-finding": "documented, and still true"}} + ) as path: + with stub_linter({"manifest": {"errors": [], "warnings": []}}): + original_which = lint.shutil.which + original_versions = lint.linter_versions + lint.shutil.which = lambda _: "/usr/bin/flatpak" + lint.linter_versions = lambda: {lint.LINTER_APP: "test"} + try: + code, output = self._main( + [ + "--manifest", + str(self.manifest), + "--exceptions", + str(path), + ] + ) + finally: + lint.shutil.which = original_which + lint.linter_versions = original_versions + self.assertEqual(code, 0, output) + self.assertNotIn("were not reported", output) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/test/tooling/flathub_metadata_guardrails_test.dart b/test/tooling/flathub_metadata_guardrails_test.dart new file mode 100644 index 00000000..eaa24311 --- /dev/null +++ b/test/tooling/flathub_metadata_guardrails_test.dart @@ -0,0 +1,268 @@ +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; + +/// The Flathub metadata rules that can be checked without building anything +/// (#449). +/// +/// `flatpak-builder-lint` is the authority, and CI runs it, but it needs a +/// Flatpak, a 90-minute build for two of its three modes, and a runner with +/// `org.flatpak.Builder` installed. These are the same rules from Flathub's +/// quality guidelines, checked in the second it takes to read a file, so a +/// regression is caught on the PR that causes it rather than in a submission +/// review. +/// +/// This is deliberately not a reimplementation of the linter. It covers the +/// handful of rules that are stable, textual, and cheap; everything else is the +/// linter's job. +void main() { + late String metainfo; + late String desktop; + + /// The tag's value as a software centre would render it. + /// + /// XML entities are decoded first: `&` is one character on screen, and + /// counting its five-character spelling would fail a valid summary that sits + /// near Flathub's limit. + String? tagValue(String tag) { + final RegExpMatch? match = + RegExp('<$tag>(.*?)', dotAll: true).firstMatch(metainfo); + final String? raw = match?.group(1)?.trim(); + if (raw == null) { + return null; + } + // One pass, no chaining. Decoding numeric references and then named ones + // decodes twice: `&lt;` renders as the literal text `<`, but a + // numeric pass turns it into `<` and a named pass then turns that into + // `<`, counting 9 characters as 6. Undercounting is the dangerous + // direction, because it lets an overlong summary through. + return raw.replaceAllMapped( + RegExp(r'&(?:#(x[0-9a-fA-F]+|[0-9]+)|(lt|gt|quot|apos|amp));'), + (Match match) { + final String? digits = match.group(1); + if (digits != null) { + final int? code = digits.startsWith('x') + ? int.tryParse(digits.substring(1), radix: 16) + : int.tryParse(digits); + // Leave anything unparseable or outside Unicode exactly as written, + // rather than throwing inside a guardrail. + if (code == null || code < 0 || code > 0x10FFFF) { + return match.group(0)!; + } + return String.fromCharCode(code); + } + return const { + 'lt': '<', + 'gt': '>', + 'quot': '"', + 'apos': "'", + 'amp': '&', + }[match.group(2)!]!; + }, + ); + } + + /// Characters as a reader counts them. + /// + /// Dart's `String.length` is UTF-16 code units, so anything outside the BMP + /// (an emoji, say) counts twice against a limit that is really about how much + /// text fits on one line. Runes are code points, which fixes that without + /// pulling `package:characters` in as a direct dependency just for a length. + /// + /// Not grapheme clusters: a combining accent still counts separately. That + /// is a smaller error than the one this replaces, and the summary is a short + /// line of Latin text rather than somewhere clusters are likely to matter. + int displayLength(String value) => value.runes.length; + + setUpAll(() { + // Comments stripped first: the file's header explains the rules below and + // names the very tags it is explaining, so a naive match for `

` + // starts inside the prose and runs to the real closing tag. + metainfo = File( + 'linux/packaging/io.github.thezupzup.linthra.metainfo.xml', + ).readAsStringSync().replaceAll( + RegExp(r'', dotAll: true), + '', + ); + desktop = File( + 'linux/packaging/io.github.thezupzup.linthra.desktop', + ).readAsStringSync(); + }); + + group('summary', () { + // Flathub's limit, not AppStream's (which allows about 90). A software + // centre renders the summary on one line beside the name. + test('is at most 35 characters', () { + final String? summary = tagValue('summary'); + expect(summary, isNotNull); + expect( + displayLength(summary!), + lessThanOrEqualTo(35), + reason: 'Flathub caps the summary at 35 characters; "$summary" is ' + '${displayLength(summary)}', + ); + }); + + test('does not end in a period', () { + expect(tagValue('summary'), isNot(endsWith('.'))); + }); + + test('does not start with an article', () { + final String summary = tagValue('summary')!.toLowerCase(); + for (final String article in ['a ', 'an ', 'the ']) { + expect( + summary.startsWith(article), + isFalse, + reason: 'the summary must not begin with "$article"', + ); + } + }); + + // "Linthra, a music player" wastes the line: the name is already shown + // right next to it. + test('does not repeat the app name', () { + expect(tagValue('summary')!.toLowerCase(), isNot(contains('linthra'))); + }); + }); + + group('name', () { + test('is at most 20 characters', () { + final String? name = tagValue('name'); + expect(name, isNotNull); + expect(displayLength(name!), lessThanOrEqualTo(20)); + }); + + test('does not end in a period', () { + expect(tagValue('name'), isNot(endsWith('.'))); + }); + }); + + group('required components', () { + // Each of these is a hard requirement for a Flathub listing, and each has + // a different consequence when missing: no id, no listing; no licence, + // no build; no rating, no age gate; no launchable, an app a software + // centre will not offer to open. + test('the metainfo carries what a listing needs', () { + for (final String required in [ + 'io.github.thezupzup.linthra', + '', + '', + 'io.github.thezupzup.linthra.desktop' + '', + '', + '', + ' releases = + RegExp(r']*>').allMatches(metainfo); + expect(releases, isNotEmpty, reason: 'a listing needs a release history'); + for (final RegExpMatch release in releases) { + final String tag = release.group(0)!; + expect(tag, contains('version="'), reason: '$tag has no version'); + expect(tag, contains('date="'), reason: '$tag has no date'); + } + }); + }); + + group('desktop entry', () { + test('is not a terminal application and has a startup class', () { + expect(desktop, contains('Terminal=false')); + expect( + desktop, + contains('StartupWMClass=io.github.thezupzup.linthra'), + ); + }); + + // The icon has to be an icon-theme name equal to the app id, or the + // exported icon does not resolve inside the sandbox. + test('names the app id as its icon', () { + expect(desktop, contains('Icon=io.github.thezupzup.linthra')); + }); + + // A `.desktop` file NoDisplay=true would hide the app from the launcher + // that Flathub exists to put it in. + test('is visible in a launcher', () { + expect(desktop, isNot(contains('NoDisplay=true'))); + expect(desktop, isNot(contains('Hidden=true'))); + }); + }); + + group('the lint runner is wired up and unsuppressed', () { + test('CI installs the linter and gates the manifest on it', () { + final String workflow = + File('.github/workflows/flatpak-build.yml').readAsStringSync(); + expect(workflow, + contains('flatpak install --user -y flathub org.flatpak.Builder')); + expect(workflow, + contains('--manifest flatpak/io.github.thezupzup.linthra.yml')); + }); + + // The appstream mode is clean today, so it gates. Deferring it with the + // repo mode would have left a future regression in the generated + // catalogue free to land, for no benefit. + test('CI gates on the AppStream catalogue the build produced', () { + final String workflow = + File('.github/workflows/flatpak-build.yml').readAsStringSync(); + expect(workflow, contains('--builddir flatpak/flatpak-builder-ci')); + }); + + // Only the repo mode waits, and only because it reports real submission + // blockers fixed by taking screenshots (#437) rather than by anything in + // the tooling. Wiring it in now would mean a permanently red job or an + // exception recording "not done yet". It arrives with the screenshots + // (#628), and this test is what makes that a decision rather than an + // oversight: turning it on has to delete this. + test('the repo mode waits for the screenshots that let it pass', () { + final String workflow = + File('.github/workflows/flatpak-build.yml').readAsStringSync(); + expect( + workflow, + isNot(contains('--repo flatpak/repo-ci')), + reason: 'wiring the repo mode in needs #628, and this test with it', + ); + expect(workflow, contains('#628'), + reason: 'the workflow must say where the missing gate went'); + }); + + // A failed step ends the job, so a lint ahead of the launch smoke would + // stop the package being installed and launched at all. Order matters, so + // it is asserted rather than left to a comment. + test('the launch smoke runs before the catalogue lint', () { + final String workflow = + File('.github/workflows/flatpak-build.yml').readAsStringSync(); + final int launch = + workflow.indexOf('- name: Install and launch packaged Flatpak'); + final int lint = workflow.indexOf('- name: Lint the AppStream catalogue'); + expect(launch, isNonNegative); + expect(lint, isNonNegative); + expect( + launch, + lessThan(lint), + reason: 'a lint finding must not stop the launch smoke from running', + ); + }); + + // #456 requires an empty exceptions file. Adding the first entry should + // have to change this test, in a diff a reviewer reads. + test('no linter finding is currently excepted', () { + final String exceptions = + File('flatpak/flathub-lint-exceptions.json').readAsStringSync(); + expect( + exceptions, + contains('"exceptions": {}'), + reason: 'an entry here needs a written reason and a reviewer, not a ' + 'green check', + ); + }); + }); +}