8178404: jlink --suggest-providers should list providers from observable modules

Reviewed-by: alanb

jdk/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Jlink.java

@@ -158,7 +158,7 @@ public final class Jlink {
          *
          * @param output Output directory, must not exist.
          * @param modulepaths Modules paths
-         * @param modules Root modules to resolve
+         * @param modules The possibly-empty set of root modules to resolve
          * @param limitmods Limit the universe of observable modules
          * @param endian Jimage byte order. Native order by default
          */
@@ -170,13 +170,10 @@ public final class Jlink {
             if (Objects.requireNonNull(modulepaths).isEmpty()) {
                 throw new IllegalArgumentException("Empty module path");
             }
-            if (Objects.requireNonNull(modules).isEmpty()) {
-                throw new IllegalArgumentException("Empty modules");
-            }
 
             this.output = output;
             this.modulepaths = modulepaths;
-            this.modules = modules;
+            this.modules = Objects.requireNonNull(modules);
             this.limitmods = Objects.requireNonNull(limitmods);
             this.endian = Objects.requireNonNull(endian);
             this.finder = moduleFinder();

jdk/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java

@@ -40,7 +40,20 @@ import java.nio.ByteOrder;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -331,11 +344,6 @@ public class JlinkTask {
     // the token for "all modules on the module path"
     private static final String ALL_MODULE_PATH = "ALL-MODULE-PATH";
     private JlinkConfiguration initJlinkConfig() throws BadArgs {
-        if (options.addMods.isEmpty()) {
-            throw taskHelper.newBadArgs("err.mods.must.be.specified", "--add-modules")
-                .showUsage(true);
-        }
-
         Set<String> roots = new HashSet<>();
         for (String mod : options.addMods) {
             if (mod.equals(ALL_MODULE_PATH)) {
@@ -369,6 +377,10 @@ public class JlinkTask {
         if (options.output == null) {
             throw taskHelper.newBadArgs("err.output.must.be.specified").showUsage(true);
         }
+        if (options.addMods.isEmpty()) {
+            throw taskHelper.newBadArgs("err.mods.must.be.specified", "--add-modules")
+                            .showUsage(true);
+        }
 
         // First create the image provider
         ImageProvider imageProvider = createImageProvider(config,
@@ -430,7 +442,7 @@ public class JlinkTask {
             // print modules to be linked in
             cf.modules().stream()
               .sorted(Comparator.comparing(ResolvedModule::name))
-              .forEach(rm -> log.format("module %s (%s)%n",
+              .forEach(rm -> log.format("%s %s%n",
                                         rm.name(), rm.reference().location().get()));
 
             // print provider info
@@ -505,16 +517,22 @@ public class JlinkTask {
 
     /*
      * Returns a map of each service type to the modules that use it
+     * It will include services that are provided by a module but may not used
+     * by any of the observable modules.
      */
     private static Map<String, Set<String>> uses(Set<ModuleReference> modules) {
         // collects the services used by the modules and print uses
-        Map<String, Set<String>> uses = new HashMap<>();
+        Map<String, Set<String>> services = new HashMap<>();
         modules.stream()
                .map(ModuleReference::descriptor)
-               .forEach(md -> md.uses().forEach(s ->
+               .forEach(md -> {
-                   uses.computeIfAbsent(s, _k -> new HashSet<>()).add(md.name()))
+                   // include services that may not be used by any observable modules
-               );
+                   md.provides().forEach(p ->
-        return uses;
+                       services.computeIfAbsent(p.service(), _k -> new HashSet<>()));
+                   md.uses().forEach(s -> services.computeIfAbsent(s, _k -> new HashSet<>())
+                                                  .add(md.name()));
+               });
+        return services;
     }
 
     private static void printProviders(PrintWriter log,
@@ -524,17 +542,16 @@ public class JlinkTask {
     }
 
     /*
-     * Prints the providers that are used by the services specified in
+     * Prints the providers that are used by the specified services.
-     * the given modules.
      *
-     * The specified uses maps a service type name to the modules
+     * The specified services maps a service type name to the modules
-     * using the service type and that may or may not be present
+     * using the service type which may be empty if no observable module uses
-     * the given modules.
+     * that service.
      */
     private static void printProviders(PrintWriter log,
                                        String header,
                                        Set<ModuleReference> modules,
-                                       Map<String, Set<String>> uses) {
+                                       Map<String, Set<String>> serviceToUses) {
         if (modules.isEmpty())
             return;
 
@@ -544,7 +561,7 @@ public class JlinkTask {
             .map(ModuleReference::descriptor)
             .forEach(md -> {
                 md.provides().stream()
-                  .filter(p -> uses.containsKey(p.service()))
+                  .filter(p -> serviceToUses.containsKey(p.service()))
                   .forEach(p -> providers.computeIfAbsent(p.service(), _k -> new HashSet<>())
                                          .add(md));
             });
@@ -564,11 +581,18 @@ public class JlinkTask {
                  .forEach(md ->
                      md.provides().stream()
                        .filter(p -> p.service().equals(service))
-                       .forEach(p -> log.format("  module %s provides %s, used by %s%n",
+                       .forEach(p -> {
-                                                md.name(), p.service(),
+                           String usedBy;
-                                                uses.get(p.service()).stream()
+                           if (serviceToUses.get(p.service()).isEmpty()) {
+                               usedBy = "not used by any observable module";
+                           } else {
+                               usedBy = serviceToUses.get(p.service()).stream()
                                             .sorted()
-                                                    .collect(Collectors.joining(","))))
+                                            .collect(Collectors.joining(",", "used by ", ""));
+                           }
+                           log.format("  %s provides %s %s%n",
+                                      md.name(), p.service(), usedBy);
+                       })
                  );
             });
     }
@@ -589,25 +613,21 @@ public class JlinkTask {
 
         ModuleFinder finder = config.finder();
         if (args.isEmpty()) {
-            // print providers used by the modules resolved without service binding
+            // print providers used by the observable modules without service binding
-            Configuration cf = config.resolve();
+            Set<ModuleReference> mrefs = finder.findAll();
-            Set<ModuleReference> mrefs = cf.modules().stream()
-                .map(ResolvedModule::reference)
-                .collect(Collectors.toSet());
-
             // print uses of the modules that would be linked into the image
             mrefs.stream()
                  .sorted(Comparator.comparing(mref -> mref.descriptor().name()))
                  .forEach(mref -> {
                      ModuleDescriptor md = mref.descriptor();
-                     log.format("module %s located (%s)%n", md.name(),
+                     log.format("%s %s%n", md.name(),
                                 mref.location().get());
                      md.uses().stream().sorted()
                        .forEach(s -> log.format("    uses %s%n", s));
                  });
 
             String msg = String.format("%n%s:", taskHelper.getMessage("suggested.providers.header"));
-            printProviders(log, msg, finder.findAll(), uses(mrefs));
+            printProviders(log, msg, mrefs, uses(mrefs));
 
         } else {
             // comma-separated service types, if specified
@@ -620,17 +640,22 @@ public class JlinkTask {
                                     .anyMatch(names::contains))
                 .collect(Collectors.toSet());
 
-            // the specified services may or may not be in the modules that
+            // find the modules that uses the specified services
-            // would be linked in.  So find uses declared in all observable modules
+            Map<String, Set<String>> uses = new HashMap<>();
-            Map<String, Set<String>> uses = uses(finder.findAll());
+            names.forEach(s -> uses.computeIfAbsent(s, _k -> new HashSet<>()));
+            finder.findAll().stream()
+                  .map(ModuleReference::descriptor)
+                  .forEach(md -> md.uses().stream()
+                                   .filter(names::contains)
+                                   .forEach(s -> uses.get(s).add(md.name())));
 
-            // check if any name given on the command line are unused service
+            // check if any name given on the command line are not provided by any module
             mrefs.stream()
                  .flatMap(mref -> mref.descriptor().provides().stream()
                                       .map(ModuleDescriptor.Provides::service))
                  .forEach(names::remove);
             if (!names.isEmpty()) {
-                log.println(taskHelper.getMessage("warn.unused.services",
+                log.println(taskHelper.getMessage("warn.provider.notfound",
                                                   toString(names)));
             }
 

jdk/src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties

@@ -57,12 +57,12 @@ main.opt.launcher=\
 \                                        if specified  
 
 main.opt.bind-services=\
-\      --bind-services                   Do full service binding
+\      --bind-services                   Link in service provider modules and\n\
+\                                        their dependences
 
 main.opt.suggest-providers=\
-\      --suggest-providers [<name>,...]  Suggest providers of services used by\n\
+\      --suggest-providers [<name>,...]  Suggest providers that implement the\n\
-\                                        the modules that would be linked, or\n\
+\                                        given service types from the module path
-\                                        of the given service types
 
 main.command.files=\
 \      @<filename>                       Read options from file
@@ -138,7 +138,7 @@ err.signing=signed modular JAR {0} is currently not supported,\
 warn.signing=WARNING: signed modular JAR {0} is currently not supported
 warn.invalid.arg=invalid classname or pathname not exist: {0}
 warn.split.package=package {0} defined in {1} {2}
-warn.unused.services=Services specified in --suggest-providers not used: {0}
+warn.provider.notfound=No provider found for service specified to --suggest-providers: {0}
 no.suggested.providers=--bind-services option is specified. No additional providers suggested.
 suggested.providers.header=Suggested providers
 providers.header=Providers

jdk/test/tools/jlink/bindservices/BindServices.java

@@ -33,7 +33,6 @@ import java.util.spi.ToolProvider;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import static jdk.testlibrary.Asserts.assertTrue;
 import static jdk.testlibrary.ProcessTools.*;
 
 import org.testng.annotations.BeforeTest;
@@ -114,7 +113,7 @@ public class BindServices {
                   "--module-path", MODULE_PATH,
                   "--add-modules", "m1",
                   "--bind-services",
-                  "--limit-modules", "m1,m2,m3,java.base");
+                  "--limit-modules", "m1,m2,m3");
 
         testImage(dir, "m1", "m2", "m3");
     }
@@ -131,16 +130,18 @@ public class BindServices {
                       "--add-modules", "m1",
                       "--bind-services",
                       "--verbose",
-                      "--limit-modules", "m1,m2,m3,java.base").output();
+                      "--limit-modules", "m1,m2,m3").output();
 
         List<String> expected = List.of(
-            "module m1 (" + MODS_DIR.resolve("m1").toUri().toString() + ")",
+            "m1 " + MODS_DIR.resolve("m1").toUri().toString(),
-            "module m2 (" + MODS_DIR.resolve("m2").toUri().toString() + ")",
+            "m2 " + MODS_DIR.resolve("m2").toUri().toString(),
-            "module m3 (" + MODS_DIR.resolve("m3").toUri().toString() + ")",
+            "m3 " + MODS_DIR.resolve("m3").toUri().toString(),
-            "module m1 provides p1.S, used by m1",
+            "java.base provides java.nio.file.spi.FileSystemProvider used by java.base",
-            "module m2 provides p1.S, used by m1",
+            "m1 provides p1.S used by m1",
-            "module m2 provides p2.T, used by m2",
+            "m2 provides p1.S used by m1",
-            "module m3 provides p2.T, used by m2"
+            "m2 provides p2.T used by m2",
+            "m3 provides p2.T used by m2",
+            "m3 provides p3.S not used by any observable module"
         );
 
         assertTrue(output.containsAll(expected));

jdk/test/tools/jlink/bindservices/SuggestProviders.java

@@ -33,8 +33,6 @@ import java.util.spi.ToolProvider;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import static jdk.testlibrary.Asserts.assertTrue;
-
 import org.testng.annotations.BeforeTest;
 import org.testng.annotations.Test;
 import static org.testng.Assert.*;
@@ -85,15 +83,8 @@ public class SuggestProviders {
         }
     }
 
-    @Test
-    public void suggestProviders() throws Throwable {
-        if (!hasJmods()) return;
-
-        List<String> output = JLink.run("--module-path", MODULE_PATH,
-                                        "--add-modules", "m1",
-                                        "--suggest-providers").output();
     // check a subset of services used by java.base
-        List<String> expected = List.of(
+    private final List<String> JAVA_BASE_USES = List.of(
         "uses java.lang.System$LoggerFinder",
         "uses java.net.ContentHandlerFactory",
         "uses java.net.spi.URLStreamHandlerProvider",
@@ -103,16 +94,91 @@ public class SuggestProviders {
         "uses java.nio.file.spi.FileSystemProvider",
         "uses java.nio.file.spi.FileTypeDetector",
         "uses java.security.Provider",
-            "uses java.util.spi.ToolProvider",
+        "uses java.util.spi.ToolProvider"
-            "uses p1.S",
-            "module jdk.charsets provides java.nio.charset.spi.CharsetProvider, used by java.base",
-            "module jdk.compiler provides java.util.spi.ToolProvider, used by java.base",
-            "module jdk.jlink provides java.util.spi.ToolProvider, used by java.base",
-            "module m1 provides p1.S, used by m1",
-            "module m2 provides p1.S, used by m1"
     );
 
-        assertTrue(output.containsAll(expected));
+    private final List<String> JAVA_BASE_PROVIDERS = List.of(
+        "java.base provides java.nio.file.spi.FileSystemProvider used by java.base"
+    );
+
+    private final List<String> SYSTEM_PROVIDERS = List.of(
+        "jdk.charsets provides java.nio.charset.spi.CharsetProvider used by java.base",
+        "jdk.compiler provides java.util.spi.ToolProvider used by java.base",
+        "jdk.compiler provides javax.tools.JavaCompiler used by java.compiler",
+        "jdk.jlink provides jdk.tools.jlink.plugin.Plugin used by jdk.jlink",
+        "jdk.jlink provides java.util.spi.ToolProvider used by java.base"
+    );
+
+    private final List<String> APP_USES = List.of(
+        "uses p1.S",
+        "uses p2.T"
+    );
+
+    private final List<String> APP_PROVIDERS = List.of(
+        "m1 provides p1.S used by m1",
+        "m2 provides p1.S used by m1",
+        "m2 provides p2.T used by m2",
+        "m3 provides p2.T used by m2",
+        "m3 provides p3.S not used by any observable module"
+    );
+
+    @Test
+    public void suggestProviders() throws Throwable {
+        if (!hasJmods()) return;
+
+        List<String> output = JLink.run("--module-path", MODULE_PATH,
+                                        "--suggest-providers").output();
+
+        Stream<String> uses =
+            Stream.concat(JAVA_BASE_USES.stream(), APP_USES.stream());
+        Stream<String> providers =
+            Stream.concat(SYSTEM_PROVIDERS.stream(), APP_PROVIDERS.stream());
+
+        assertTrue(output.containsAll(Stream.concat(uses, providers)
+                                            .collect(Collectors.toList())));
+    }
+
+    /**
+     * find providers from the observable modules and --add-modules has no
+     * effect on the suggested providers
+     */
+    @Test
+    public void observableModules() throws Throwable {
+        if (!hasJmods()) return;
+
+        List<String> output = JLink.run("--module-path", MODULE_PATH,
+                                        "--add-modules", "m1",
+                                        "--suggest-providers").output();
+
+        Stream<String> uses =
+            Stream.concat(JAVA_BASE_USES.stream(), Stream.of("uses p1.S"));
+        Stream<String> providers =
+            Stream.concat(SYSTEM_PROVIDERS.stream(), APP_PROVIDERS.stream());
+
+        assertTrue(output.containsAll(Stream.concat(uses, providers)
+                                            .collect(Collectors.toList())));
+    }
+
+    /**
+     * find providers from the observable modules with --limit-modules
+     */
+    @Test
+    public void limitModules() throws Throwable {
+        if (!hasJmods()) return;
+
+        List<String> output = JLink.run("--module-path", MODULE_PATH,
+                                        "--limit-modules", "m1",
+                                        "--suggest-providers").output();
+
+        Stream<String> uses =
+            Stream.concat(JAVA_BASE_USES.stream(), Stream.of("uses p1.S"));
+        Stream<String> providers =
+            Stream.concat(JAVA_BASE_PROVIDERS.stream(),
+                          Stream.of("m1 provides p1.S used by m1")
+        );
+
+        assertTrue(output.containsAll(Stream.concat(uses, providers)
+                                            .collect(Collectors.toList())));
     }
 
     @Test
@@ -121,20 +187,17 @@ public class SuggestProviders {
 
         List<String> output =
             JLink.run("--module-path", MODULE_PATH,
-                      "--add-modules", "m1",
                       "--suggest-providers",
-                      "java.nio.charset.spi.CharsetProvider,p1.S,p2.T").output();
+                      "java.nio.charset.spi.CharsetProvider,p1.S").output();
 
         System.out.println(output);
-        List<String> expected = List.of(
+        Stream<String> expected = Stream.concat(
-            "module jdk.charsets provides java.nio.charset.spi.CharsetProvider, used by java.base",
+            Stream.of("jdk.charsets provides java.nio.charset.spi.CharsetProvider used by java.base"),
-            "module m1 provides p1.S, used by m1",
+            Stream.of("m1 provides p1.S used by m1",
-            "module m2 provides p1.S, used by m1",
+                      "m2 provides p1.S used by m1")
-            "module m2 provides p2.T, used by m2",
-            "module m3 provides p2.T, used by m2"
         );
 
-        assertTrue(output.containsAll(expected));
+        assertTrue(output.containsAll(expected.collect(Collectors.toList())));
     }
 
     @Test
@@ -143,15 +206,30 @@ public class SuggestProviders {
 
         List<String> output =
             JLink.run("--module-path", MODULE_PATH,
-                "--add-modules", "m1",
+                      "--suggest-providers",
+                      "p3.S").output();
+
+        List<String> expected = List.of(
+            "m3 provides p3.S not used by any observable module"
+        );
+        assertTrue(output.containsAll(expected));
+
+        // should not print other services m3 provides
+        assertFalse(output.contains("m3 provides p2.T used by m2"));
+    }
+
+    @Test
+    public void nonExistentService() throws Throwable {
+        if (!hasJmods()) return;
+
+        List<String> output =
+            JLink.run("--module-path", MODULE_PATH,
                       "--suggest-providers",
                       "nonExistentType").output();
 
-        System.out.println(output);
         List<String> expected = List.of(
-            "Services specified in --suggest-providers not used: nonExistentType"
+            "No provider found for service specified to --suggest-providers: nonExistentType"
         );
-
         assertTrue(output.containsAll(expected));
     }
 
@@ -161,9 +239,7 @@ public class SuggestProviders {
 
         List<String> output =
             JLink.run("--module-path", MODULE_PATH,
-                      "--add-modules", "m1",
                       "--bind-services",
-                      "--limit-modules", "m1,m2,m3,java.base",
                       "--suggest-providers").output();
 
         String expected = "--bind-services option is specified. No additional providers suggested.";

jdk/test/tools/jlink/bindservices/src/m3/module-info.java

@@ -24,4 +24,5 @@
 module m3 {
     requires m2;
     provides p2.T with p3.Impl;
+    provides p3.S with p3.MyProvider;
 }

jdk/test/tools/jlink/bindservices/src/m3/p3/MyProvider.java

@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package p3;
+
+public class MyProvider implements S {
+    public void run() {
+    }
+}

jdk/test/tools/jlink/bindservices/src/m3/p3/S.java

@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package p3;
+
+public interface S {
+    void run();
+}