From c85d2bd027ea541ee8bcb7abe44826a521732d2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Wed, 28 Sep 2022 14:07:10 +0200 Subject: [PATCH] Improve JapiCmp: avoid misses, improve reporting and exclusions (#3198) This PR improves the japicmp integration in the build in several ways: 1) Add a finalizedBy task that prints a filtered report The basic text report uses * and ! as markers for binary and source incompatible changes respectively. Additionally, a MODIFIED class is marked with ***. This task attempts at pinpointing the incompatible changes only in the report, and printing these lines. This is called out in the ci job when prepare step fails fast. 2) Ensure sourceModified changes are considered See reactor/reactor#722. If a change is binary compatible but not source compatible, using `onlyBinaryCompatibleModified = true` will mask the issue. This change uses `onlyModified = true` instead. This is slightly more verbose, but this is alleviated by (1). 3) Bump to japicmp plugin v0.4.1, exclude NEW_DEFAULT_METHOD as a whole The new `compatibilityChangeExcludes` parameter allows us to ignore all occurrences of a default method added to an interface, instead of having to exclude each such method one by one. --- .github/workflows/ci.yml | 5 ++++- gradle/libs.versions.toml | 2 +- reactor-core/build.gradle | 29 ++++++++++++++++++++++++++++- reactor-test/build.gradle | 35 +++++++++++++++++++++++++++++++---- 4 files changed, 64 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 67f6d87471..bbf9d76caf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,7 +33,10 @@ jobs: echo -e "\n - \u001b[1mSpotless (license headers)\u001b[0m failures on touched java files \033[38;5;255;48;5;0m\u001b[1mcan be automatically fixed by running\u001b[0m:" echo -e " \033[38;5;0;48;5;255m ./gradlew spotlessApply \033[0m" echo -e "\n - \u001b[1mAPI Compatibility\u001b[0m failures should be considered carefully and \033[38;5;255;48;5;0m\u001b[1mdiscussed with maintainers in the PR\u001b[0m" - echo " If there are failures, the detail should be available in the logs of the api compatibility step above" + echo " If there are failures, the detail should be available in the step's log:" + echo -e " Look for the \033[38;5;0;48;5;255m API compatibility failures \033[0m block(s)." + echo " Alternatively, locally run the following command to get access to the full report:" + echo -e " \033[38;5;0;48;5;255m ./gradlew japicmp \033[0m" echo "" exit -1 core-fast: diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 1dbc3aa2f8..ec4f8846d6 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -44,7 +44,7 @@ asciidoctor-convert = { id = "org.asciidoctor.jvm.convert", version.ref = "ascii asciidoctor-pdf = { id = "org.asciidoctor.jvm.pdf", version.ref = "asciidoctor" } bnd = { id = "biz.aQute.bnd.builder", version = "6.3.1" } download = { id = "de.undercouch.download", version = "5.1.2" } -japicmp = { id = "me.champeau.gradle.japicmp", version = "0.4.0" } +japicmp = { id = "me.champeau.gradle.japicmp", version = "0.4.1" } jcstress = { id = "io.github.reyerizo.gradle.jcstress", version = "0.8.13" } kotlin = { id = "org.jetbrains.kotlin.jvm", version.ref = "kotlin" } nohttp = { id = "io.spring.nohttp", version = "0.0.10" } diff --git a/reactor-core/build.gradle b/reactor-core/build.gradle index 36c974451d..44ad59f663 100644 --- a/reactor-core/build.gradle +++ b/reactor-core/build.gradle @@ -135,6 +135,28 @@ task downloadBaseline(type: Download) { dest "${buildDir}/baselineLibs/reactor-core-${libs.versions.baseline.core.api.get()}.jar" } +def japicmpReport = tasks.register('japicmpReport') { + onlyIf { + japicmp.state.failure != null + } + doLast { + def reportFile = file("${project.buildDir}/reports/japi.txt") + if (reportFile.exists()) { + println "\n **********************************" + println " * /!\\ API compatibility failures *" + println " **********************************" + println "Japicmp report was filtered and interpreted to find the following incompatibilities:" + reportFile.eachLine { + if (it.contains("*") && (!it.contains("***") || it.contains("****"))) + println "source incompatible change: $it" + else if (it.contains("!")) + println "binary incompatible change: $it" + } + } + else println "No incompatible change to report" + } +} + task japicmp(type: JapicmpTask) { if (project.gradle.startParameter.isOffline()) { println "Offline: skipping downloading of baseline and JAPICMP" @@ -148,16 +170,21 @@ task japicmp(type: JapicmpTask) { println "Will download and perform baseline comparison with ${libs.versions.baseline.core.api.get()}" dependsOn(downloadBaseline) dependsOn(jar) + finalizedBy(japicmpReport) } oldClasspath.from(files("${buildDir}/baselineLibs/reactor-core-${libs.versions.baseline.core.api.get()}.jar")) newClasspath.from(files(jar.archiveFile)) - onlyBinaryIncompatibleModified = true + // these onlyXxx parameters result in a report that is slightly too noisy, but better than + // onlyBinaryIncompatibleModified = true which masks source-incompatible-only changes + onlyBinaryIncompatibleModified = false + onlyModified = true failOnModification = true failOnSourceIncompatibility = true txtOutputFile = file("${project.buildDir}/reports/japi.txt") ignoreMissingClasses = true includeSynthetic = true + compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ] // TODO after a .0 release, bump the gradle.properties baseline // TODO after a .0 release, remove the reactor-core exclusions below if any diff --git a/reactor-test/build.gradle b/reactor-test/build.gradle index 0ee9ec6bbf..a84f624ee4 100644 --- a/reactor-test/build.gradle +++ b/reactor-test/build.gradle @@ -64,28 +64,55 @@ task downloadBaseline(type: Download) { finalizedBy { japicmp } } +def japicmpReport = tasks.register('japicmpReport') { + onlyIf { + japicmp.state.failure != null + } + doLast { + def reportFile = file("${project.buildDir}/reports/japi.txt") + if (reportFile.exists()) { + println "\n *********************************" + println " * /!\\ API compatibility failures *" + println " **********************************" + println "Japicmp report was filtered and interpreted to find the following incompatibilities:" + reportFile.eachLine { + if (it.contains("*") && (!it.contains("***") || it.contains("****"))) + println "source incompatible change: $it" + else if (it.contains("!")) + println "binary incompatible change: $it" + } + } + else println "No incompatible change to report" + } +} + task japicmp(type: JapicmpTask) { if (project.gradle.startParameter.isOffline()) { println "Offline: skipping downloading of baseline and JAPICMP" - enabled = false + enabled = false } else if ("${libs.versions.baseline.core.api.get()}" == "SKIP") { println "SKIP: Instructed to skip the baseline comparison" - enabled = false + enabled = false } else { println "Will download and perform baseline comparison with ${libs.versions.baseline.core.api.get()}" - dependsOn(downloadBaseline) + dependsOn(downloadBaseline) + finalizedBy(japicmpReport) } oldClasspath.from(files("${buildDir}/baselineLibs/reactor-test-${libs.versions.baseline.core.api.get()}.jar")) newClasspath.from(files(jar.archiveFile)) - onlyBinaryIncompatibleModified = true + // these onlyXxx parameters result in a report that is slightly too noisy, but better than + // onlyBinaryIncompatibleModified = true which masks source-incompatible-only changes + onlyBinaryIncompatibleModified = false + onlyModified = true failOnModification = true failOnSourceIncompatibility = true txtOutputFile = file("${project.buildDir}/reports/japi.txt") ignoreMissingClasses = true includeSynthetic = true + compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ] // TODO after a .0 release, remove the reactor-test exclusions below if any methodExcludes = [ ]