paulk-asert commented on PR #2770: URL: https://github.com/apache/groovy/pull/2770#issuecomment-5211246187
I haven't done a proper review yet but here is an early AI read: > Overall I'm in favour — this encodes knowledge that currently lives only in our heads, and it's knowledge no tool can derive after the fact. I did a deep dive comparing it against our existing `checkBinaryCompatibility` task to see whether they overlap, and I'm convinced they're complementary: japicmp tells us *what changed* across the whole protected+ surface vs one baseline, but it can't distinguish "internal wiring, safe to change" from "referenced by bytecode an older compiler emitted". The Groovy 6 report makes this vivid: it flags ~850 binary-incompatible items (mostly the callsite extraction and Grape split), and deciding which of those actually break the compiled-code contract required exactly the human knowledge this annotation captures. The `since` value also covers something a single-baseline compare never can: how far back the promise extends (2.5-compiled bytecode running on 5/6 spans many baselines). > > A few suggestions: > > 1. **`@Retention(CLASS)` instead of `SOURCE`.** This is my main request. With SOURCE retention the annotation is documentation only — invisible to japicmp, ArchUnit-style tests, or any bytecode scanner. With CLASS retention (no runtime loading cost, a few bytes per member) we can wire it straight into the build: the japicmp plugin we already use (0.4.6) supports `annotationIncludes`, so we could add a second, strict task variant per module — `annotationIncludes = ['@org.apache.groovy.lang.annotation.GroovyABI']`, `failOnModification = true` — making the annotated subset an enforced CI gate while the broad report stays advisory. It would also finally give the long-standing `richReport` TODO in `subprojects/binary-compatibility/build.gradle` its custom rule. > > 2. **Prior art supports the combination.** Gradle's own build does exactly this: japicmp plus annotation-driven rules (`@Incubating`, `@Deprecated`) plus an `accepted-public-api-changes.json` escape hatch for intentional breaks. Most tellingly, they have `@UsedByScanPlugin` — an annotation marking *internal* code that external bytecode links against — which is the direct analogue of what you're proposing. > > 3. **Completeness could be attacked mechanically.** Rather than hand-auditing (which you rightly note was a lot of work), we could compile a corpus (our own test suite, indy on and off), harvest all `INVOKE*`/`GETSTATIC` targets landing in `org.codehaus.*`/`org.apache.groovy.*`, and diff against the annotated set. That could even become a build-time check so the set can't silently rot. > > 4. **Lifecycle of a dropped guarantee.** Worth deciding now: when we intentionally drop an ABI member at a major version, do we just delete the annotation/member, or record it (a `removedIn` attribute, or an acceptance file à la Gradle)? Related: the `since` archaeology is only worth polishing if we also document the support-window policy it implies (e.g. "kept for N major versions after `since`") — otherwise a plain marker would serve. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
