Merge pull request #25181 Filter locations without java executable

Backport of #23643

Fixes: #24439

Co-authored-by: Xavier Arias Seguí <xavier.arias.segui@gradle.com>

subprojects/docs/src/docs/userguide/jvm/toolchains.adoc

@@ -193,6 +193,12 @@ JVM auto-detection knows how to work with:
 
 Among the set of all detected JRE/JDK installations, one will be picked according to the <<sec:precedence,Toolchain Precedence Rules>>.
 
+[NOTE]
+====
+Whether you are using toolchain auto-detection or you are configuring <<sec:custom_loc>>, installations that are
+non-existing or without a `bin/java` executable will be ignored with a warning, but they won't generate an error.
+====
+
 [[sub:disable_auto_detect]]
 === How to disable auto-detection
 

subprojects/jvm-services/src/integTest/groovy/org/gradle/jvm/toolchain/InvalidJvmInstallationReportingIntegrationTest.groovy

@@ -19,6 +19,7 @@ package org.gradle.jvm.toolchain
 import org.gradle.integtests.fixtures.AbstractIntegrationSpec
 import org.gradle.integtests.fixtures.AvailableJavaHomes
 import org.gradle.integtests.fixtures.executer.GradleContextualExecuter
+import org.gradle.internal.os.OperatingSystem
 import spock.lang.IgnoreIf
 import spock.lang.TempDir
 
@@ -33,10 +34,8 @@ class InvalidJvmInstallationReportingIntegrationTest extends AbstractIntegration
 
         // Use two invalid installation paths to verify that the implementation reports a warning on each of them
         // (as opposed to e.g. reporting only the first one)
-        def invalidJdkHome1 = new File(temporaryDirectory, "jdk1")
-        def invalidJdkHome2 = new File(temporaryDirectory, "jdk2")
-        assert(invalidJdkHome1.mkdirs())
-        assert(invalidJdkHome2.mkdirs())
+        def invalidJdkHome1 = createInvalidJdkHome("jdk1")
+        def invalidJdkHome2 = createInvalidJdkHome("jdk2")
 
         // The builds should trigger toolchains discovery; here it's done from different subprojects in order to test
         // that the JVM installation detection is cached during a build
@@ -90,4 +89,13 @@ class InvalidJvmInstallationReportingIntegrationTest extends AbstractIntegration
     private int countMatches(String pattern, String text) {
         return Pattern.compile(Pattern.quote(pattern)).matcher(text).count
     }
+
+    private File createInvalidJdkHome(String name) {
+        def jdkHome = new File(temporaryDirectory, name)
+        def executableName = OperatingSystem.current().getExecutableName("java")
+        def executable = new File(jdkHome, "bin/$executableName")
+        assert executable.mkdirs()
+        executable.createNewFile()
+        jdkHome
+    }
 }

subprojects/jvm-services/src/main/java/org/gradle/jvm/toolchain/internal/JavaInstallationRegistry.java

@@ -80,6 +80,7 @@ private Set<InstallationLocation> collectInstallations(List<InstallationSupplier
             .flatMap(Set::stream)
             .filter(this::installationExists)
             .map(this::canonicalize)
+            .filter(this::installationHasExecutable)
             .filter(distinctByKey(InstallationLocation::getLocation))
             .collect(Collectors.toSet());
     }
@@ -97,6 +98,14 @@ boolean installationExists(InstallationLocation installationLocation) {
         return true;
     }
 
