reactor-netty icon indicating copy to clipboard operation
reactor-netty copied to clipboard

Improve JapiCmp: avoid misses, improve reporting and exclusions

Open simonbasle opened this issue 3 years ago • 1 comments

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.

  1. 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).

  1. 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.

simonbasle avatar Sep 21 '22 15:09 simonbasle

the onlyModified change seems to bring up complaints that were ignored before, on the following classes:

source incompatible change: **** MODIFIED CLASS: PUBLIC ABSTRACT reactor.netty.http.client.ContextAwareHttpClientMetricsRecorder  (not serializable)
source incompatible change: **** MODIFIED CLASS: PUBLIC ABSTRACT reactor.netty.http.client.HttpClient  (not serializable)
source incompatible change: **** MODIFIED CLASS: PUBLIC ABSTRACT reactor.netty.http.ContextAwareHttpMetricsRecorder  (not serializable)
source incompatible change: **** MODIFIED CLASS: PUBLIC ABSTRACT reactor.netty.http.server.ContextAwareHttpServerMetricsRecorder  (not serializable)
source incompatible change: **** MODIFIED CLASS: PUBLIC ABSTRACT reactor.netty.http.server.HttpServer  (not serializable)

(note: the above doesn't explain what exactly the issue is. please ignore "not serializable" for instance)

it seems that whatever the exact issues are, they are of type METHOD_ABSTRACT_ADDED_IN_SUPERCLASS and METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE, as excluding these two categories passes the japicmp.

simonbasle avatar Sep 21 '22 16:09 simonbasle

@reactor/netty-team I'll need to probably open a separate PR for the netty5 branch, right?

simonbasle avatar Sep 22 '22 08:09 simonbasle

@simonbasle ,

Usually, we do not create separate PR: we first merge the PR into the 1.0.x branch, then we merge it into main (for version 1.1.0), then after, we merge from main branch into the netty5 branch.

pderop avatar Sep 22 '22 08:09 pderop

but I will let Violeta confirm.

pderop avatar Sep 22 '22 08:09 pderop

@simonbasle We prepare separate PRs in very rare cases. For this one forward merge should be working.

violetagg avatar Sep 22 '22 11:09 violetagg