diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 93e171e..55980a9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -149,6 +149,25 @@ jobs: echo "javadoc strictness confirmed: the poisoned build failed with 'error: reference not found'" + # The canary above only poisons rift-java-core, so it pins the ROOT pluginManagement. A + # per-module false in some other module leaves it — and the main + # release build — green while that module ships broken javadoc to Central (#203). This sweeps + # the effective pom of EVERY module in the release reactor for the known silencers. Same flags + # as the main step, so the reactor inspected is the one that actually publishes. + - name: Javadoc strictness across all modules (effective-pom sweep) + run: | + set -euo pipefail + EFF=$(mktemp) + trap 'rm -f "$EFF"' EXIT + # Prove the detector still detects before trusting a clean sweep: a checker edit that + # stopped flagging anything would otherwise report "confirmed" forever. + python3 scripts/check-javadoc-strictness.py --self-test + ./mvnw -B -ntp -Prelease,natives-bundle -DdualEmbedded help:effective-pom -Doutput="$EFF" + # --require names the modules this JDK-21 + -DdualEmbedded reactor must contain, so a + # module silently dropping out of the reactor fails the sweep instead of shrinking it. + python3 scripts/check-javadoc-strictness.py "$EFF" --require \ + rift-java-parent,rift-java-core,rift-java-jackson,rift-java-junit5,rift-java-natives,rift-java-spring,rift-java-testcontainers,rift-java-conformance,rift-java-bom,rift-java-embedded,rift-java-embedded-jdk21 + # The Docker-enabled lane for rift-java-testcontainers: RIFT_IT=1 un-gates the RiftContainer # round-trip ITs (they self-skip everywhere else). ubuntu-latest runners ship a Docker daemon. testcontainers-it: diff --git a/.gitignore b/.gitignore index d87c558..f03706e 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,7 @@ native-local/ # MkDocs build output site/ + +# Bytecode from scripts/*.py (the javadoc-strictness sweep) +__pycache__/ +*.pyc diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d0a3ba5..99c8dc1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,13 +46,38 @@ both; release-smoke covers them on every PR. Missing-tag warnings (`no @param`, `no @return`) are *not* errors and do not fail the build. -That strictness is itself guarded. A clean build cannot distinguish "strict and clean" from -"permissive and clean", so release-smoke runs a canary step that injects a broken `{@link}` into -`Rift.java` and fails the job if the release-lane build *succeeds* — re-adding `failOnError=false`, -`doclint=none`, or any other silencer cannot slip through unnoticed. The step reverts its own edit. -One maintenance note: it anchors on the phrase `admin API.` in `Rift.java`'s opening javadoc -sentence, so if you reword that sentence, update the anchor in `.github/workflows/ci.yml`. The step -fails loudly and says so when the anchor stops matching. +That strictness is itself guarded, by two complementary release-smoke steps. A clean build cannot +distinguish "strict and clean" from "permissive and clean", so neither step trusts a green build. + +**The canary** injects a broken `{@link}` into `Rift.java` and fails the job if the release-lane +build *succeeds*. It proves the *behaviour* — that a bad reference really is fatal — and reverts its +own edit. One maintenance note: it anchors on the phrase `admin API.` in `Rift.java`'s opening +javadoc sentence, so if you reword that sentence, update the anchor in `.github/workflows/ci.yml`. +The step fails loudly and says so when the anchor stops matching. + +**The effective-pom sweep** (`scripts/check-javadoc-strictness.py`) covers what the canary cannot: +the canary poisons only `rift-java-core`, so it pins the root `pluginManagement`, and a per-module +override elsewhere would leave it green. The sweep runs `help:effective-pom` over the release +reactor and rejects any module whose *effective* javadoc config silences errors — `failOnError`, +`skip`, `skippedModules`, a `doclint` value other than `all`, an `-Xdoclint` option that disables a +group, or the equivalent `maven.javadoc.*` / `doclint` **properties**, which need no `` +at all. Run it locally with: + +```sh +# -DdualEmbedded matches CI, so the sweep covers rift-java-embedded too +./mvnw -Prelease,natives-bundle -DdualEmbedded help:effective-pom -Doutput=/tmp/eff.xml +python3 scripts/check-javadoc-strictness.py /tmp/eff.xml +python3 scripts/check-javadoc-strictness.py --self-test # asserts the detector still detects +``` + +The module set depends on your JDK (`rift-java-embedded-jdk21` only joins on JDK 21), so a local run +sweeps fewer modules than CI — pass `--require` only if you know which set to expect. + +Two things it deliberately does *not* do: it reads configuration, not behaviour (that is the +canary's job), and it cannot see a `-Dmaven.javadoc.failOnError=false` added to a workflow's own +`mvn` command line. Its `--require` list in `ci.yml` names the modules the reactor must contain, so +a module dropping out of the reactor fails the sweep instead of silently shrinking it — add new +published modules there. ## Module layout diff --git a/pom.xml b/pom.xml index fad79a2..3dbb7c0 100644 --- a/pom.xml +++ b/pom.xml @@ -70,6 +70,7 @@ 3.5.0 1.6.0 3.8.0 + 3.5.2 3.6.0 @@ -137,6 +138,14 @@ maven-javadoc-plugin ${maven-javadoc-plugin.version} + + + org.apache.maven.plugins + maven-help-plugin + ${maven-help-plugin.version} + diff --git a/scripts/check-javadoc-strictness.py b/scripts/check-javadoc-strictness.py new file mode 100755 index 0000000..b7ddf22 --- /dev/null +++ b/scripts/check-javadoc-strictness.py @@ -0,0 +1,378 @@ +#!/usr/bin/env python3 +# +# Fail if any module's EFFECTIVE maven-javadoc-plugin configuration silences javadoc errors. +# +# ./mvnw -Prelease,... help:effective-pom -Doutput=effective-pom.xml +# scripts/check-javadoc-strictness.py effective-pom.xml --require rift-java-core,rift-java-bom,... +# scripts/check-javadoc-strictness.py --self-test +# +# Why this exists (#203): the poison canary in .github/workflows/ci.yml proves *behaviourally* that +# a broken {@link} fails the build, but it only poisons rift-java-core, so it pins the ROOT pom's +# pluginManagement. A per-module override in, say, rift-java-jackson leaves both that canary and the +# main release build green while that module ships broken javadoc to Central — the same +# self-concealing shape #200 removed, relocated one level down. This sweeps every module instead. +# +# Reading the EFFECTIVE pom (not the source poms) is the point: it is the model Maven will actually +# execute, with inheritance, profiles and per-module overrides already merged, so the check cannot be +# defeated by whitespace, a property indirection, or the setting living in a different file. +# +# Every rejection below was verified behaviourally: each one makes a build with a deliberately broken +# {@link} in rift-java-jackson exit 0 instead of failing. +# +# `doclint` and `-Xdoclint` are matched as an ALLOWLIST, not a denylist. Their value space is open +# (`none`, `syntax`, `all,-missing,-reference`, …) and only some values disable the `reference` group +# that produces `error: reference not found`, so enumerating the bad ones is a losing game. The +# repo's intended state is "not configured at all", so anything other than `all` is rejected and +# widening the allowlist is a conscious edit. +# +# Scope, stated honestly: this asserts CONFIGURATION reachable from the POM. It cannot see a +# `-Dmaven.javadoc.failOnError=false` added to a workflow's own mvn command line. The rift-java-core +# poison canary asserts BEHAVIOUR. Each covers the other's blind spot — keep both. + +import argparse +import contextlib +import io +import os +import sys +import tempfile +import xml.etree.ElementTree as ET + +NS = "{http://maven.apache.org/POM/4.0.0}" +JAVADOC_PLUGIN = "maven-javadoc-plugin" + +# Properties are a first-class silencer: maven-javadoc-plugin binds these parameters to user +# properties, so a module that sets one in needs no at all. +# +# failOnError is an allowlist ("true" is the only strict value) rather than a `!= "false"` denylist: +# Maven parses these with Boolean.valueOf, so an EMPTY value is false — i.e. silenced — and a +# denylist would wave it through. +STRICTNESS_PROPERTIES = { + "maven.javadoc.failOnError": lambda v: v.lower() == "true", + "maven.javadoc.skip": lambda v: v.lower() != "true", + "maven.javadoc.skippedModules": lambda v: v.strip() == "", + "doclint": lambda v: _doclint_is_strict(v), +} + +# Both the plural container and the singular scalar parameter pass options through verbatim. +ADDITIONAL_OPTION_TAGS = ( + "additionalJOptions", + "additionalJOption", + "additionalparam", + "additionalOptions", +) + + +def _doclint_is_strict(value): + """Only an unset or fully-enabled doclint keeps `error: reference not found` fatal.""" + return value.strip().lower() in ("", "all") + + +def _joptions_are_strict(text): + """True unless some -Xdoclint token disables any doclint group.""" + for token in text.split(): + bare = token.strip().lower() + if bare.startswith("-xdoclint") and bare not in ("-xdoclint", "-xdoclint:all"): + return False + return True + + +def local(tag): + return tag[len(NS):] if tag.startswith(NS) else tag + + +def permissive_settings(config): + """Reasons this weakens javadoc strictness.""" + reasons = [] + for child in config: + name = local(child.tag) + value = (child.text or "").strip() + if name == "failOnError" and value.lower() != "true": + # Empty counts: Boolean.valueOf("") is false, so silences errors too. + reasons.append(f"{value or '(empty)'} (only 'true' is strict)") + elif name == "skip" and value.lower() == "true": + reasons.append("true") + elif name == "skippedModules" and value: + reasons.append(f"{value}") + elif name == "doclint" and not _doclint_is_strict(value): + reasons.append(f"{value} (only 'all' keeps reference errors fatal)") + elif name in ADDITIONAL_OPTION_TAGS: + # Options may be nested (…) or inline, so + # flatten the subtree rather than reading .text. + joined = " ".join(t.strip() for t in child.itertext() if t.strip()) + if not _joptions_are_strict(joined): + reasons.append(f"<{name}>{joined}") + return reasons + + +def javadoc_plugins(project): + """Javadoc plugin elements that actually govern the build. + + Scoped to and . is site-only and + any the effective pom retains are, by definition, not the active model — including + either would raise false alarms, and a guard that cries wolf is a guard that gets deleted. + """ + found = [] + build = project.find(NS + "build") + if build is None: + return found + containers = [build.find(NS + "plugins")] + management = build.find(NS + "pluginManagement") + if management is not None: + containers.append(management.find(NS + "plugins")) + for container in containers: + if container is None: + continue + for plugin in container.findall(NS + "plugin"): + if plugin.findtext(NS + "artifactId") == JAVADOC_PLUGIN: + found.append(plugin) + return found + + +def check_project(project): + """(artifactId, javadoc_plugin_count, [(artifactId, where, reason)]).""" + artifact_id = project.findtext(NS + "artifactId") or "" + findings = [] + plugins = javadoc_plugins(project) + + for plugin in plugins: + config = plugin.find(NS + "configuration") + if config is not None: + findings += [(artifact_id, "plugin configuration", r) for r in permissive_settings(config)] + + # An execution-level overrides the plugin-level one, so it silences errors + # just as effectively and must be inspected too. + for execution in plugin.iter(NS + "execution"): + exec_config = execution.find(NS + "configuration") + if exec_config is None: + continue + exec_id = execution.findtext(NS + "id") or "" + findings += [ + (artifact_id, f"execution '{exec_id}'", r) for r in permissive_settings(exec_config) + ] + + properties = project.find(NS + "properties") + if properties is not None: + for name, is_strict in STRICTNESS_PROPERTIES.items(): + value = properties.findtext(NS + name) + if value is not None and not is_strict(value.strip()): + findings.append((artifact_id, "properties", f"<{name}>{value.strip()}")) + + return artifact_id, len(plugins), findings + + +def run_check(path, required): + try: + root = ET.parse(path).getroot() + except (OSError, ET.ParseError) as exc: + print(f"::error::could not read the effective pom at {path}: {exc}") + return 1 + + projects = list(root.iter(NS + "project")) + + # A check that inspected nothing must never report success — that is the exact self-concealing + # failure this guard exists to prevent. + if not projects: + print(f"::error::no elements found in {path} — the check proved nothing") + return 1 + + findings = [] + without_plugin = [] + inspected = [] + for project in projects: + artifact_id, plugin_count, project_findings = check_project(project) + inspected.append(artifact_id) + findings += project_findings + if plugin_count == 0: + without_plugin.append(artifact_id) + print(f" checked {artifact_id} ({plugin_count} javadoc-plugin entries)") + + print(f"inspected {len(projects)} modules: {', '.join(inspected)}") + + # A sweep is only as good as its reactor. If a module drops out — a broken profile activation, a + # deleted — the sweep would still pass while that module publishes unswept. Naming the + # expected set turns that silent shrink into a red build. + missing = [m for m in required if m not in inspected] + if missing: + print( + "::error::expected modules absent from the release reactor: " + + ", ".join(missing) + + " — they were NOT swept, so javadoc strictness is unproven for them" + ) + return 1 + + if without_plugin: + print( + "::error::maven-javadoc-plugin is absent from in: " + + ", ".join(without_plugin) + + " — javadoc strictness is unproven for those modules" + ) + return 1 + + if findings: + print("::error::javadoc strictness is silenced in the effective pom (#203):") + for artifact_id, where, reason in findings: + print(f"::error:: {artifact_id}: {where} sets {reason}") + return 1 + + print("javadoc strictness confirmed: no module weakens maven-javadoc-plugin") + return 0 + + +# A guard that cannot fail is not a guard. If a future edit broke permissive_settings so it always +# returned [], every real sweep would still print "confirmed" — #200's shape relocated into this +# script. These fixtures make that regression loud, and cost no Maven invocation. +_STRICT_FIXTURE = """ + fixture-strict + maven-javadoc-plugin +""" + +_PERMISSIVE_FIXTURES = { + "plugin failOnError": "false", + "plugin doclint=none": "none", + "plugin doclint=syntax": "syntax", + "plugin skip": "true", + "JOption -Xdoclint:none": ( + "" + "-Xdoclint:none" + "" + ), + "JOption -Xdoclint:all,-missing,-reference": ( + "" + "-Xdoclint:all,-missing,-reference" + "" + ), + "execution failOnError": ( + "attach-javadocs" + "false" + "" + ), + # Boolean.valueOf("") is false, so an empty element silences errors exactly like `false`. + "plugin failOnError empty": "", + "singular additionalJOption": ( + "-Xdoclint:none" + ), +} + +_PERMISSIVE_PROPERTY_FIXTURES = { + "property maven.javadoc.failOnError": "false", + "property maven.javadoc.failOnError empty": "", + "property maven.javadoc.skip": "true", + "property doclint": "none", + "property maven.javadoc.skippedModules": "rift-java-core", +} + + +def _fixture(plugin_body="", properties=""): + props = f"{properties}" if properties else "" + return ( + '' + "fixture" + f"{props}" + "maven-javadoc-plugin" + f"{plugin_body}" + "" + ) + + +def self_test(): + """Assert the detector still detects. Returns 0 if every fixture behaves.""" + failures = 0 + + strict = ET.fromstring(_STRICT_FIXTURE) + _, _, strict_findings = check_project(next(strict.iter(NS + "project"))) + if strict_findings: + print(f"::error::self-test: the strict fixture was flagged: {strict_findings}") + failures += 1 + else: + print(" ok strict fixture -> no findings") + + cases = [(n, _fixture(plugin_body=b)) for n, b in _PERMISSIVE_FIXTURES.items()] + cases += [(n, _fixture(properties=p)) for n, p in _PERMISSIVE_PROPERTY_FIXTURES.items()] + for name, xml in cases: + project = next(ET.fromstring(xml).iter(NS + "project")) + _, _, found = check_project(project) + if not found: + print(f"::error::self-test: '{name}' was NOT detected — the checker is disarmed") + failures += 1 + else: + print(f" ok {name} -> detected") + + # The fixtures above exercise the MATCHER. run_check is the reporting layer, and a weakened + # `if findings: return 1` or a dropped guard there would leave those green while the real sweep + # printed "confirmed" — so exercise it end-to-end too, still without a Maven invocation. + with tempfile.TemporaryDirectory() as tmp: + strict_path = os.path.join(tmp, "strict.xml") + permissive_path = os.path.join(tmp, "permissive.xml") + with open(strict_path, "w", encoding="utf-8") as handle: + handle.write(_fixture()) + with open(permissive_path, "w", encoding="utf-8") as handle: + handle.write(_fixture(plugin_body="false")) + + roundtrips = [ + ("run_check: clean pom, --require satisfied", strict_path, ["fixture"], 0), + ("run_check: clean pom, --require missing a module", strict_path, ["absent-module"], 1), + ("run_check: permissive pom", permissive_path, [], 1), + ("run_check: unreadable path", os.path.join(tmp, "nope.xml"), [], 1), + ] + for name, path, required, expected in roundtrips: + # run_check narrates every module; keep the self-test's own output readable. + with contextlib.redirect_stdout(io.StringIO()): + actual = run_check(path, required) + if actual != expected: + print(f"::error::self-test: '{name}' returned {actual}, expected {expected}") + failures += 1 + else: + print(f" ok {name} -> {actual}") + + if failures: + print(f"::error::self-test failed: {failures} case(s)") + return 1 + print( + f"self-test passed: {len(cases)} silencers detected, " + f"{len(roundtrips)} run_check cases, strict fixture clean" + ) + return 0 + + +def main(argv): + # Parsed strictly, and never leniently: a typo'd flag, a dropped value or an empty list must be + # a usage error, not a silent skip of the completeness guard. Hand-rolled argv matching made + # `--requires x` degrade to "no modules required" while still printing "confirmed" — the exact + # prove-nothing-but-report-success shape this whole script exists to prevent. + parser = argparse.ArgumentParser( + prog=os.path.basename(argv[0]), + description="Fail if any module's effective maven-javadoc-plugin config silences javadoc errors.", + allow_abbrev=False, + ) + parser.add_argument("effective_pom", nargs="?", help="output of help:effective-pom") + parser.add_argument( + "--require", + metavar="A,B,C", + help="comma-separated artifactIds the reactor MUST contain; missing ones fail the check", + ) + parser.add_argument( + "--self-test", action="store_true", help="assert the detector still detects, then exit" + ) + args = parser.parse_args(argv[1:]) # unknown flags / extra positionals exit 2 here + + if args.self_test: + # `--self-test eff.xml --require x` would otherwise sweep nothing and exit 0 — the same + # prove-nothing-report-success shape the strict parsing above exists to reject. + if args.effective_pom or args.require: + parser.error("--self-test takes no other arguments") + return self_test() + + if not args.effective_pom: + parser.error("an effective-pom path is required (or use --self-test)") + + required = [] + if args.require is not None: + required = [m.strip() for m in args.require.split(",") if m.strip()] + if not required: + parser.error("--require was given an empty list; omit the flag or name the modules") + + return run_check(args.effective_pom, required) + + +if __name__ == "__main__": + sys.exit(main(sys.argv))