+    boolean installationHasExecutable(InstallationLocation installationLocation) {
+        if (!hasJavaExecutable(installationLocation.getLocation())) {
+            logger.warn("Path for java installation {} does not contain a java executable", installationLocation.getDisplayName());
+            return false;
+        }
+        return true;
+    }
+
     private InstallationLocation canonicalize(InstallationLocation location) {
         final File file = location.getLocation();
         try {

subprojects/jvm-services/src/test/groovy/org/gradle/jvm/toolchain/internal/JavaInstallationRegistryTest.groovy

@@ -27,6 +27,8 @@ class JavaInstallationRegistryTest extends Specification {
 
     def "registry keeps track of initial installations"() {
         when:
+        createExecutable(tempFolder)
+
         def registry = newRegistry(tempFolder)
 
         then:
@@ -36,6 +38,8 @@ class JavaInstallationRegistryTest extends Specification {
 
     def "registry filters non-unique locations"() {
         when:
+        createExecutable(tempFolder)
+
         def registry = newRegistry(tempFolder, tempFolder)
 
         then:
@@ -44,6 +48,8 @@ class JavaInstallationRegistryTest extends Specification {
 
     def "duplicates are detected using canonical form"() {
         given:
+        createExecutable(tempFolder)
+
         def registry = newRegistry(tempFolder, new File(tempFolder, "/."))
 
         when:
@@ -55,8 +61,11 @@ class JavaInstallationRegistryTest extends Specification {
 
     def "can be initialized with suppliers"() {
         given:
+        createExecutable(tempFolder)
         def tmpDir2 = createTempDir()
+        createExecutable(tmpDir2)
         def tmpDir3 = createTempDir()
+        createExecutable(tmpDir3)
 
         when:
         def registry = newRegistry(tempFolder, tmpDir2, tmpDir3)
@@ -67,6 +76,8 @@ class JavaInstallationRegistryTest extends Specification {
 
     def "list of installations is cached"() {
         given:
+        createExecutable(tempFolder)
+
         def registry = newRegistry(tempFolder)
 
         when:
@@ -80,7 +91,8 @@ class JavaInstallationRegistryTest extends Specification {
     def "normalize installations to account for macOS folder layout"() {
         given:
         def expectedHome = new File(tempFolder, "Contents/Home")
-        assert expectedHome.mkdirs()
+        createExecutable(expectedHome, OperatingSystem.MAC_OS)
+
         def registry = new JavaInstallationRegistry([forDirectory(tempFolder)], new TestBuildOperationExecutor(), OperatingSystem.MAC_OS)
 
         when:
@@ -93,10 +105,7 @@ class JavaInstallationRegistryTest extends Specification {
     def "normalize installations to account for standalone jre"() {
         given:
         def expectedHome = new File(tempFolder, "jre")
-        assert expectedHome.mkdirs()
-        def jreBinFolder = new File(expectedHome, "bin")
-        assert jreBinFolder.mkdir()
-        assert new File(jreBinFolder, OperatingSystem.current().getExecutableName( "java")).createNewFile()
+        createExecutable(expectedHome)
 
         def registry = new JavaInstallationRegistry([forDirectory(tempFolder)], new TestBuildOperationExecutor(), OperatingSystem.current())
 
@@ -110,8 +119,10 @@ class JavaInstallationRegistryTest extends Specification {
     def "skip path normalization on non-osx systems"() {
         given:
         def rootWithMacOsLayout = createTempDir()
+        createExecutable(rootWithMacOsLayout, OperatingSystem.LINUX)
         def expectedHome = new File(rootWithMacOsLayout, "Contents/Home")
         assert expectedHome.mkdirs()
+
         def registry = new JavaInstallationRegistry([forDirectory(rootWithMacOsLayout)], new TestBuildOperationExecutor(), OperatingSystem.LINUX)
 
         when:
@@ -155,6 +166,21 @@ class JavaInstallationRegistryTest extends Specification {
         '/foo/file' | true   | false     | false | 'Path for java installation {} points to a file, not a directory'
     }
 
+    def "warns and filters installations without java executable"() {
+        given:
+        def logger = Mock(Logger)
+        def tempFolder = createTempDir()
+        def registry = JavaInstallationRegistry.withLogger([forDirectory(tempFolder)], logger, new TestBuildOperationExecutor())
+        def logOutput = "Path for java installation {} does not contain a java executable"
+
+        when:
+        def installations = registry.listInstallations()
+
+        then:
+        installations.isEmpty()
+        1 * logger.warn(logOutput, "'" + tempFolder + "' (testSource)")
+    }
+
     InstallationSupplier forDirectory(File directory) {
         { it -> Collections.singleton(new InstallationLocation(directory, "testSource")) }
     }
@@ -165,6 +191,13 @@ class JavaInstallationRegistryTest extends Specification {
         file.canonicalFile
     }
 
+    void createExecutable(File javaHome, os = OperatingSystem.current()) {
+        def executableName = os.getExecutableName("java")
+        def executable = new File(javaHome, "bin/$executableName")
+        assert executable.parentFile.mkdirs()
+        executable.createNewFile()
+    }
+
     private JavaInstallationRegistry newRegistry(File... location) {
         new JavaInstallationRegistry(location.collect { forDirectory(it) }, new TestBuildOperationExecutor(), OperatingSystem.current())
     }

subprojects/platform-jvm/src/test/groovy/org/gradle/jvm/toolchain/internal/JavaToolchainQueryServiceTest.groovy

@@ -423,6 +423,9 @@ class JavaToolchainQueryServiceTest extends Specification {
             boolean installationExists(InstallationLocation installationLocation) {
                 return true
             }
+            boolean installationHasExecutable(InstallationLocation installationLocation) {
+                return true
+            }
         }
         registry
     }