From c56742f3a770d3a222dc611a01c78514e0897f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Mon, 26 Sep 2022 16:26:27 +0200 Subject: [PATCH] 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 | 5 +++- reactor-netty-core/build.gradle | 33 +++++++++++++++++++++++-- reactor-netty-http-brave/build.gradle | 35 ++++++++++++++++++++++++--- reactor-netty-http/build.gradle | 35 ++++++++++++++++++++++++--- 4 files changed, 99 insertions(+), 9 deletions(-) diff --git a/.github/workflows/check_transport.yml b/.github/workflows/check_transport.yml index 246ba45f06..b341275a96 100644 --- a/.github/workflows/check_transport.yml +++ b/.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 diff --git a/reactor-netty-core/build.gradle b/reactor-netty-core/build.gradle index 17cc986b1c..b31c077a79 100644 --- a/reactor-netty-core/build.gradle +++ b/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) diff --git a/reactor-netty-http-brave/build.gradle b/reactor-netty-http-brave/build.gradle index f9b50ee647..ab3fcb735d 100644 --- a/reactor-netty-http-brave/build.gradle +++ b/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) diff --git a/reactor-netty-http/build.gradle b/reactor-netty-http/build.gradle index f8fb9bba58..d0d74fc9c5 100644 --- a/reactor-netty-http/build.gradle +++ b/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)