Skip to content

Commit

Permalink
Re-enable animalsniffer, fixing violations
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ejona86 committed Dec 18, 2024
1 parent 486b8ba commit abb3211
Show file tree
Hide file tree
Showing 55 changed files with 325 additions and 104 deletions.
6 changes: 5 additions & 1 deletion alts/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ dependencies {
classifier = "linux-x86_64"
}
}
signature libraries.signature.java
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
}

configureProtoCompilation()
Expand Down
15 changes: 13 additions & 2 deletions api/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ tasks.named("jar").configure {
dependencies {
compileOnly sourceSets.context.output
api libraries.jsr305,
libraries.animalsniffer.annotations,
libraries.errorprone.annotations
implementation libraries.guava

Expand All @@ -48,8 +49,18 @@ dependencies {
testImplementation project(':grpc-testing')
testImplementation libraries.guava.testlib

signature libraries.signature.java
signature libraries.signature.android
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
// Temporarily disabled until StatusException is fixed.
// Context: https://github.com/grpc/grpc-java/pull/11066
//signature (libraries.signature.android) {
// artifact {
// extension = "signature"
// }
//}
}

tasks.named("javadoc").configure {
Expand Down
2 changes: 2 additions & 0 deletions api/src/main/java/io/grpc/TimeUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
package io.grpc;

import java.time.Duration;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;

final class TimeUtils {
private TimeUtils() {}

@IgnoreJRERequirement
static long convertToNanos(Duration duration) {
try {
return duration.toNanos();
Expand Down
2 changes: 2 additions & 0 deletions api/src/test/java/io/grpc/CallOptionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import io.grpc.internal.SerializingExecutor;
import java.time.Duration;
import java.util.concurrent.Executor;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -152,6 +153,7 @@ public void withDeadlineAfter() {
}

@Test
@IgnoreJRERequirement
public void withDeadlineAfterDuration() {
Deadline actual = CallOptions.DEFAULT.withDeadlineAfter(Duration.ofMinutes(1L)).getDeadline();
Deadline expected = Deadline.after(1, MINUTES);
Expand Down
5 changes: 4 additions & 1 deletion api/src/test/java/io/grpc/SynchronizationContextTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
import org.junit.After;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -248,6 +249,7 @@ public void schedule() {
}

@Test
@IgnoreJRERequirement
public void scheduleDuration() {
MockScheduledExecutorService executorService = new MockScheduledExecutorService();
ScheduledHandle handle =
Expand All @@ -265,6 +267,7 @@ public void scheduleDuration() {
}

@Test
@IgnoreJRERequirement
public void scheduleWithFixedDelayDuration() {
MockScheduledExecutorService executorService = new MockScheduledExecutorService();
ScheduledHandle handle =
Expand Down Expand Up @@ -402,4 +405,4 @@ static class MockScheduledExecutorService extends ForwardingScheduledExecutorSer
return future = super.scheduleWithFixedDelay(command, intialDelay, delay, unit);
}
}
}
}
4 changes: 3 additions & 1 deletion api/src/test/java/io/grpc/TimeUtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
import static org.junit.Assert.assertEquals;

import java.time.Duration;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Unit tests for {@link TimeUtils}. */
@RunWith(JUnit4.class)
@IgnoreJRERequirement
public class TimeUtilsTest {

@Test
Expand Down Expand Up @@ -56,4 +58,4 @@ public void testConvertTooLargeNegativeDuration() {

assertEquals(Long.MIN_VALUE, TimeUtils.convertToNanos(duration));
}
}
}
12 changes: 10 additions & 2 deletions auth/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ dependencies {
project(':grpc-core'),
project(":grpc-context"), // Override google-auth dependency with our newer version
libraries.google.auth.oauth2Http
signature libraries.signature.java
signature libraries.signature.android
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
signature (libraries.signature.android) {
artifact {
extension = "signature"
}
}
}
6 changes: 5 additions & 1 deletion authz/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ dependencies {
shadow configurations.implementation.getDependencies().minus([xdsDependency])
shadow project(path: ':grpc-xds', configuration: 'shadow')

signature libraries.signature.java
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
}

tasks.named("jar").configure {
Expand Down
6 changes: 5 additions & 1 deletion benchmarks/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ dependencies {
testImplementation libraries.junit,
libraries.mockito.core

signature libraries.signature.java
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
}

import net.ltgt.gradle.errorprone.CheckSeverity
Expand Down
12 changes: 10 additions & 2 deletions census/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,16 @@ dependencies {
project(':grpc-testing'),
libraries.opencensus.impl

signature libraries.signature.java
signature libraries.signature.android
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
signature (libraries.signature.android) {
artifact {
extension = "signature"
}
}
}

tasks.named("javadoc").configure {
Expand Down
12 changes: 10 additions & 2 deletions contextstorage/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,16 @@ dependencies {
libraries.assertj.core
testImplementation 'junit:junit:4.13.1'// opentelemetry.sdk.testing uses compileOnly for assertj

signature libraries.signature.java
signature libraries.signature.android
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
signature (libraries.signature.android) {
artifact {
extension = "signature"
}
}
}

tasks.named("jar").configure {
Expand Down
12 changes: 10 additions & 2 deletions core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,16 @@ dependencies {

jmh project(':grpc-testing')

signature libraries.signature.java
signature libraries.signature.android
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
signature (libraries.signature.android) {
artifact {
extension = "signature"
}
}
}

tasks.named("javadoc").configure {
Expand Down
9 changes: 4 additions & 5 deletions core/src/main/java/io/grpc/internal/SpiffeUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,12 @@
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.Files;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.CertificateFactory;
Expand Down Expand Up @@ -188,8 +187,8 @@ public static SpiffeBundle loadTrustBundleFromFile(String trustBundleFile) throw
}

private static Map<String, ?> readTrustDomainsFromFile(String filePath) throws IOException {
Path path = Paths.get(checkNotNull(filePath, "trustBundleFile"));
String json = new String(Files.readAllBytes(path), StandardCharsets.UTF_8);
File file = new File(checkNotNull(filePath, "trustBundleFile"));
String json = new String(Files.toByteArray(file), StandardCharsets.UTF_8);
Object jsonObject = JsonParser.parse(json);
if (!(jsonObject instanceof Map)) {
throw new IllegalArgumentException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import static com.google.common.truth.Truth.assertThat;

import java.time.Instant;
import java.util.concurrent.TimeUnit;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -36,10 +35,8 @@ public void testConcurrentCurrentTimeNanos() {
// Get the current time from the ConcurrentTimeProvider
long actualTimeNanos = concurrentTimeProvider.currentTimeNanos();

// Get the current time from Instant for comparison
Instant instantNow = Instant.now();
long expectedTimeNanos = TimeUnit.SECONDS.toNanos(instantNow.getEpochSecond())
+ instantNow.getNano();
// Get the current time from System for comparison
long expectedTimeNanos = TimeUnit.MILLISECONDS.toNanos(System.currentTimeMillis());

// Validate the time returned is close to the expected value within a tolerance
// (i,e 10 millisecond tolerance in nanoseconds).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.time.Instant;
import java.util.concurrent.TimeUnit;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -28,6 +29,7 @@
* Unit tests for {@link InstantTimeProvider}.
*/
@RunWith(JUnit4.class)
@IgnoreJRERequirement
public class InstantTimeProviderTest {
@Test
public void testInstantCurrentTimeNanos() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.SettableFuture;
Expand Down Expand Up @@ -1199,8 +1200,10 @@ public void addressResolutionError_noPriorNameResolution_usesDefaultServiceConfi
// config simply gets ignored and not gets reassigned.
resolver.resolved();
timer.forwardNanos(1234);
assertThat(getStats(channel).channelTrace.events.stream().filter(
event -> event.description.equals("Service config changed")).count()).isEqualTo(0);
assertThat(Iterables.filter(
getStats(channel).channelTrace.events,
event -> event.description.equals("Service config changed")))
.isEmpty();
}

@Test
Expand Down
25 changes: 15 additions & 10 deletions core/src/test/java/io/grpc/internal/SpiffeUtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@
import static org.junit.Assert.assertTrue;

import com.google.common.base.Optional;
import com.google.common.io.ByteStreams;
import io.grpc.internal.SpiffeUtil.SpiffeBundle;
import io.grpc.internal.SpiffeUtil.SpiffeId;
import io.grpc.testing.TlsTesting;
import io.grpc.util.CertificateUtils;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.io.OutputStream;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collection;
Expand Down Expand Up @@ -247,10 +248,12 @@ public void setUp() throws Exception {
}

private String copyFileToTmp(String fileName) throws Exception {
Path tempFilePath = tempFolder.newFile(fileName).toPath();
File tempFilePath = tempFolder.newFile(fileName);
try (InputStream resourceStream = SpiffeUtilTest.class.getClassLoader()
.getResourceAsStream(TEST_DIRECTORY_PREFIX + fileName)) {
Files.copy(resourceStream, tempFilePath, StandardCopyOption.REPLACE_EXISTING);
.getResourceAsStream(TEST_DIRECTORY_PREFIX + fileName);
OutputStream fileStream = new FileOutputStream(tempFilePath)) {
ByteStreams.copy(resourceStream, fileStream);
fileStream.flush();
}
return tempFilePath.toString();
}
Expand Down Expand Up @@ -358,9 +361,11 @@ public void loadTrustBundleFromFileParameterValidityTest() {
NullPointerException npe = assertThrows(NullPointerException.class, () -> SpiffeUtil
.loadTrustBundleFromFile(null));
assertEquals("trustBundleFile", npe.getMessage());
NoSuchFileException nsfe = assertThrows(NoSuchFileException.class, () -> SpiffeUtil
FileNotFoundException nsfe = assertThrows(FileNotFoundException.class, () -> SpiffeUtil
.loadTrustBundleFromFile("i_do_not_exist"));
assertEquals("i_do_not_exist", nsfe.getMessage());
assertTrue(
"Did not contain expected substring: " + nsfe.getMessage(),
nsfe.getMessage().contains("i_do_not_exist"));
}
}
}
}
6 changes: 5 additions & 1 deletion gae-interop-testing/gae-jdk8/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ dependencies {
implementation libraries.junit
implementation libraries.protobuf.java
runtimeOnly libraries.netty.tcnative, libraries.netty.tcnative.classes
signature libraries.signature.java
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
}

tasks.named("compileJava").configure {
Expand Down
6 changes: 5 additions & 1 deletion gcp-csm-observability/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,9 @@ dependencies {
libraries.opentelemetry.sdk.testing,
libraries.assertj.core // opentelemetry.sdk.testing uses compileOnly for this dep

signature libraries.signature.java
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
}
6 changes: 5 additions & 1 deletion gcp-observability/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ dependencies {
exclude group: 'junit', module: 'junit'
}

signature libraries.signature.java
signature (libraries.signature.java) {
artifact {
extension = "signature"
}
}
}

configureProtoCompilation()
Loading

0 comments on commit abb3211

Please sign in to comment.