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.

Sign up for GitHub

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

Jump to bottom

Builds with japicmp should set onlyModified = true / onBinaryIncompatibleModified = false #722

Closed
3 tasks done
simonbasle opened this issue Jul 1, 2022 · 0 comments
Closed
3 tasks done

Builds with japicmp should set onlyModified = true / onBinaryIncompatibleModified = false #722

simonbasle opened this issue Jul 1, 2022 · 0 comments
Assignees
@simonbasle
Labels
area/all-projects issues that are transverse to all projects, epics
Milestone
BOM 2020.0.24

Comments

@simonbasle
Copy link
Member

simonbasle commented Jul 1, 2022 •
edited

Context

It was found out that some significant modifications were not picked up by japicmp gradle plugin in our build: in reactor-core, a new (non-default) method was added to an interface and this didn't fail the build.

Interestingly, modifying the added method to add a default implementation did result in a failing build. We do know that japicmp considers a new default method on interface a binary (this is key) incompatible change, and that such changes usually need to be excluded specifically.

What this surfaced though is that the current japicmp plugin configuration suppresses source incompatible changes and only fails the build if a binary incompatible change is found. This is due to the use of the option onlyBinaryIncompatibleModified.

To sum it up, taking the example of adding a method (non-default) to an interface:

  • onlyBinaryIncompatibleModified = true (current state) suppresses source incompatible failures, so the japicmp task succeeds when it should fail 😞
  • removing the above or setting it to false produces a failure, but also makes the report useless by listing everything that hasn't changed as well
  • another param, onlyModified = true, does trigger the failure and improves on the noise. it still lists new methods and classes that are neither source incompatible nor binary incompatible, but that should be far less than listing all unmodified public classes
    One drawback is that without the option, a lot of noise

onlyModified = true "noise" includes new public classes and new methods on an existing class (eg. a new operator is added to Flux). I couldn't find a param that only list classes with incompatible changes (be it source or binary) in the report.

Desired Change

This issue is to track the remediation across all projects that have japicmp checks in place.

1 . set onlyModified = true in every japicmp configuration blocks
2. remove onlyBinaryIncompatibleModified = true from same blocks

  • in reactor-core
  • in reactor-netty
  • in reactor-pool
@simonbasle simonbasle added the area/all-projects issues that are transverse to all projects, epics label Jul 1, 2022
@simonbasle simonbasle self-assigned this Sep 20, 2022
@simonbasle simonbasle mentioned this issue Sep 21, 2022
Merged
simonbasle added a commit to reactor/reactor-core that referenced this issue Sep 21, 2022
@simonbasle
Ensure sourceModified changes are considered (see reactor/reactor#722)
36bd19b
@simonbasle simonbasle mentioned this issue Sep 21, 2022
Merged
simonbasle added a commit to reactor/reactor-netty that referenced this issue Sep 21, 2022
@simonbasle
Improve JapiCmp: avoid misses, improve reporting and exclusions
cf3b61c 
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 added a commit to reactor/reactor-pool that referenced this issue Sep 21, 2022
@simonbasle
Improve JapiCmp: avoid misses, improve reporting and exclusions
f54c7c9 
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) Update japicmp-gradle-plugin, 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 mentioned this issue Sep 21, 2022
Merged
simonbasle added a commit to reactor/reactor-netty that referenced this issue Sep 26, 2022
@simonbasle
Improve JapiCmp: avoid misses, improve reporting and exclusions (#2497)
c56742f 
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.
simonbasle added a commit to reactor/reactor-core that referenced this issue Sep 28, 2022
@simonbasle
Improve JapiCmp: avoid misses, improve reporting and exclusions (#3198)
c85d2bd 
This PR improves the japicmp integration in the build in several ways:

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 is called out
in the ci job when prepare step fails fast.

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) Bump to japicmp plugin v0.4.1, exclude NEW_DEFAULT_METHOD as a whole
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 added a commit to reactor/reactor-pool that referenced this issue Sep 28, 2022
@simonbasle
Improve JapiCmp: avoid misses, improve reporting and exclusions (#162)
16aa37b 
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) Update japicmp-gradle-plugin, 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 BOM 2020.0.24 milestone Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@simonbasle simonbasle

Labels
area/all-projects issues that are transverse to all projects, epics
Projects
None yet
Milestone
BOM 2020.0.24
Development

No branches or pull requests
1 participant
@simonbasle