From cf3b61c7ff99dc667ea96b48e142b364a0d709c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Wed, 21 Sep 2022 17:48:14 +0200 Subject: [PATCH 1/3] Improve JapiCmp: avoid misses, improve reporting and exclusions 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. --- .github/workflows/check_transport.yml | 5 +++- reactor-netty-core/build.gradle | 33 ++++++++++++++++++++++++-- reactor-netty-http-brave/build.gradle | 33 ++++++++++++++++++++++++-- reactor-netty-http/build.gradle | 34 ++++++++++++++++++++++++--- 4 files changed, 97 insertions(+), 8 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..1170bb30a3 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 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..aae52dbfa0 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 + // 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 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..1da8b28a16 100644 --- a/reactor-netty-http/build.gradle +++ b/reactor-netty-http/build.gradle @@ -180,16 +180,44 @@ 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 - failOnModification = 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) From dcc9b2965a965f0a109e44b45b1ac9bc70db2cae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Wed, 21 Sep 2022 18:00:31 +0200 Subject: [PATCH 2/3] oops, botched copy paste --- reactor-netty-core/build.gradle | 2 +- reactor-netty-http-brave/build.gradle | 2 +- reactor-netty-http/build.gradle | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/reactor-netty-core/build.gradle b/reactor-netty-core/build.gradle index 1170bb30a3..b31c077a79 100644 --- a/reactor-netty-core/build.gradle +++ b/reactor-netty-core/build.gradle @@ -208,7 +208,7 @@ task japicmp(type: JapicmpTask) { // 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 + onlyModified = true failOnModification = true failOnSourceIncompatibility = true txtOutputFile = file("${project.buildDir}/reports/japi.txt") diff --git a/reactor-netty-http-brave/build.gradle b/reactor-netty-http-brave/build.gradle index aae52dbfa0..4016025382 100644 --- a/reactor-netty-http-brave/build.gradle +++ b/reactor-netty-http-brave/build.gradle @@ -107,7 +107,7 @@ task japicmp(type: JapicmpTask) { // 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 + onlyModified = true failOnModification = true failOnSourceIncompatibility = true txtOutputFile = file("${project.buildDir}/reports/japi.txt") diff --git a/reactor-netty-http/build.gradle b/reactor-netty-http/build.gradle index 1da8b28a16..d3a1b06a02 100644 --- a/reactor-netty-http/build.gradle +++ b/reactor-netty-http/build.gradle @@ -211,7 +211,8 @@ task japicmp(type: JapicmpTask) { // 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 + onlyModified = true + failOnModification = true failOnSourceIncompatibility = true txtOutputFile = file("${project.buildDir}/reports/japi.txt") ignoreMissingClasses = true From c8c72ddd057122f190fb3b1ef716be2af5eb00f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Thu, 22 Sep 2022 10:27:07 +0200 Subject: [PATCH 3/3] Fix submodules japicmp conf so that netty-core isn't considered in newClasspath --- reactor-netty-http-brave/build.gradle | 2 +- reactor-netty-http/build.gradle | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/reactor-netty-http-brave/build.gradle b/reactor-netty-http-brave/build.gradle index 4016025382..ab3fcb735d 100644 --- a/reactor-netty-http-brave/build.gradle +++ b/reactor-netty-http-brave/build.gradle @@ -103,7 +103,7 @@ task japicmp(type: JapicmpTask) { onlyIf { "$compatibleVersion" != "SKIP" } oldClasspath.from(files("${buildDir}/baselineLibs/reactor-netty-http-brave-${compatibleVersion}.jar")) - newClasspath.from(files(jar.archiveFile, project(':reactor-netty-core').jar)) + 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 diff --git a/reactor-netty-http/build.gradle b/reactor-netty-http/build.gradle index d3a1b06a02..d0d74fc9c5 100644 --- a/reactor-netty-http/build.gradle +++ b/reactor-netty-http/build.gradle @@ -207,7 +207,7 @@ task japicmp(type: JapicmpTask) { onlyIf { "$compatibleVersion" != "SKIP" } oldClasspath.from(files("${buildDir}/baselineLibs/reactor-netty-http-${compatibleVersion}.jar")) - newClasspath.from(files(jar.archiveFile, project(':reactor-netty-core').jar)) + 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