[GitHub] [maven] mthmulders commented on a diff in pull request #1129: Automatic discovery of JDK Toolchains

["mthmulders (via GitHub)" <gi...@apache.org>]

mthmulders commented on code in PR #1129:
URL: https://github.com/apache/maven/pull/1129#discussion_r1208584611


##########
maven-toolchain-builder/src/main/java/org/apache/maven/toolchain/building/DefaultToolchainsBuilder.java:
##########
@@ -54,23 +55,44 @@ public class DefaultToolchainsBuilder implements ToolchainsBuilder {
     private final MavenToolchainMerger toolchainsMerger = new MavenToolchainMerger();
     private final ToolchainsWriter toolchainsWriter;
     private final ToolchainsReader toolchainsReader;
+    private final List<ToolchainDiscoverer> toolchainDiscoverers;
 
     @Inject
-    public DefaultToolchainsBuilder(ToolchainsWriter toolchainsWriter, ToolchainsReader toolchainsReader) {
+    public DefaultToolchainsBuilder(
+            ToolchainsWriter toolchainsWriter,
+            ToolchainsReader toolchainsReader,
+            List<ToolchainDiscoverer> toolchainDiscoverers) {
         this.toolchainsWriter = toolchainsWriter;
         this.toolchainsReader = toolchainsReader;
+        this.toolchainDiscoverers = toolchainDiscoverers;
     }
 
     @Override
     public ToolchainsBuildingResult build(ToolchainsBuildingRequest request) throws ToolchainsBuildingException {
         ProblemCollector problems = ProblemCollectorFactory.newInstance(null);
 
+        PersistedToolchains discoveredToolchains = null;
+        if (toolchainDiscoverers != null) {
+            for (ToolchainDiscoverer discoverer : toolchainDiscoverers) {
+                PersistedToolchains toolchains = discoverer.discoverToolchains();
+                if (toolchains != null) {

Review Comment:
   Can't we ensure that the return type of `discoverToolchains()` is never `null`? If there are no toolchains discovered, I'd prefer an empty `PersistedToolchains` over `null`. This, too, would make the code in this method more concise I think.



##########
maven-toolchain-builder/src/main/java/org/apache/maven/toolchain/building/DefaultToolchainsBuilder.java:
##########
@@ -54,23 +55,44 @@ public class DefaultToolchainsBuilder implements ToolchainsBuilder {
     private final MavenToolchainMerger toolchainsMerger = new MavenToolchainMerger();
     private final ToolchainsWriter toolchainsWriter;
     private final ToolchainsReader toolchainsReader;
+    private final List<ToolchainDiscoverer> toolchainDiscoverers;
 
     @Inject
-    public DefaultToolchainsBuilder(ToolchainsWriter toolchainsWriter, ToolchainsReader toolchainsReader) {
+    public DefaultToolchainsBuilder(
+            ToolchainsWriter toolchainsWriter,
+            ToolchainsReader toolchainsReader,
+            List<ToolchainDiscoverer> toolchainDiscoverers) {
         this.toolchainsWriter = toolchainsWriter;
         this.toolchainsReader = toolchainsReader;
+        this.toolchainDiscoverers = toolchainDiscoverers;
     }
 
     @Override
     public ToolchainsBuildingResult build(ToolchainsBuildingRequest request) throws ToolchainsBuildingException {
         ProblemCollector problems = ProblemCollectorFactory.newInstance(null);
 
+        PersistedToolchains discoveredToolchains = null;
+        if (toolchainDiscoverers != null) {

Review Comment:
   Can't we ensure the `List<ToolchainDiscoverer>` that gets injected is never `null`? If there are no discoverers, I'd prefer an empty `List` over `null`. It would make the code in this method more concise I think.



##########
maven-toolchain-builder/src/main/java/org/apache/maven/toolchain/discovery/JavaHomeFinderBasic.java:
##########
@@ -0,0 +1,410 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF 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.
+ */
+package org.apache.maven.toolchain.discovery;
+
+import java.io.ByteArrayOutputStream;
+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.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class JavaHomeFinderBasic {
+    protected final Logger log = LoggerFactory.getLogger(getClass());
+    private final List<Supplier<? extends Set<String>>> myFinders = new ArrayList<>();
+
+    private boolean myCheckEmbeddedJava = false;
+
+    private String[] mySpecifiedPaths = new String[0];
+
+    public JavaHomeFinderBasic() {
+        myFinders.add(this::findInPATH);
+        myFinders.add(this::findInJavaHome);
+        myFinders.add(this::findInSpecifiedPaths);
+        myFinders.add(this::findJavaInstalledBySdkMan);
+        myFinders.add(this::findJavaInstalledByAsdfJava);
+        myFinders.add(this::findJavaInstalledByGradle);
+

Review Comment:
   Linux and macOS have the Homebrew package manager. It allows installing various flavours of Java, too. Would it make sense to add support for that package manager?



##########
maven-toolchain-builder/src/main/java/org/apache/maven/toolchain/discovery/JavaHomeFinder.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   The PR says "This borrows code from IntelliJ Idea (which is ASL2)" and I guess this is one of the classes that was borrowed from there (given the comment in line 44). Does this text in the license header still make sense then, or should it reflect the original code that we borrow?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org