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

Update to Gradle 7.3 and fail build if binary compatibility broken #3779

Merged
merged 8 commits into from
Oct 21, 2021

Conversation

anuraaga
Copy link
Contributor

Found the magic to allow japicmp to enforce binary compatibility.

Compatible change that passed, and still prints diff as normal
c51d55f

Incompatible change that fails, while also outputing the diff for local debugging
c23ac31

@anuraaga
Copy link
Contributor Author

Ran locally with java 11, but looks like something about this change triggers bad behavior with java 17 will see what's up

General error during canonicalization: PermittedSubclasses requires ASM9

@jkwatson
Copy link
Contributor

Cool. I had looked into this very early on in the japicmp journey, but bailed out when running into the default method thing. Glad you figured out how to deal with that.

@anuraaga anuraaga marked this pull request as draft October 21, 2021 02:10
@anuraaga anuraaga changed the title Fail build if binary compatibility broken Update to Gradle 7.3 and fail build if binary compatibility broken Oct 21, 2021
@anuraaga anuraaga marked this pull request as ready for review October 21, 2021 06:58
@anuraaga
Copy link
Contributor Author

Looking at the Gradle 7.3 release notes it mentions support for Java 17 - despite us using Java 17 for some time already :) Presumably due to corner cases that still caused errors, and it seems that this change was one of them.

I think Gradle RCs tend to be good to use, and can bring up Gradle bugs that we would otherwise hit post-release in some cases, so think it's fine to upgrade now and get our compatibility check with it

@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #3779 (1ce26c9) into main (2187950) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3779      +/-   ##
============================================
+ Coverage     89.05%   89.15%   +0.10%     
- Complexity     3940     3959      +19     
============================================
  Files           471      473       +2     
  Lines         12435    12442       +7     
  Branches       1226     1225       -1     
============================================
+ Hits          11074    11093      +19     
+ Misses          945      935      -10     
+ Partials        416      414       -2     
Impacted Files Coverage Δ
...try/sdk/testing/exporter/InMemorySpanExporter.java 93.33% <0.00%> (-2.13%) ⬇️
...exporter/otlp/http/trace/OtlpHttpSpanExporter.java 85.24% <0.00%> (-2.08%) ⬇️
.../incubator/trace/ExecutorServiceSpanProcessor.java 95.28% <0.00%> (-0.95%) ⬇️
...porter/otlp/internal/grpc/DefaultGrpcExporter.java 93.33% <0.00%> (-0.90%) ⬇️
...telemetry/sdk/trace/export/BatchSpanProcessor.java 90.07% <0.00%> (-0.71%) ⬇️
...rter/otlp/http/metrics/OtlpHttpMetricExporter.java 86.88% <0.00%> (-0.39%) ⬇️
...y/exporter/otlp/http/logs/OtlpHttpLogExporter.java 78.33% <0.00%> (-0.24%) ⬇️
...ry/sdk/metrics/testing/InMemoryMetricExporter.java 100.00% <0.00%> (ø)
...telemetry/sdk/logs/export/InMemoryLogExporter.java 100.00% <0.00%> (ø)
...emetry/exporter/otlp/internal/ExporterMetrics.java 100.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2187950...1ce26c9. Read the comment docs.

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

👍🏽 We'll have to figure out what we're going to do when we need to rev to 2.0 and we actually do need to break binary compatibility. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants