Improve JapiCmp: avoid misses, improve reporting and exclusions (#2497)

This commit improves the japicmp integration in the build:

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 report is called out in the "how to fix" tip in the CI preliminary
step.

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) Exclude NEW_DEFAULT_METHOD as a whole

Since japicmp-grale-plugin v0.4.1 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.

A misconfiguration is also fixed to avoid comparing sources which
include reactor-netty-core in other submodules, leading to false
positives.

.github/workflows/check_transport.yml

@@ -30,7 +30,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
 

reactor-netty-core/build.gradle

@@ -177,16 +177,45 @@ task downloadBaseline(type: Download) {
 	dest "${buildDir}/baselineLibs/reactor-netty-core-${compatibleVersion}-original.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) {
+	finalizedBy(japicmpReport)
+	onlyIf { "$compatibleVersion" != "SKIP" }
+
 	oldClasspath.from(files("${buildDir}/baselineLibs/reactor-netty-core-${compatibleVersion}-original.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
-	onlyIf { "$compatibleVersion" != "SKIP" }
+
+	compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ]
 }
 
 tasks.japicmp.dependsOn(downloadBaseline)

reactor-netty-http-brave/build.gradle

@@ -76,16 +76,45 @@ task downloadBaseline(type: Download) {
 	dest "${buildDir}/baselineLibs/reactor-netty-http-brave-${compatibleVersion}.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) {
+	finalizedBy(japicmpReport)
+	onlyIf { "$compatibleVersion" != "SKIP" }
+
 	oldClasspath.from(files("${buildDir}/baselineLibs/reactor-netty-http-brave-${compatibleVersion}.jar"))
-	newClasspath.from(files(jar.archiveFile, project(':reactor-netty-core').jar))
-	onlyBinaryIncompatibleModified = true
+	newClasspath.from(files(jar.archiveFile))
+	// 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
-	onlyIf { "$compatibleVersion" != "SKIP" }
+
+	compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ]
 }
 
 tasks.japicmp.dependsOn(downloadBaseline)

reactor-netty-http/build.gradle

@@ -180,16 +180,45 @@ task downloadBaseline(type: Download) {
 	dest "${buildDir}/baselineLibs/reactor-netty-http-${compatibleVersion}.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) {
+	finalizedBy(japicmpReport)
+	onlyIf { "$compatibleVersion" != "SKIP" }
+
 	oldClasspath.from(files("${buildDir}/baselineLibs/reactor-netty-http-${compatibleVersion}.jar"))
-	newClasspath.from(files(jar.archiveFile, project(':reactor-netty-core').jar))
-	onlyBinaryIncompatibleModified = true
+	newClasspath.from(files(jar.archiveFile))
+	// 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
-	onlyIf { "$compatibleVersion" != "SKIP" }
+
+	compatibilityChangeExcludes = [ "METHOD_NEW_DEFAULT" ]
 }
 
 tasks.japicmp.dependsOn(downloadBaseline)