changeset 15750:7e5d2398a250

jmod code review comments. 1) format describe output similar to -listmods 2) always remove .tmp file 3) disallow classes in the unnamed package 4) remove unused message resources
author chegar
date Tue, 15 Mar 2016 20:20:38 +0000
parents 2ebb5997fb5c
children 988c192cecc5
files src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java src/jdk.jlink/share/classes/jdk/tools/jmod/resources/jmod.properties test/tools/jlink/JLinkNegativeTest.java test/tools/jmod/JmodTest.java
diffstat 4 files changed, 114 insertions(+), 61 deletions(-) [+]
line wrap: on
line diff
--- a/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java	Tue Mar 15 19:12:49 2016 +0000
+++ b/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java	Tue Mar 15 20:20:38 2016 +0000
@@ -95,7 +95,10 @@
 import jdk.internal.module.Hasher.DependencyHashes;
 import jdk.internal.module.ModuleInfoExtender;
 
+import static java.util.function.Function.identity;
 import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toMap;
 
 /**
  * Implementation for the jmod tool.
@@ -288,12 +291,10 @@
         }
     }
 
-    static <T> String toString(Set<T> set,
-                               CharSequence prefix,
-                               CharSequence suffix ) {
+    static <T> String toString(Set<T> set) {
         if (set.isEmpty()) { return ""; }
-        return set.stream().map(e -> e.toString())
-                  .collect(joining(", ", prefix, suffix));
+        return set.stream().map(e -> e.toString().toLowerCase(Locale.ROOT))
+                  .collect(joining(" "));
     }
 
     private boolean printModuleDescriptor(InputStream in)
@@ -308,58 +309,58 @@
                 if (e.getName().equals(mi)) {
                     ModuleDescriptor md = ModuleDescriptor.read(zis);
                     StringBuilder sb = new StringBuilder();
-                    sb.append("\nName:\n  " + md.toNameAndVersion());
+                    sb.append("\n").append(md.toNameAndVersion());
 
-                    Set<Requires> requires = md.requires();
+                    List<Requires> requires = md.requires().stream().sorted().collect(toList());
                     if (!requires.isEmpty()) {
-                        sb.append("\nRequires:");
-                        requires.forEach(r ->
-                                sb.append("\n  ").append(r.name())
-                                  .append(toString(r.modifiers(), " [ ", " ]")));
+                        requires.forEach(r -> {
+                                sb.append("\n  requires ");
+                                if (!r.modifiers().isEmpty())
+                                  sb.append(toString(r.modifiers())).append(" ");
+                                sb.append(r.name());
+                            });
                     }
 
-                    Set<String> s = md.uses();
-                    if (!s.isEmpty()) {
-                        sb.append("\nUses: ");
-                        s.forEach(sv -> sb.append("\n  ").append(sv));
+                    List<String> l = md.uses().stream().sorted().collect(toList());
+                    if (!l.isEmpty()) {
+                        l.forEach(sv -> sb.append("\n  uses ").append(sv));
                     }
 
-                    Set<ModuleDescriptor.Exports> exports = md.exports();
+                    List<ModuleDescriptor.Exports> exports = sortExports(md.exports());
                     if (!exports.isEmpty()) {
-                        sb.append("\nExports:");
-                        exports.forEach(sv -> sb.append("\n  ").append(sv));
+                        exports.forEach(ex -> sb.append("\n  exports ").append(ex));
+                    }
+
+                    l = md.conceals().stream().sorted().collect(toList());
+                    if (!l.isEmpty()) {
+                        l.forEach(p -> sb.append("\n  conceals ").append(p));
                     }
 
                     Map<String, ModuleDescriptor.Provides> provides = md.provides();
                     if (!provides.isEmpty()) {
-                        sb.append("\nProvides: ");
                         provides.values().forEach(p ->
-                                sb.append("\n  ").append(p.service())
+                                sb.append("\n  provides ").append(p.service())
                                   .append(" with ")
-                                  .append(toString(p.providers(), "", "")));
+                                  .append(toString(p.providers())));
                     }
 
                     Optional<String> mc = md.mainClass();
                     if (mc.isPresent())
-                        sb.append("\nMain class:\n  " + mc.get());
+                        sb.append("\n  main-class " + mc.get());
 
-                    s = md.conceals();
-                    if (!s.isEmpty()) {
-                        sb.append("\nConceals:");
-                        s.forEach(p -> sb.append("\n  ").append(p));
-                    }
+
 
                     Optional<String> osname = md.osName();
                     if (osname.isPresent())
-                        sb.append("\nOperating system name:\n  " + osname.get());
+                        sb.append("\n  operating-system-name " + osname.get());
 
                     Optional<String> osarch = md.osArch();
                     if (osarch.isPresent())
-                        sb.append("\nOperating system architecture:\n  " + osarch.get());
+                        sb.append("\n  operating-system-architecture " + osarch.get());
 
                     Optional<String> osversion = md.osVersion();
                     if (osversion.isPresent())
-                        sb.append("\nOperating system version:\n  " + osversion.get());
+                        sb.append("\n  operating-system-version " + osversion.get());
 
                     try {
                         Method m = ModuleDescriptor.class.getDeclaredMethod("hashes");
@@ -370,11 +371,10 @@
 
                         if (optHashes.isPresent()) {
                             Hasher.DependencyHashes hashes = optHashes.get();
-                            sb.append("\nHashes:");
-                            sb.append("\n  Algorithm: " + hashes.algorithm());
                             hashes.names().stream().forEach(mod ->
-                                    sb.append("\n  ").append(mod)
-                                      .append(": ").append(hashes.hashFor(mod)));
+                                    sb.append("\n  hashes ").append(mod).append(" ")
+                                      .append(hashes.algorithm()).append(" ")
+                                      .append(hashes.hashFor(mod)));
                         }
                     } catch (ReflectiveOperationException x) {
                         throw new InternalError(x);
@@ -387,6 +387,21 @@
         return false;
     }
 
+    static List<ModuleDescriptor.Exports> sortExports(Set<ModuleDescriptor.Exports> exports) {
+        Map<String,ModuleDescriptor.Exports> map =
+                exports.stream()
+                       .collect(toMap(ModuleDescriptor.Exports::source,
+                                      identity()));
+        List<String> sources = exports.stream()
+                                      .map(ModuleDescriptor.Exports::source)
+                                      .sorted()
+                                      .collect(toList());
+
+        List<ModuleDescriptor.Exports> l = new ArrayList<>();
+        sources.forEach(e -> l.add(map.get(e)));
+        return l;
+    }
+
     private boolean create() throws IOException {
         JmodFileWriter jmod = new JmodFileWriter();
 
@@ -394,10 +409,21 @@
         // when scanning the module path
         Path target = options.jmodFile;
         Path tempTarget = target.resolveSibling(target.getFileName() + ".tmp");
-        try (OutputStream out = Files.newOutputStream(tempTarget)) {
-            jmod.write(out);
+        try {
+            try (OutputStream out = Files.newOutputStream(tempTarget)) {
+                jmod.write(out);
+            }
+            Files.move(tempTarget, target);
+        } catch (Exception e) {
+            if (Files.exists(tempTarget)) {
+                try {
+                    Files.delete(tempTarget);
+                } catch (IOException ioe) {
+                    e.addSuppressed(ioe);
+                }
+            }
+            throw e;
         }
-        Files.move(tempTarget, target);
         return true;
     }
 
@@ -626,8 +652,12 @@
             int index = name.lastIndexOf(File.separatorChar);
             if (index != -1)
                 return name.substring(0, index).replace(File.separatorChar, '.');
-            else
-                return "";
+
+            if (!name.equals(MODULE_INFO)) {
+                IOException e = new IOException(name  + " in the unnamed package");
+                throw new UncheckedIOException(e);
+            }
+            return "";
         }
 
         String toPackageName(ZipEntry entry) {
--- a/src/jdk.jlink/share/classes/jdk/tools/jmod/resources/jmod.properties	Tue Mar 15 19:12:49 2016 +0000
+++ b/src/jdk.jlink/share/classes/jdk/tools/jmod/resources/jmod.properties	Tue Mar 15 20:20:38 2016 +0000
@@ -42,21 +42,17 @@
 err.invalid.version=invalid module version {0}
 err.output.must.be.specified:--output must be specified
 err.mods.must.be.specified:--mods must be specified
-err.cp.must.be.specified:--class-path must be specified
 err.modulepath.must.be.specified:--module-path must be specified when hashing dependencies
 err.invalid.main-class:invalid main-class name: {0}
 err.path.not.found=path not found: {0}
 err.path.not.valid=invalid path: {0}
 err.path.not.a.dir=path must be a directory: {0}
 err.invalid.class.path.entry=invalid class path entry: {0}
-err.dir.not.empty=not empty: {0}
 err.file.already.exists=file already exists: {0}
 err.jmod.not.found=no jmod file found: {0}
 err.bad.pattern=bad pattern {0}
 err.unknown.option=unknown option(s): {0}
 err.missing.arg=no value given for {0}
 err.internal.error=internal error: {0} {1} {2}
-err.invalid.arg.for.option=invalid argument for option: {0}
-err.option.after.class=option must be specified before classes: {0}
 err.module.descriptor.not.found=Module descriptor not found
 warn.invalid.arg=Invalid classname or pathname not exist: {0}
--- a/test/tools/jlink/JLinkNegativeTest.java	Tue Mar 15 19:12:49 2016 +0000
+++ b/test/tools/jlink/JLinkNegativeTest.java	Tue Mar 15 20:20:38 2016 +0000
@@ -197,18 +197,20 @@
         }
     }
 
-    public void testAddDefaultPackage() throws IOException {
-        String moduleName = "hacked1";
-        Path module = helper.generateModuleCompiledClasses(helper.getJmodSrcDir(), helper.getJmodClassesDir(),
-                moduleName, Arrays.asList("hacked1.Main", "A", "B"), "leaf1");
-        JImageGenerator
-                .getJModTask()
-                .addClassPath(module)
-                .jmod(helper.getJmodDir().resolve(moduleName + ".jmod"))
-                .create().assertSuccess();
-        Path image = helper.generateDefaultImage(moduleName).assertSuccess();
-        helper.checkImage(image, moduleName, null, null);
-    }
+    // Temporarily exclude; the jmod tool can no longer be used to create a jmod
+    // with a class in the unnamed package. Find another way, or remove.
+//    public void testAddDefaultPackage() throws IOException {
+//        String moduleName = "hacked1";
+//        Path module = helper.generateModuleCompiledClasses(helper.getJmodSrcDir(), helper.getJmodClassesDir(),
+//                moduleName, Arrays.asList("hacked1.Main", "A", "B"), "leaf1");
+//        JImageGenerator
+//                .getJModTask()
+//                .addClassPath(module)
+//                .jmod(helper.getJmodDir().resolve(moduleName + ".jmod"))
+//                .create().assertSuccess();
+//        Path image = helper.generateDefaultImage(moduleName).assertSuccess();
+//        helper.checkImage(image, moduleName, null, null);
+//    }
 
     public void testAddSomeTopLevelFiles() throws IOException {
         String moduleName = "hacked2";
--- a/test/tools/jmod/JmodTest.java	Tue Mar 15 19:12:49 2016 +0000
+++ b/test/tools/jmod/JmodTest.java	Tue Mar 15 20:20:38 2016 +0000
@@ -268,16 +268,16 @@
              MODS_DIR.resolve("describeFoo.jmod").toString())
              .assertSuccess()
              .resultChecker(r -> {
-                 // Expect similar output: "Name:foo,  Requires: java.base
-                 // Exports: jdk.test.foo,  Conceals: jdk.test.foo.internal"
-                 Pattern p = Pattern.compile("\\s+Name:\\s+foo\\s+Requires:\\s+java.base");
+                 // Expect similar output: "foo,  requires mandated java.base
+                 // exports jdk.test.foo,  conceals jdk.test.foo.internal"
+                 Pattern p = Pattern.compile("\\s+foo\\s+requires\\s+mandated\\s+java.base");
                  assertTrue(p.matcher(r.output).find(),
-                           "Expecting to find \"Name: foo, Requires: java.base\"" +
+                           "Expecting to find \"foo, requires java.base\"" +
                                 "in output, but did not: [" + r.output + "]");
                  p = Pattern.compile(
-                        "Exports:\\s+jdk.test.foo\\s+Conceals:\\s+jdk.test.foo.internal");
+                        "exports\\s+jdk.test.foo\\s+conceals\\s+jdk.test.foo.internal");
                  assertTrue(p.matcher(r.output).find(),
-                           "Expecting to find \"Exports: ..., Conceals: ...\"" +
+                           "Expecting to find \"exports ..., conceals ...\"" +
                                 "in output, but did not: [" + r.output + "]");
              });
     }
@@ -321,6 +321,30 @@
             );
     }
 
+    @Test
+    public void testTmpFileRemoved() throws IOException {
+        // Implementation detail: jmod tool creates <jmod-file>.tmp
+        // Ensure that it is removed in the event of a failure.
+        // The failure in this case is a class in the unnamed package.
+
+        Path jmod = MODS_DIR.resolve("testTmpFileRemoved.jmod");
+        Path tmp = MODS_DIR.resolve("testTmpFileRemoved.jmod.tmp");
+        FileUtils.deleteFileIfExistsWithRetry(jmod);
+        FileUtils.deleteFileIfExistsWithRetry(tmp);
+        String cp = EXPLODED_DIR.resolve("foo").resolve("classes") + File.pathSeparator +
+                    EXPLODED_DIR.resolve("foo").resolve("classes")
+                                .resolve("jdk").resolve("test").resolve("foo").toString();
+
+        jmod("create",
+             "--class-path", cp,
+             jmod.toString())
+             .assertFailure()
+             .resultChecker(r -> {
+                 assertContains(r.output, "unnamed package");
+                 assertTrue(Files.notExists(tmp), "Unexpected tmp file:" + tmp);
+             });
+    }
+
     // ---
 
     static boolean compileModule(String name, Path dest) throws IOException {
@@ -414,6 +438,7 @@
             this.output = output;
         }
         JmodResult assertSuccess() { assertTrue(exitCode == 0, output); return this; }
+        JmodResult assertFailure() { assertTrue(exitCode != 0, output); return this; }
         JmodResult resultChecker(Consumer<JmodResult> r) { r.accept(this); return this; }
     }