From 34084c09fb3eb5de543a881378efa8c3785afb18 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Fri, 20 Dec 2024 16:35:47 -0600 Subject: [PATCH 1/7] Add arch test to detect usage of shared internal code --- all/build.gradle.kts | 65 ++++++++++----- .../all/NoSharedInternalCodeTest.java | 79 +++++++++++++++++++ 2 files changed, 126 insertions(+), 18 deletions(-) create mode 100644 all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java diff --git a/all/build.gradle.kts b/all/build.gradle.kts index 2cd16981fbb..1974b0d4e68 100644 --- a/all/build.gradle.kts +++ b/all/build.gradle.kts @@ -1,3 +1,5 @@ +import java.util.stream.Collectors + plugins { id("otel.java-conventions") } @@ -5,30 +7,13 @@ plugins { description = "OpenTelemetry All" otelJava.moduleName.set("io.opentelemetry.all") -tasks { - // We don't compile much here, just some API boundary tests. This project is mostly for - // aggregating jacoco reports and it doesn't work if this isn't at least as high as the - // highest supported Java version in any of our projects. All of our - // projects target Java 8 except :exporters:http-sender:jdk, which targets - // Java 11 - withType(JavaCompile::class) { - options.release.set(11) - } - - val testJavaVersion: String? by project - if (testJavaVersion == "8") { - test { - enabled = false - } - } -} - // Skip OWASP dependencyCheck task on test module dependencyCheck { skip = true } val testTasks = mutableListOf() +val jarTasks = mutableListOf() dependencies { rootProject.subprojects.forEach { subproject -> @@ -41,6 +26,12 @@ dependencies { subproject.tasks.withType().configureEach { testTasks.add(this) } + subproject.tasks.withType { + if (this.archiveClassifier.get().isEmpty()) { + System.out.println(this.archiveFile.get().toString()) + jarTasks.add(this) + } + } } } } @@ -48,6 +39,44 @@ dependencies { testImplementation("com.tngtech.archunit:archunit-junit5") } +val artifactsAndJarsFile = layout.buildDirectory.file("artifacts_and_jars.txt").get().asFile + +var writeArtifactsAndJars = tasks.register("writeArtifactsAndJars") { + dependsOn(jarTasks) + artifactsAndJarsFile.createNewFile() + val content = jarTasks.stream() + .filter { + !it.archiveFile.get().toString().contains("jmh") + } + .map { + it.archiveBaseName.get() + ":" + it.archiveFile.get().toString() + }.collect(Collectors.joining("\n")) + artifactsAndJarsFile.writeText(content) +} + +tasks { + // We don't compile much here, just some API boundary tests. This project is mostly for + // aggregating jacoco reports and it doesn't work if this isn't at least as high as the + // highest supported Java version in any of our projects. All of our + // projects target Java 8 except :exporters:http-sender:jdk, which targets + // Java 11 + withType(JavaCompile::class) { + options.release.set(11) + } + + val testJavaVersion: String? by project + if (testJavaVersion == "8") { + test { + enabled = false + } + } else { + test { + dependsOn(writeArtifactsAndJars) + environment("ARTIFACTS_AND_JARS", artifactsAndJarsFile.absolutePath) + } + } +} + // https://docs.gradle.org/current/samples/sample_jvm_multi_project_with_code_coverage.html val sourcesPath by configurations.creating { diff --git a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java new file mode 100644 index 00000000000..ae2fb4caf30 --- /dev/null +++ b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java @@ -0,0 +1,79 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.all; + +import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses; + +import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.core.domain.JavaClass; +import com.tngtech.archunit.core.domain.JavaClasses; +import com.tngtech.archunit.core.importer.ClassFileImporter; +import com.tngtech.archunit.lang.syntax.elements.ClassesShouldConjunction; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Set; +import java.util.jar.JarFile; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class NoSharedInternalCodeTest { + + private static final String OTEL_BASE_PACKAGE = "io.opentelemetry"; + private static final Logger logger = Logger.getLogger(NoSharedInternalCodeTest.class.getName()); + + @ParameterizedTest + @MethodSource("artifactsAndJars") + void noSharedInternalCode(String artifactId, String absolutePath) throws IOException { + JavaClasses artifactClasses = + new ClassFileImporter().importJar(new JarFile(new File(absolutePath))); + + Set artifactOtelPackages = + artifactClasses.stream() + .map(JavaClass::getPackageName) + .filter(packageName -> packageName.startsWith(OTEL_BASE_PACKAGE)) + .collect(Collectors.toSet()); + + ClassesShouldConjunction noSharedInternalCodeRule = + noClasses() + .that() + .resideInAnyPackage(artifactOtelPackages.toArray(new String[0])) + .should() + .dependOnClassesThat( + new DescribedPredicate<>("are in internal modules of other opentelemetry artifacts") { + @Override + public boolean test(JavaClass javaClass) { + String packageName = javaClass.getPackageName(); + return packageName.startsWith(OTEL_BASE_PACKAGE) + && packageName.contains(".internal") + && !artifactOtelPackages.contains(packageName); + } + }); + + // TODO: when all shared internal code is removed, remove the catch block and fail when detected + try { + noSharedInternalCodeRule.check(artifactClasses); + } catch (AssertionError e) { + logger.log(Level.WARNING, "Internal shared code detected for: " + artifactId + "\n" + e.getMessage() + "\n"); + } + } + + private static Stream artifactsAndJars() throws IOException { + List lines = Files.readAllLines(Path.of(System.getenv("ARTIFACTS_AND_JARS"))); + return lines.stream() + .map(line -> { + String[] parts = line.split(":"); + return Arguments.of(parts[0], parts[1]); + }); + } +} From 5dbf3b67340721dd9d7fae7b1ff048c04f68c878 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Sat, 21 Dec 2024 12:44:36 -0600 Subject: [PATCH 2/7] Remove debug print statement --- all/build.gradle.kts | 1 - 1 file changed, 1 deletion(-) diff --git a/all/build.gradle.kts b/all/build.gradle.kts index 1974b0d4e68..a119716fd79 100644 --- a/all/build.gradle.kts +++ b/all/build.gradle.kts @@ -28,7 +28,6 @@ dependencies { } subproject.tasks.withType { if (this.archiveClassifier.get().isEmpty()) { - System.out.println(this.archiveFile.get().toString()) jarTasks.add(this) } } From dec4710debb573e06e98945e5ecdd9f9502ae0a4 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Thu, 2 Jan 2025 12:12:26 -0600 Subject: [PATCH 3/7] Debug --- .github/workflows/build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 7244872f30f..1b0a1f05551 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -74,6 +74,7 @@ jobs: -PtestJavaVersion=${{ matrix.test-java-version }} "-Porg.gradle.java.installations.paths=${{ steps.setup-java-test.outputs.path }}" "-Porg.gradle.java.installations.auto-download=false" + --info env: # JMH-based tests run only if this environment variable is set to true RUN_JMH_BASED_TESTS: ${{ matrix.jmh-based-tests }} From bcc692a479f67410439f280027fdb5aa26624a3c Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Thu, 2 Jan 2025 12:16:06 -0600 Subject: [PATCH 4/7] stacktrace --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1b0a1f05551..b1dfe9a6322 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -74,7 +74,7 @@ jobs: -PtestJavaVersion=${{ matrix.test-java-version }} "-Porg.gradle.java.installations.paths=${{ steps.setup-java-test.outputs.path }}" "-Porg.gradle.java.installations.auto-download=false" - --info + --stacktrace env: # JMH-based tests run only if this environment variable is set to true RUN_JMH_BASED_TESTS: ${{ matrix.jmh-based-tests }} From 7bbf0590bb6ba6dc7d235c30f1f2ab16b3ec508a Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Thu, 2 Jan 2025 12:26:37 -0600 Subject: [PATCH 5/7] mkdirs --- .github/workflows/build.yml | 1 - all/build.gradle.kts | 5 ++-- .../all/NoSharedInternalCodeTest.java | 30 +++++++++++-------- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b1dfe9a6322..7244872f30f 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -74,7 +74,6 @@ jobs: -PtestJavaVersion=${{ matrix.test-java-version }} "-Porg.gradle.java.installations.paths=${{ steps.setup-java-test.outputs.path }}" "-Porg.gradle.java.installations.auto-download=false" - --stacktrace env: # JMH-based tests run only if this environment variable is set to true RUN_JMH_BASED_TESTS: ${{ matrix.jmh-based-tests }} diff --git a/all/build.gradle.kts b/all/build.gradle.kts index a119716fd79..9ab4075611a 100644 --- a/all/build.gradle.kts +++ b/all/build.gradle.kts @@ -42,14 +42,15 @@ val artifactsAndJarsFile = layout.buildDirectory.file("artifacts_and_jars.txt"). var writeArtifactsAndJars = tasks.register("writeArtifactsAndJars") { dependsOn(jarTasks) + artifactsAndJarsFile.parentFile.mkdirs() artifactsAndJarsFile.createNewFile() val content = jarTasks.stream() .filter { !it.archiveFile.get().toString().contains("jmh") } .map { - it.archiveBaseName.get() + ":" + it.archiveFile.get().toString() - }.collect(Collectors.joining("\n")) + it.archiveBaseName.get() + ":" + it.archiveFile.get().toString() + }.collect(Collectors.joining("\n")) artifactsAndJarsFile.writeText(content) } diff --git a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java index ae2fb4caf30..51f62196dab 100644 --- a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java +++ b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java @@ -50,28 +50,32 @@ void noSharedInternalCode(String artifactId, String absolutePath) throws IOExcep .resideInAnyPackage(artifactOtelPackages.toArray(new String[0])) .should() .dependOnClassesThat( - new DescribedPredicate<>("are in internal modules of other opentelemetry artifacts") { - @Override - public boolean test(JavaClass javaClass) { - String packageName = javaClass.getPackageName(); - return packageName.startsWith(OTEL_BASE_PACKAGE) - && packageName.contains(".internal") - && !artifactOtelPackages.contains(packageName); - } - }); + new DescribedPredicate<>( + "are in internal modules of other opentelemetry artifacts") { + @Override + public boolean test(JavaClass javaClass) { + String packageName = javaClass.getPackageName(); + return packageName.startsWith(OTEL_BASE_PACKAGE) + && packageName.contains(".internal") + && !artifactOtelPackages.contains(packageName); + } + }); // TODO: when all shared internal code is removed, remove the catch block and fail when detected try { noSharedInternalCodeRule.check(artifactClasses); } catch (AssertionError e) { - logger.log(Level.WARNING, "Internal shared code detected for: " + artifactId + "\n" + e.getMessage() + "\n"); + logger.log( + Level.WARNING, + "Internal shared code detected for: " + artifactId + "\n" + e.getMessage() + "\n"); } } private static Stream artifactsAndJars() throws IOException { - List lines = Files.readAllLines(Path.of(System.getenv("ARTIFACTS_AND_JARS"))); - return lines.stream() - .map(line -> { + List lines = Files.readAllLines(Path.of(System.getenv("ARTIFACTS_AND_JARS"))); + return lines.stream() + .map( + line -> { String[] parts = line.split(":"); return Arguments.of(parts[0], parts[1]); }); From 96831a94e7991e637743ceda2719e585e5f45045 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Thu, 2 Jan 2025 12:52:56 -0600 Subject: [PATCH 6/7] add println to debug on windows --- .../java/io/opentelemetry/all/NoSharedInternalCodeTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java index 51f62196dab..d691802c914 100644 --- a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java +++ b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java @@ -71,11 +71,13 @@ public boolean test(JavaClass javaClass) { } } + @SuppressWarnings("SystemOut") // TODO: remove private static Stream artifactsAndJars() throws IOException { List lines = Files.readAllLines(Path.of(System.getenv("ARTIFACTS_AND_JARS"))); return lines.stream() .map( line -> { + System.out.println(line); String[] parts = line.split(":"); return Arguments.of(parts[0], parts[1]); }); From 96f172a9ea18fd08037e4fafd36e896dc71c1d23 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Thu, 2 Jan 2025 12:58:01 -0600 Subject: [PATCH 7/7] Fix on windows --- .../java/io/opentelemetry/all/NoSharedInternalCodeTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java index d691802c914..d9ca7f99b74 100644 --- a/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java +++ b/all/src/test/java/io/opentelemetry/all/NoSharedInternalCodeTest.java @@ -71,14 +71,12 @@ public boolean test(JavaClass javaClass) { } } - @SuppressWarnings("SystemOut") // TODO: remove private static Stream artifactsAndJars() throws IOException { List lines = Files.readAllLines(Path.of(System.getenv("ARTIFACTS_AND_JARS"))); return lines.stream() .map( line -> { - System.out.println(line); - String[] parts = line.split(":"); + String[] parts = line.split(":", 2); return Arguments.of(parts[0], parts[1]); }); }