Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Sep 26, 2022

Conversation

simonbasle
Copy link
Member

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.

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.
@simonbasle simonbasle added this to the 1.0.x Backlog milestone Sep 21, 2022
@simonbasle simonbasle added the type/chore A task not related to code (build, formatting, process, ...) label Sep 21, 2022
@simonbasle simonbasle self-assigned this Sep 21, 2022
@simonbasle simonbasle requested a review from a team September 21, 2022 15:54
@simonbasle simonbasle marked this pull request as draft September 21, 2022 15:59
@simonbasle
Copy link
Member Author

simonbasle commented Sep 21, 2022

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
Copy link
Member Author

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

@simonbasle simonbasle marked this pull request as ready for review September 22, 2022 08:35
@pderop
Copy link
Member

pderop commented Sep 22, 2022

@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
Copy link
Member

pderop commented Sep 22, 2022

but I will let Violeta confirm.

@violetagg
Copy link
Member

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

Copy link
Member

@pderop pderop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simonbasle simonbasle merged commit c56742f into 1.0.x Sep 26, 2022
@simonbasle simonbasle deleted the improveJapiCmpBuild branch September 26, 2022 14:26
@simonbasle simonbasle modified the milestones: 1.0.x Backlog, 1.0.24 Sep 26, 2022
simonbasle added a commit that referenced this pull request Sep 26, 2022
simonbasle added a commit that referenced this pull request Sep 26, 2022
Adapt the changes to the new build.gradle files (core and http).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/chore A task not related to code (build, formatting, process, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants