Ignore module-info in jar hell checks (#33011)

* Ignore module-info in JarHell checks
* Add unit test
* integration test to test that jarhell is ran with precommit

Author: alpar-t

buildSrc/build.gradle

@@ -176,7 +176,6 @@ if (project != rootProject) {
       it.tasks.matching { it.name == 'publishNebulaPublicationToLocalTestRepository'}
     }
     exclude "**/*Tests.class"
-    include "**/*IT.class"
     testClassesDirs = sourceSets.test.output.classesDirs
     classpath = sourceSets.test.runtimeClasspath
     inputs.dir(file("src/testKit"))

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/JarHellTask.groovy

@@ -22,8 +22,8 @@ package org.elasticsearch.gradle.precommit
 import com.github.jengelman.gradle.plugins.shadow.ShadowPlugin
 import org.elasticsearch.gradle.LoggedExec
 import org.gradle.api.file.FileCollection
+import org.gradle.api.tasks.Classpath
 import org.gradle.api.tasks.OutputFile
-
 /**
  * Runs CheckJarHell on a classpath.
  */
@@ -35,9 +35,13 @@ public class JarHellTask extends LoggedExec {
      * inputs (ie the jars/class files).
      */
     @OutputFile
-    File successMarker = new File(project.buildDir, 'markers/jarHell')
+    File successMarker
+
+    @Classpath
+    FileCollection classpath
 
     public JarHellTask() {
+        successMarker = new File(project.buildDir, 'markers/jarHell-' + getName())
         project.afterEvaluate {
             FileCollection classpath = project.sourceSets.test.runtimeClasspath
             if (project.plugins.hasPlugin(ShadowPlugin)) {

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

@@ -31,7 +31,7 @@ class PrecommitTasks {
 
     /** Adds a precommit task, which depends on non-test verification tasks. */
     public static Task create(Project project, boolean includeDependencyLicenses) {
-        Configuration forbiddenApisConfiguration = project.configurations.create("forbiddenApisCliJar")
+        project.configurations.create("forbiddenApisCliJar")
         project.dependencies {
             forbiddenApisCliJar ('de.thetaphi:forbiddenapis:2.5')
         }
@@ -43,7 +43,7 @@ class PrecommitTasks {
             project.tasks.create('forbiddenPatterns', ForbiddenPatternsTask.class),
             project.tasks.create('licenseHeaders', LicenseHeadersTask.class),
             project.tasks.create('filepermissions', FilePermissionsTask.class),
-            project.tasks.create('jarHell', JarHellTask.class),
+            configureJarHell(project),
             configureThirdPartyAudit(project)
         ]
 
@@ -80,6 +80,12 @@ class PrecommitTasks {
         return project.tasks.create(precommitOptions)
     }
 
+    private static Task configureJarHell(Project project) {
+        Task task = project.tasks.create('jarHell', JarHellTask.class)
+        task.classpath = project.sourceSets.test.runtimeClasspath
+        return task
+    }
+
     private static Task configureThirdPartyAudit(Project project) {
         ThirdPartyAuditTask thirdPartyAuditTask = project.tasks.create('thirdPartyAudit', ThirdPartyAuditTask.class)
         ExportElasticsearchBuildResourcesTask buildResources = project.tasks.getByName('buildResources')

buildSrc/src/test/java/org/elasticsearch/gradle/BuildExamplePluginsIT.java

@@ -153,17 +153,4 @@ private Path writeBuildScript(String script) {
         }
     }
 
-    private String getLocalTestRepoPath() {
-        String property = System.getProperty("test.local-test-repo-path");
-        Objects.requireNonNull(property, "test.local-test-repo-path not passed to tests");
-        File file = new File(property);
-        assertTrue("Expected " + property + " to exist, but it did not!", file.exists());
-        if (File.separator.equals("\\")) {
-            // Use / on Windows too, the build script is not happy with \
-            return file.getAbsolutePath().replace(File.separator, "/");
-        } else {
-            return file.getAbsolutePath();
-        }
-    }
-
 }

buildSrc/src/test/java/org/elasticsearch/gradle/ExportElasticsearchBuildResourcesTaskIT.java

@@ -40,7 +40,7 @@ public void testUpToDateWithSourcesConfigured() {
             .withArguments("buildResources", "-s", "-i")
             .withPluginClasspath()
             .build();
-        assertTaskSuccessfull(result, ":buildResources");
+        assertTaskSuccessful(result, ":buildResources");
         assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle.xml");
         assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle_suppressions.xml");
 
@@ -61,8 +61,8 @@ public void testImplicitTaskDependencyCopy() {
             .withPluginClasspath()
             .build();
 
-        assertTaskSuccessfull(result, ":buildResources");
-        assertTaskSuccessfull(result, ":sampleCopyAll");
+        assertTaskSuccessful(result, ":buildResources");
+        assertTaskSuccessful(result, ":sampleCopyAll");
         assertBuildFileExists(result, PROJECT_NAME, "sampleCopyAll/checkstyle.xml");
         // This is a side effect of compile time reference
         assertBuildFileExists(result, PROJECT_NAME, "sampleCopyAll/checkstyle_suppressions.xml");
@@ -75,7 +75,7 @@ public void testImplicitTaskDependencyInputFileOfOther() {
             .withPluginClasspath()
             .build();
 
-        assertTaskSuccessfull(result, ":sample");
+        assertTaskSuccessful(result, ":sample");
         assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle.xml");
         assertBuildFileExists(result, PROJECT_NAME, "build-tools-exported/checkstyle_suppressions.xml");
     }

buildSrc/src/test/java/org/elasticsearch/gradle/precommit/JarHellTaskIT.java

@@ -0,0 +1,42 @@
+package org.elasticsearch.gradle.precommit;
+
+import org.elasticsearch.gradle.test.GradleIntegrationTestCase;
+import org.gradle.testkit.runner.BuildResult;
+import org.gradle.testkit.runner.GradleRunner;
+
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+public class JarHellTaskIT extends GradleIntegrationTestCase {
+
+    public void testJarHellDetected() {
+        BuildResult result = GradleRunner.create()
+            .withProjectDir(getProjectDir("jarHell"))
+            .withArguments("clean", "precommit", "-s", "-Dlocal.repo.path=" + getLocalTestRepoPath())
+            .withPluginClasspath()
+            .buildAndFail();
+
+        assertTaskFailed(result, ":jarHell");
+        assertOutputContains(
+            result.getOutput(),
+            "Exception in thread \"main\" java.lang.IllegalStateException: jar hell!",
+            "class: org.apache.logging.log4j.Logger"
+        );
+    }
+
+}

buildSrc/src/test/java/org/elasticsearch/gradle/test/GradleIntegrationTestCase.java

@@ -9,6 +9,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.Objects;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -66,15 +67,24 @@ protected void assertOutputDoesNotContain(String output, String... lines) {
         }
     }
 
-    protected void assertTaskSuccessfull(BuildResult result, String taskName) {
+    protected void assertTaskFailed(BuildResult result, String taskName) {
+        assertTaskOutcome(result, taskName, TaskOutcome.FAILED);
+    }
+
+    protected void assertTaskSuccessful(BuildResult result, String taskName) {
+        assertTaskOutcome(result, taskName, TaskOutcome.SUCCESS);
+    }
+
+    private void assertTaskOutcome(BuildResult result, String taskName, TaskOutcome taskOutcome) {
         BuildTask task = result.task(taskName);
         if (task == null) {
-            fail("Expected task `" + taskName + "` to be successful, but it did not run");
+            fail("Expected task `" + taskName + "` to be " + taskOutcome +", but it did not run" +
+                "\n\nOutput is:\n" + result.getOutput());
         }
         assertEquals(
             "Expected task to be successful but it was: " + task.getOutcome() +
-                "\n\nOutput is:\n" + result.getOutput() ,
-            TaskOutcome.SUCCESS,
+                taskOutcome + "\n\nOutput is:\n" + result.getOutput() ,
+            taskOutcome,
             task.getOutcome()
         );
     }
@@ -109,4 +119,17 @@ protected void assertBuildFileDoesNotExists(BuildResult result, String projectNa
             Files.exists(absPath)
         );
     }
+
+    protected String getLocalTestRepoPath() {
+        String property = System.getProperty("test.local-test-repo-path");
+        Objects.requireNonNull(property, "test.local-test-repo-path not passed to tests");
+        File file = new File(property);
+        assertTrue("Expected " + property + " to exist, but it did not!", file.exists());
+        if (File.separator.equals("\\")) {
+            // Use / on Windows too, the build script is not happy with \
+            return file.getAbsolutePath().replace(File.separator, "/");
+        } else {
+            return file.getAbsolutePath();
+        }
+    }
 }

buildSrc/src/testKit/jarHell/build.gradle

@@ -0,0 +1,29 @@
+plugins {
+    id 'java'
+    id 'elasticsearch.build'
+}
+
+dependencyLicenses.enabled = false
+dependenciesInfo.enabled = false
+forbiddenApisMain.enabled = false
+forbiddenApisTest.enabled = false
+thirdPartyAudit.enabled = false
+namingConventions.enabled = false
+ext.licenseFile = file("$buildDir/dummy/license")
+ext.noticeFile = file("$buildDir/dummy/notice")
+
+repositories {
+    mavenCentral()
+    repositories {
+        maven {
+            url System.getProperty("local.repo.path")
+        }
+    }
+}
+
+dependencies {
+    // Needed for the JarHell task
+    testCompile ("org.elasticsearch.test:framework:${versions.elasticsearch}")
+    // causes jar hell with local sources
+    compile "org.apache.logging.log4j:log4j-api:${versions.log4j}"
+}

buildSrc/src/testKit/jarHell/src/main/java/org/apache/logging/log4j/Logger.java

@@ -0,0 +1,7 @@
+package org.apache.logging.log4j;
+
+// Jar Hell !
+public class Logger {
+
+}
+

libs/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java

@@ -255,6 +255,10 @@ public static void checkJavaVersion(String resource, String targetVersion) {
     }
 
     private static void checkClass(Map<String, Path> clazzes, String clazz, Path jarpath) {
+        if (clazz.equals("module-info") || clazz.endsWith(".module-info")) {
+            // Ignore jigsaw module descriptions
+            return;
+        }
         Path previous = clazzes.put(clazz, jarpath);
         if (previous != null) {
             if (previous.equals(jarpath)) {

libs/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java

@@ -76,6 +76,28 @@ public void testDifferentJars() throws Exception {
         }
     }
 
+    public void testModuleInfo() throws Exception {
+        Path dir = createTempDir();
+        JarHell.checkJarHell(
+            asSet(
+                makeJar(dir, "foo.jar", null, "module-info.class"),
+                makeJar(dir, "bar.jar", null, "module-info.class")
+            ),
+            logger::debug
+        );
+    }
+
+    public void testModuleInfoPackage() throws Exception {
+        Path dir = createTempDir();
+        JarHell.checkJarHell(
+            asSet(
+                makeJar(dir, "foo.jar", null, "foo/bar/module-info.class"),
+                makeJar(dir, "bar.jar", null, "foo/bar/module-info.class")
+            ),
+            logger::debug
+        );
+    }
+
     public void testDirsOnClasspath() throws Exception {
         Path dir1 = createTempDir();
         Path dir2 = createTempDir();