Re-enable animalsniffer, fixing violations#11762
Merged
ejona86 merged 3 commits intogrpc:masterfrom Dec 19, 2024
Merged
Conversation
In 61f19d7 I swapped the signatures to use the version catalog. But I failed to preserve the `@signature` extension and it all seemed to work... But in fact all the animalsniffer tasks were completing as SKIPPED as they lacked signatures. The build.gradle changes in this commit are to fix that while still using version catalog. But while it was broken violations crept in. Most violations weren't too important and we're not surprised went unnoticed. For example, Netty with TLS has long required the Java 8 API `setEndpointIdentificationAlgorithm()`, so using `Optional` in the same code path didn't harm anything in particular. I still swapped it to Guava's `Optional` to avoid overuse of `@IgnoreJRERequirement`. One important violation has not been fixed and instead I've disabled the android signature in api/build.gradle for the moment. The violation is in StatusException using the `fillInStackTrace` overload of Exception. This problem [had been noticed][PR11066], but we couldn't figure out what was going on. AnimalSniffer is now noticing this and agreeing with the internal linter. There is still a question of why our interop tests failed to notice this, but given they are no longer running on pre-API level 24, that may forever be a mystery. [PR11066]: grpc#11066
Contributor
larry-safran
left a comment
There was a problem hiding this comment.
Should all of interop-testing check for Java 8 instead of 7?
kannanjgithub
approved these changes
Dec 19, 2024
Member
Author
It checks for Java 8 and Android. interop-testing is used by android-interop-testing. |
This was referenced Dec 19, 2024
Contributor
larry-safran
left a comment
There was a problem hiding this comment.
A couple of nit things I commented on, but seems good.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In 61f19d7 I swapped the signatures to use the version catalog. But I failed to preserve the
@signatureextension and it all seemed to work... But in fact all the animalsniffer tasks were completing as SKIPPED as they lacked signatures. The build.gradle changes in this commit are to fix that while still using version catalog.But while it was broken violations crept in. Most violations weren't too important and we're not surprised went unnoticed. For example, Netty with TLS has long required the Java 8 API
setEndpointIdentificationAlgorithm(), so usingOptionalin the same code path didn't harm anything in particular. I still swapped it to Guava'sOptionalto avoid overuse of@IgnoreJRERequirement.One important violation has not been fixed and instead I've disabled the android signature in api/build.gradle for the moment. The violation is in StatusException using the
fillInStackTraceoverload of Exception. This problem had been noticed, but we couldn't figure out what was going on. AnimalSniffer is now noticing this and agreeing with the internal linter. There is still a question of why our interop tests failed to notice this, but given they are no longer running on pre-API level 24, that may forever be a mystery.