8192945: Need stable sort for MODULES entry in the release file
authormchung
Thu, 07 Dec 2017 09:23:15 -0800
changeset 48206 8b967e200e35
parent 48205 6cd25cd7df81
child 48207 acfac57f4c35
8192945: Need stable sort for MODULES entry in the release file Reviewed-by: alanb, redestad
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ModuleSorter.java
test/jdk/tools/jlink/ModuleNamesOrderTest.java
--- a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ModuleSorter.java	Thu Dec 07 09:22:35 2017 -0800
+++ b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ModuleSorter.java	Thu Dec 07 09:23:15 2017 -0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -30,13 +30,16 @@
 import jdk.tools.jlink.plugin.ResourcePoolModuleView;
 
 import java.lang.module.ModuleDescriptor;
+import java.lang.module.ModuleDescriptor.Requires;
 import java.lang.module.ModuleDescriptor.Requires.Modifier;
 
 import java.nio.ByteBuffer;
-import java.util.Deque;
+import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedList;
+import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Stream;
@@ -45,9 +48,8 @@
  * Helper class to sort modules in topological order
  */
 public final class ModuleSorter {
-    private final Deque<ResourcePoolModule> nodes = new LinkedList<>();
-    private final Map<String, Set<ResourcePoolModule>> edges = new HashMap<>();
-    private final Deque<ResourcePoolModule> result = new LinkedList<>();
+    private final Map<ResourcePoolModule, Set<ResourcePoolModule>> graph = new HashMap<>();
+    private final List<ResourcePoolModule> result = new ArrayList<>();
 
     private final ResourcePoolModuleView moduleView;
 
@@ -69,11 +71,17 @@
 
     private ModuleSorter addModule(ResourcePoolModule module) {
         addNode(module);
-        readModuleDescriptor(module).requires().forEach(req -> {
+        // the module graph will be traversed in a stable order for
+        // the topological sort. So add the dependences in the module name order
+        readModuleDescriptor(module).requires()
+                                    .stream()
+                                    .sorted(Comparator.comparing(Requires::name))
+                                    .forEach(req ->
+        {
             ResourcePoolModule dep = moduleView.findModule(req.name()).orElse(null);
             if (dep != null) {
                 addNode(dep);
-                edges.get(module.name()).add(dep);
+                graph.get(module).add(dep);
             } else if (!req.modifiers().contains(Modifier.STATIC)) {
                 throw new PluginException(req.name() + " not found");
             }
@@ -82,22 +90,23 @@
     }
 
     private void addNode(ResourcePoolModule module) {
-        nodes.add(module);
-        edges.computeIfAbsent(module.name(), _n -> new HashSet<>());
+        graph.computeIfAbsent(module, _n -> new LinkedHashSet<>());
     }
 
+    /*
+     * The module graph will be traversed in a stable order
+     * (traversing the modules and their dependences in alphabetical order)
+     * so that it will produce the same result of a given module graph.
+     */
     private synchronized void build() {
-        if (!result.isEmpty() || nodes.isEmpty())
+        if (!result.isEmpty() || graph.isEmpty())
             return;
 
-        Deque<ResourcePoolModule> visited = new LinkedList<>();
-        Deque<ResourcePoolModule> done = new LinkedList<>();
-        ResourcePoolModule node;
-        while ((node = nodes.poll()) != null) {
-            if (!visited.contains(node)) {
-                visit(node, visited, done);
-            }
-        }
+        Set<ResourcePoolModule> visited = new HashSet<>();
+        Set<ResourcePoolModule> done = new HashSet<>();
+        graph.keySet().stream()
+             .sorted(Comparator.comparing(ResourcePoolModule::name))
+             .forEach(node -> visit(node, visited, done));
     }
 
     public Stream<ResourcePoolModule> sorted() {
@@ -106,19 +115,21 @@
     }
 
     private void visit(ResourcePoolModule node,
-                       Deque<ResourcePoolModule> visited,
-                       Deque<ResourcePoolModule> done) {
+                       Set<ResourcePoolModule> visited,
+                       Set<ResourcePoolModule> done) {
         if (visited.contains(node)) {
             if (!done.contains(node)) {
                 throw new IllegalArgumentException("Cyclic detected: " +
-                    node + " " + edges.get(node.name()));
+                    node + " " + graph.get(node));
             }
             return;
         }
+
+        // traverse the dependences of the given module which are
+        // also sorted in alphabetical order
         visited.add(node);
-        edges.get(node.name())
-             .forEach(x -> visit(x, visited, done));
+        graph.get(node).forEach(x -> visit(x, visited, done));
         done.add(node);
-        result.addLast(node);
+        result.add(node);
     }
 }
--- a/test/jdk/tools/jlink/ModuleNamesOrderTest.java	Thu Dec 07 09:22:35 2017 -0800
+++ b/test/jdk/tools/jlink/ModuleNamesOrderTest.java	Thu Dec 07 09:23:15 2017 -0800
@@ -24,14 +24,17 @@
 import java.io.File;
 import java.io.FileReader;
 import java.io.IOException;
-import java.io.PrintWriter;
-import java.io.StringWriter;
 import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
 import java.util.Properties;
 import java.util.spi.ToolProvider;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import tests.Helper;
 import tests.JImageGenerator;
+import tests.JImageGenerator.JLinkTask;
 
 /*
  * @test
@@ -44,6 +47,9 @@
  *          jdk.jlink/jdk.tools.jmod
  *          jdk.jlink/jdk.tools.jimage
  *          jdk.compiler
+ *          jdk.scripting.nashorn
+ *          jdk.scripting.nashorn.shell
+ *
  * @build tests.*
  * @run main ModuleNamesOrderTest
  */
@@ -60,12 +66,18 @@
             return;
         }
 
-        Path outputDir = helper.createNewImageDir("image8168925");
-        JImageGenerator.getJLinkTask()
-                .modulePath(helper.defaultModulePath())
-                .output(outputDir)
-                .addMods("jdk.scripting.nashorn")
-                .call().assertSuccess();
+        testDependences(helper);
+        testModulesOrder(helper);
+    }
+
+    private static List<String> modulesProperty(Path outputDir, String modulePath, String... roots)
+        throws IOException
+    {
+        JLinkTask jlinkTask = JImageGenerator.getJLinkTask()
+                                             .modulePath(modulePath)
+                                             .output(outputDir);
+        Stream.of(roots).forEach(jlinkTask::addMods);
+        jlinkTask.call().assertSuccess();
 
         File release = new File(outputDir.toString(), "release");
         if (!release.exists()) {
@@ -81,9 +93,21 @@
         if (!modules.startsWith("\"java.base ")) {
             throw new AssertionError("MODULES should start with 'java.base'");
         }
+        if (modules.charAt(0) != '"' || modules.charAt(modules.length()-1) != '"') {
+            throw new AssertionError("MODULES value should be double quoted");
+        }
 
-        if (!modules.endsWith(" jdk.scripting.nashorn\"")) {
-            throw new AssertionError("MODULES end with 'jdk.scripting.nashorn'");
+        return Stream.of(modules.substring(1, modules.length()-1).split("\\s+"))
+                     .collect(Collectors.toList());
+    }
+
+    private static void testDependences(Helper helper) throws IOException {
+        Path outputDir = helper.createNewImageDir("test");
+        List<String> modules = modulesProperty(outputDir, helper.defaultModulePath(),
+            "jdk.scripting.nashorn");
+        String last = modules.get(modules.size()-1);
+        if (!last.equals("jdk.scripting.nashorn")) {
+            throw new AssertionError("Unexpected MODULES value: " + modules);
         }
 
         checkDependency(modules, "java.logging", "java.base");
@@ -94,7 +118,23 @@
         checkDependency(modules, "jdk.scripting.nashorn", "java.scripting");
     }
 
-    private static void checkDependency(String modules, String fromMod, String toMod) {
+    /*
+     * Verify the MODULES list must be the same for the same module graph
+     */
+    private static void testModulesOrder(Helper helper) throws IOException {
+        Path image1 = helper.createNewImageDir("test1");
+        List<String> modules1 = modulesProperty(image1, helper.defaultModulePath(),
+            "jdk.scripting.nashorn", "jdk.scripting.nashorn.shell");
+        Path image2 = helper.createNewImageDir("test2");
+        List<String> modules2 = modulesProperty(image2, helper.defaultModulePath(),
+            "jdk.scripting.nashorn.shell", "jdk.scripting.nashorn");
+        if (!modules1.equals(modules2)) {
+            throw new AssertionError("MODULES should be a stable order: " +
+                modules1 + " vs " + modules2);
+        }
+    }
+
+    private static void checkDependency(List<String> modules, String fromMod, String toMod) {
         int fromModIdx = modules.indexOf(fromMod);
         if (fromModIdx == -1) {
             throw new AssertionError(fromMod + " is missing in MODULES");