8168836: Minor clean up on warning/error messages on --add-exports and --add-reads
authormchung
Mon, 19 Dec 2016 15:01:14 -0800
changeset 42774 74bcf37d15d8
parent 42773 f32797dd8806
child 42775 a45fa7082c81
child 42940 0d1409532a41
8168836: Minor clean up on warning/error messages on --add-exports and --add-reads Reviewed-by: alanb
jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java
jdk/src/java.base/share/classes/jdk/internal/module/ModulePatcher.java
jdk/src/java.base/share/native/libjli/java.c
jdk/test/lib/testlibrary/ModuleSourceBuilder.java
jdk/test/tools/launcher/modules/addexports/AddExportsTestWarningError.java
jdk/test/tools/launcher/modules/addreads/AddReadsTestWarningError.java
jdk/test/tools/launcher/modules/patch/basic/PatchTestWarningError.java
--- a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java	Mon Dec 19 14:52:19 2016 -0800
+++ b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java	Mon Dec 19 15:01:14 2016 -0800
@@ -306,6 +306,13 @@
                         fail(name + ": cannot be loaded from application module path");
                 }
             }
+
+            // check if module specified in --patch-module is present
+            for (String mn: patcher.patchedModules()) {
+                if (!cf.findModule(mn).isPresent()) {
+                    warnUnknownModule(PATCH_MODULE, mn);
+                }
+            }
         }
 
         // if needed check that there are no split packages in the set of
@@ -481,7 +488,7 @@
             String mn = e.getKey();
             Optional<Module> om = bootLayer.findModule(mn);
             if (!om.isPresent()) {
-                warn("Unknown module: " + mn);
+                warnUnknownModule(ADD_READS, mn);
                 continue;
             }
             Module m = om.get();
@@ -495,7 +502,7 @@
                     if (om.isPresent()) {
                         Modules.addReads(m, om.get());
                     } else {
-                        warn("Unknown module: " + name);
+                        warnUnknownModule(ADD_READS, name);
                     }
                 }
             }
@@ -527,24 +534,25 @@
                                                Map<String, List<String>> map,
                                                boolean opens)
     {
+        String option = opens ? ADD_OPENS : ADD_EXPORTS;
         for (Map.Entry<String, List<String>> e : map.entrySet()) {
 
             // the key is $MODULE/$PACKAGE
             String key = e.getKey();
             String[] s = key.split("/");
             if (s.length != 2)
-                fail("Unable to parse as <module>/<package>: " + key);
+                fail(unableToParse(option,  "<module>/<package>", key));
 
             String mn = s[0];
             String pn = s[1];
             if (mn.isEmpty() || pn.isEmpty())
-                fail("Module and package name must be specified: " + key);
+                fail(unableToParse(option,  "<module>/<package>", key));
 
             // The exporting module is in the boot layer
             Module m;
             Optional<Module> om = bootLayer.findModule(mn);
             if (!om.isPresent()) {
-                warn("Unknown module: " + mn);
+                warnUnknownModule(option, mn);
                 continue;
             }
 
@@ -566,7 +574,7 @@
                     if (om.isPresent()) {
                         other = om.get();
                     } else {
-                        warn("Unknown module: " + name);
+                        warnUnknownModule(option, name);
                         continue;
                     }
                 }
@@ -610,24 +618,30 @@
 
             int pos = value.indexOf('=');
             if (pos == -1)
-                fail("Unable to parse as <module>=<value>: " + value);
+                fail(unableToParse(option(prefix), "<module>=<value>", value));
             if (pos == 0)
-                fail("Missing module name in: " + value);
+                fail(unableToParse(option(prefix), "<module>=<value>", value));
 
             // key is <module> or <module>/<package>
             String key = value.substring(0, pos);
 
             String rhs = value.substring(pos+1);
             if (rhs.isEmpty())
-                fail("Unable to parse as <module>=<value>: " + value);
+                fail(unableToParse(option(prefix), "<module>=<value>", value));
 
             // value is <module>(,<module>)* or <file>(<pathsep><file>)*
             if (!allowDuplicates && map.containsKey(key))
-                fail(key + " specified more than once");
+                fail(key + " specified more than once in " + option(prefix));
             List<String> values = map.computeIfAbsent(key, k -> new ArrayList<>());
+            int ntargets = 0;
             for (String s : rhs.split(regex)) {
-                if (s.length() > 0) values.add(s);
+                if (s.length() > 0) {
+                    values.add(s);
+                    ntargets++;
+                }
             }
+            if (ntargets == 0)
+                fail("Target must be specified: " + option(prefix) + " " + value);
 
             index++;
             value = getAndRemoveProperty(prefix + index);
@@ -689,6 +703,42 @@
         System.err.println("WARNING: " + m);
     }
 
+    static void warnUnknownModule(String option, String mn) {
+        warn("Unknown module: " + mn + " specified in " + option);
+    }
+
+    static String unableToParse(String option, String text, String value) {
+        return "Unable to parse " +  option + " " + text + ": " + value;
+    }
+
+    private static final String ADD_MODULES  = "--add-modules";
+    private static final String ADD_EXPORTS  = "--add-exports";
+    private static final String ADD_OPENS    = "--add-opens";
+    private static final String ADD_READS    = "--add-reads";
+    private static final String PATCH_MODULE = "--patch-module";
+
+
+    /*
+     * Returns the command-line option name corresponds to the specified
+     * system property prefix.
+     */
+    static String option(String prefix) {
+        switch (prefix) {
+            case "jdk.module.addexports.":
+                return ADD_EXPORTS;
+            case "jdk.module.addopens.":
+                return ADD_OPENS;
+            case "jdk.module.addreads.":
+                return ADD_READS;
+            case "jdk.module.patch.":
+                return PATCH_MODULE;
+            case "jdk.module.addmods.":
+                return ADD_MODULES;
+            default:
+                throw new IllegalArgumentException(prefix);
+        }
+    }
+
     static class PerfCounters {
 
         static PerfCounter systemModulesTime
--- a/jdk/src/java.base/share/classes/jdk/internal/module/ModulePatcher.java	Mon Dec 19 14:52:19 2016 -0800
+++ b/jdk/src/java.base/share/classes/jdk/internal/module/ModulePatcher.java	Mon Dec 19 15:01:14 2016 -0800
@@ -175,6 +175,12 @@
         return map.isEmpty();
     }
 
+    /*
+     * Returns the names of the patched modules.
+     */
+    Set<String> patchedModules() {
+        return map.keySet();
+    }
 
     /**
      * A ModuleReader that reads resources from a patched module.
--- a/jdk/src/java.base/share/native/libjli/java.c	Mon Dec 19 14:52:19 2016 -0800
+++ b/jdk/src/java.base/share/native/libjli/java.c	Mon Dec 19 15:01:14 2016 -0800
@@ -573,6 +573,17 @@
            JLI_StrCmp(name, "--patch-module") == 0;
 }
 
+static jboolean
+IsLongFormModuleOption(const char* name) {
+    return JLI_StrCCmp(name, "--module-path=") == 0 ||
+           JLI_StrCCmp(name, "--upgrade-module-path=") == 0 ||
+           JLI_StrCCmp(name, "--add-modules=") == 0 ||
+           JLI_StrCCmp(name, "--limit-modules=") == 0 ||
+           JLI_StrCCmp(name, "--add-exports=") == 0 ||
+           JLI_StrCCmp(name, "--add-reads=") == 0 ||
+           JLI_StrCCmp(name, "--patch-module=") == 0;
+}
+
 /*
  * Test if the given name has a white space option.
  */
@@ -1236,7 +1247,7 @@
         char *option = NULL;
         char *value = NULL;
         int kind = GetOpt(&argc, &argv, &option, &value);
-        jboolean has_arg = value != NULL;
+        jboolean has_arg = value != NULL && JLI_StrLen(value) > 0;
 
 /*
  * Option to set main entry point
@@ -1285,19 +1296,13 @@
 /*
  * Error missing argument
  */
-        } else if (!has_arg && IsWhiteSpaceOption(arg)) {
-            if (JLI_StrCmp(arg, "--module-path") == 0 ||
-                JLI_StrCmp(arg, "-p") == 0 ||
-                JLI_StrCmp(arg, "--upgrade-module-path") == 0) {
-                REPORT_ERROR (has_arg, ARG_ERROR4, arg);
-            } else if (JLI_StrCmp(arg, "--add-modules") == 0 ||
-                       JLI_StrCmp(arg, "--limit-modules") == 0 ||
-                       JLI_StrCmp(arg, "--add-exports") == 0 ||
-                       JLI_StrCmp(arg, "--add-opens") == 0 ||
-                       JLI_StrCmp(arg, "--add-reads") == 0 ||
-                       JLI_StrCmp(arg, "--patch-module") == 0) {
-                REPORT_ERROR (has_arg, ARG_ERROR6, arg);
-            }
+        } else if (!has_arg && (JLI_StrCmp(arg, "--module-path") == 0 ||
+                                JLI_StrCmp(arg, "-p") == 0 ||
+                                JLI_StrCmp(arg, "--upgrade-module-path") == 0)) {
+            REPORT_ERROR (has_arg, ARG_ERROR4, arg);
+
+        } else if (!has_arg && (IsModuleOption(arg) || IsLongFormModuleOption(arg))) {
+            REPORT_ERROR (has_arg, ARG_ERROR6, arg);
 /*
  * The following cases will cause the argument parsing to stop
  */
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/lib/testlibrary/ModuleSourceBuilder.java	Mon Dec 19 15:01:14 2016 -0800
@@ -0,0 +1,127 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+import java.io.BufferedWriter;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Stream;
+
+import static org.testng.Assert.assertTrue;
+
+/**
+ * Utility class for creating test modules.
+ */
+public class ModuleSourceBuilder {
+    private static String MODULE_INFO_JAVA = "module-info.java";
+    private static Pattern MODULE_PATTERN =
+        Pattern.compile("module\\s+((?:\\w+\\.)*)");
+    private static Pattern PACKAGE_PATTERN =
+                       Pattern.compile("package\\s+(((?:\\w+\\.)*)(?:\\w+))");
+    private static Pattern CLASS_PATTERN =
+          Pattern.compile("(?:public\\s+)?(?:class|enum|interface)\\s+(\\w+)");
+
+    private final Path dir;
+    public ModuleSourceBuilder(Path dir) {
+        this.dir = dir;
+    }
+
+    /**
+     * Create java source files of the given module
+     */
+    public void writeJavaFiles(String module, String moduleInfoJava, String... contents)
+        throws IOException
+    {
+        Path msrc = dir.resolve(module);
+        new JavaSource(moduleInfoJava).write(msrc);
+        for (String c : contents) {
+            new JavaSource(c).write(msrc);
+        }
+    }
+
+    /**
+     * Compile the module to the given destination.
+     */
+    public void compile(String module, Path dest, String... options)
+        throws IOException
+    {
+        Path msrc = dir.resolve(module);
+        Stream<String> args =
+            Stream.concat(Arrays.stream(options),
+                          Stream.of("--module-source-path",
+                                    dir.toString()));
+        assertTrue(CompilerUtils.compile(msrc, dest, args.toArray(String[]::new)),
+                   "Fail to compile " + module);
+    }
+
+    static class JavaSource {
+        final String source;
+        JavaSource(String source) {
+            this.source = source;
+        }
+
+        /**
+         * Writes the source code to a file in a specified directory.
+         * @param dir the directory
+         * @throws IOException if there is a problem writing the file
+         */
+        public void write(Path dir) throws IOException {
+            Path file = dir.resolve(getJavaFileNameFromSource(source));
+            Files.createDirectories(file.getParent());
+            try (BufferedWriter out = Files.newBufferedWriter(file)) {
+                out.write(source.replace("\n", System.lineSeparator()));
+            }
+        }
+
+        /**
+         * Extracts the Java file name from the class declaration.
+         * This method is intended for simple files and uses regular expressions,
+         * so comments matching the pattern can make the method fail.
+         */
+        static String getJavaFileNameFromSource(String source) {
+            String packageName = null;
+
+            Matcher matcher = MODULE_PATTERN.matcher(source);
+            if (matcher.find())
+                return MODULE_INFO_JAVA;
+
+            matcher = PACKAGE_PATTERN.matcher(source);
+            if (matcher.find())
+                packageName = matcher.group(1).replace(".", "/");
+
+            matcher = CLASS_PATTERN.matcher(source);
+            if (matcher.find()) {
+                String className = matcher.group(1) + ".java";
+                return (packageName == null) ? className : packageName + "/" + className;
+            } else if (packageName != null) {
+                return packageName + "/package-info.java";
+            } else {
+                throw new Error("Could not extract the java class " +
+                    "name from the provided source");
+            }
+        }
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/tools/launcher/modules/addexports/AddExportsTestWarningError.java	Mon Dec 19 15:01:14 2016 -0800
@@ -0,0 +1,234 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+/**
+ * @test
+ * @bug 8168836
+ * @summary Basic argument validation for --add-exports
+ * @library /lib/testlibrary
+ * @modules jdk.compiler
+ * @build AddExportsTestWarningError CompilerUtils ModuleSourceBuilder
+ * @build jdk.testlibrary.*
+ * @run testng AddExportsTestWarningError
+ */
+
+import java.io.BufferedOutputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.stream.Stream;
+
+import jdk.testlibrary.OutputAnalyzer;
+import static jdk.testlibrary.ProcessTools.*;
+
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import static org.testng.Assert.*;
+
+
+@Test
+public class AddExportsTestWarningError {
+
+    private static final Path MODS_DIR = Paths.get("mods");
+    private static final Path SRC_DIR = Paths.get("src");
+    private static final String M1_MAIN = "m1/p1.C1";
+    private static final String M3_MAIN = "m3/p3.C3";
+
+    @BeforeTest
+    public void setup() throws Exception {
+        ModuleSourceBuilder builder = new ModuleSourceBuilder(SRC_DIR);
+        builder.writeJavaFiles("m1",
+            "module m1 { }",
+            "package p1; public class C1 { " +
+                "    public static void main(String... args) {}" +
+                "}");
+
+        builder.writeJavaFiles("m2",
+            "module m2 { requires m1; exports p2; }",
+            "package p2; public class C2 {  private p1.C1 c1; }");
+
+        builder.writeJavaFiles("m3",
+            "module m3 { requires m2; }",
+            "package p3; class C3 { " +
+                "    p1.C1 c; " +
+                "    public static void main(String... args) { new p2.C2(); }" +
+                "}");
+
+        builder.compile("m1", MODS_DIR);
+        builder.compile("m2", MODS_DIR, "--add-exports", "m1/p1=m2");
+        builder.compile("m3", MODS_DIR, "--add-exports", "m1/p1=m3");
+    }
+
+
+    @DataProvider(name = "goodcases")
+    public Object[][] goodCases() {
+        return new Object[][]{
+
+            // empty items
+            { "m1/p1=,m2,m3",       null },
+            { "m1/p1=m2,,m3",       null },
+            { "m1/p1=m2,m3,",       null },
+
+            // duplicates
+            { "m1/p1=m2,m2,m3,,",   null },
+
+        };
+    }
+
+
+    @Test(dataProvider = "goodcases")
+    public void test(String value, String ignore) throws Exception {
+        testNoWarning(value);
+    }
+
+
+    @DataProvider(name = "illFormedAddExports")
+    public Object[][] illFormedAddExports() {
+        return new Object[][]{
+            { "m1",         "Unable to parse --add-exports <module>=<value>: m1"},
+
+            // missing source part
+            { "=m2",        "Unable to parse --add-exports <module>=<value>: =m2"},
+            { "/=m2",       "Unable to parse --add-exports <module>/<package>: /" },
+            { "m1=m2",      "Unable to parse --add-exports <module>/<package>: m1" },
+            { "/p1=m2",     "Unable to parse --add-exports <module>/<package>: /p1" },
+            { "m1p1=m2",    "Unable to parse --add-exports <module>/<package>: m1p1" },
+
+            // empty list, missing target
+            { "m1/p1=",     "Unable to parse --add-exports <module>=<value>: m1/p1=" },
+            { "m1/p1=,,",   "Target must be specified: --add-exports m1/p1=,," },
+        };
+    }
+
+    @Test(dataProvider = "illFormedAddExports")
+    public void testIllFormedAddExports(String value, String msg) throws Exception {
+        testError(value, msg);
+    }
+
+
+    @DataProvider(name = "unknownNames")
+    public Object[][] unknownNames() {
+        return new Object[][]{
+
+            // source not found
+            {"DoesNotExist/p=m1",  "WARNING: Unknown module: DoesNotExist specified in --add-exports"},
+            {"m1/DoesNotExist=m2", "WARNING: package DoesNotExist not in m1"},
+
+            // target not found
+            {"m1/p1=DoesNotExist", "WARNING: Unknown module: DoesNotExist specified in --add-exports"},
+
+            // bad names
+            {"m*/p1=m2",           "WARNING: Unknown module: m* specified in --add-exports"},
+            {"m1/p!=m2",           "WARNING: package p! not in m1"},
+            {"m1/p1=m!",           "WARNING: Unknown module: m! specified in --add-exports"},
+
+        };
+    }
+
+
+    @Test(dataProvider = "unknownNames")
+    public void testUnknownNames(String value, String msg) throws Exception {
+        testWarning(value, msg);
+    }
+
+
+    @DataProvider(name = "missingArguments")
+    public Object[][] missingArguments() {
+        return new Object[][]{
+            { new String[] { "--add-exports" },
+                "Error: --add-exports requires modules to be specified"},
+
+            { new String[] { "--add-exports=" },
+                "Error: --add-exports= requires modules to be specified" },
+
+            { new String[] { "--add-exports", "" },
+                "Error: --add-exports requires modules to be specified"}
+
+        };
+    }
+
+
+    @Test(dataProvider = "missingArguments")
+    public void testMissingArguments(String[] options, String msg) throws Exception {
+        String[] args = Stream.concat(Arrays.stream(options),
+                                      Stream.of("-version"))
+                              .toArray(String[]::new);
+        int exitValue = executeTestJava(args)
+            .outputTo(System.out)
+            .errorTo(System.out)
+            .shouldContain(msg)
+            .getExitValue();
+
+        assertTrue(exitValue != 0);
+    }
+
+     private void testWarning(String value, String msg) throws Exception {
+        int exitValue =
+            executeTestJava("--add-exports", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", M1_MAIN)
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .shouldContain(msg)
+                .getExitValue();
+
+        assertTrue(exitValue == 0);
+    }
+
+    private void testError(String value, String msg) throws Exception {
+        int exitValue =
+            executeTestJava("--add-exports", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", M1_MAIN)
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .shouldContain(msg)
+                .getExitValue();
+
+        assertTrue(exitValue != 0);
+    }
+
+    private void testNoWarning(String value) throws Exception {
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        PrintStream ps = new PrintStream(new BufferedOutputStream(baos));
+        OutputAnalyzer outputAnalyzer =
+            executeTestJava("--add-exports", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", M3_MAIN)
+                .outputTo(ps)
+                .errorTo(ps);
+
+        assertTrue(outputAnalyzer.getExitValue() == 0);
+
+        System.out.println(baos.toString());
+        String[] output = baos.toString().split("\\R");
+        assertFalse(Arrays.stream(output)
+                          .filter(s -> !s.matches("WARNING: Module name .* may soon be illegal"))
+                          .filter(s -> s.startsWith("WARNING:"))
+                          .findAny().isPresent());
+
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/tools/launcher/modules/addreads/AddReadsTestWarningError.java	Mon Dec 19 15:01:14 2016 -0800
@@ -0,0 +1,224 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+/**
+ * @test
+ * @bug 8168836
+ * @summary  Basic argument validation for --add-reads
+ * @library /lib/testlibrary
+ * @modules jdk.compiler
+ * @build AddReadsTestWarningError CompilerUtils ModuleSourceBuilder
+ * @build jdk.testlibrary.*
+ * @run testng AddReadsTestWarningError
+ */
+
+import java.io.BufferedOutputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.stream.Stream;
+
+import jdk.testlibrary.OutputAnalyzer;
+import static jdk.testlibrary.ProcessTools.*;
+
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import static org.testng.Assert.*;
+
+
+@Test
+public class AddReadsTestWarningError {
+
+    private static final Path MODS_DIR = Paths.get("mods");
+    private static final Path SRC_DIR = Paths.get("src");
+    private static final String M1_MAIN = "m1/p1.C1";
+    private static final String M4_MAIN = "m4/p4.C4";
+
+    @BeforeTest
+    public void setup() throws Exception {
+        ModuleSourceBuilder builder = new ModuleSourceBuilder(SRC_DIR);
+        builder.writeJavaFiles("m1",
+            "module m1 { requires m4; }",
+            "package p1; public class C1 { " +
+            "    public static void main(String... args) {" +
+            "        p2.C2 c2 = new p2.C2();" +
+            "        p3.C3 c3 = new p3.C3();" +
+            "    }" +
+            "}"
+        );
+
+        builder.writeJavaFiles("m2",
+            "module m2 { exports p2; }",
+            "package p2; public class C2 { }"
+        );
+
+        builder.writeJavaFiles("m3",
+            "module m3 { exports p3; }",
+            "package p3; public class C3 { }"
+        );
+
+        builder.writeJavaFiles("m4",
+            "module m4 { requires m2; requires m3; }",
+            "package p4; public class C4 { " +
+            "    public static void main(String... args) {}" +
+            "}"
+        );
+
+        builder.compile("m2", MODS_DIR);
+        builder.compile("m3", MODS_DIR);
+        builder.compile("m4", MODS_DIR);
+        builder.compile("m1", MODS_DIR, "--add-reads", "m1=m2,m3");
+    }
+
+
+    @DataProvider(name = "goodcases")
+    public Object[][] goodCases() {
+        return new Object[][]{
+            // empty items
+            { "m1=,m2,m3",       null },
+            { "m1=m2,,m3",       null },
+            { "m1=m2,m3,",       null },
+
+            // duplicates
+            { "m1=m2,m2,m3,,",  null },
+
+        };
+    }
+
+
+    @Test(dataProvider = "goodcases")
+    public void test(String value, String ignore) throws Exception {
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        PrintStream ps = new PrintStream(new BufferedOutputStream(baos));
+        OutputAnalyzer outputAnalyzer =
+            executeTestJava("--add-reads", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", M1_MAIN)
+                .outputTo(ps)
+                .errorTo(ps);
+
+        assertTrue(outputAnalyzer.getExitValue() == 0);
+
+        System.out.println(baos.toString());
+        String[] output = baos.toString().split("\\R");
+        assertFalse(Arrays.stream(output)
+                          .filter(s -> !s.matches("WARNING: Module name .* may soon be illegal"))
+                          .filter(s -> s.startsWith("WARNING:"))
+                          .findAny().isPresent());
+    }
+
+
+    @DataProvider(name = "illFormedAddReads")
+    public Object[][] illFormedAddReads() {
+        return new Object[][]{
+            { "m1",         "Unable to parse --add-reads <module>=<value>: m1" },
+
+            // missing source part
+            { "=m2",        "Unable to parse --add-reads <module>=<value>: =m2" },
+
+            // empty list, missing target
+            { "m1=",        "Unable to parse --add-reads <module>=<value>: m1=" },
+
+            // empty list
+            { "m1=,,",      "Target must be specified: --add-reads m1=,," },
+        };
+    }
+
+
+    @Test(dataProvider = "illFormedAddReads")
+    public void testIllFormedAddReads(String value, String msg) throws Exception {
+        int exitValue =
+            executeTestJava("--add-reads", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", M4_MAIN)
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .shouldContain(msg)
+                .getExitValue();
+
+        assertTrue(exitValue != 0);
+    }
+
+
+    @DataProvider(name = "unknownNames")
+    public Object[][] unknownNames() {
+        return new Object[][]{
+
+            // source not found
+            {"DoesNotExist=m2",    "WARNING: Unknown module: DoesNotExist specified in --add-reads"},
+
+            // target not found
+            {"m2=DoesNotExist",    "WARNING: Unknown module: DoesNotExist specified in --add-reads"},
+
+            // bad names
+            {"m*=m2",              "WARNING: Unknown module: m* specified in --add-reads"},
+            {"m2=m!",              "WARNING: Unknown module: m! specified in --add-reads"},
+
+        };
+    }
+
+    @Test(dataProvider = "unknownNames")
+    public void testUnknownNames(String value, String msg) throws Exception {
+        int exitValue =
+            executeTestJava("--add-reads", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", M4_MAIN)
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .shouldContain(msg)
+                .getExitValue();
+
+        assertTrue(exitValue == 0);
+    }
+
+
+    @DataProvider(name = "missingArguments")
+    public Object[][] missingArguments() {
+        return new Object[][]{
+            { new String[] {"--add-reads" },
+                "Error: --add-reads requires modules to be specified"},
+
+            { new String[] { "--add-reads=" },
+                "Error: --add-reads= requires modules to be specified"},
+
+            { new String[] { "--add-reads", "" },
+                "Error: --add-reads requires modules to be specified"},
+        };
+    }
+
+    @Test(dataProvider = "missingArguments")
+    public void testEmptyArgument(String[] options, String msg) throws Exception {
+        String[] args = Stream.concat(Arrays.stream(options), Stream.of("-version"))
+                              .toArray(String[]::new);
+        int exitValue = executeTestJava(args)
+            .outputTo(System.out)
+            .errorTo(System.out)
+            .shouldContain(msg)
+            .getExitValue();
+
+        assertTrue(exitValue != 0);
+    }
+}
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/tools/launcher/modules/patch/basic/PatchTestWarningError.java	Mon Dec 19 15:01:14 2016 -0800
@@ -0,0 +1,220 @@
+/*
+ * Copyright (c) 2016, 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.
+ */
+
+/**
+ * @test
+ * @bug 8168836
+ * @summary Basic argument validation for --patch-module
+ * @library /lib/testlibrary
+ * @modules jdk.compiler
+ * @build PatchTestWarningError CompilerUtils JarUtils jdk.testlibrary.*
+ * @run testng PatchTestWarningError
+ */
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static jdk.testlibrary.ProcessTools.*;
+
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import static org.testng.Assert.*;
+
+
+/**
+ * This test
+ * See PatchTestWarningError for test description.
+ */
+
+@Test
+public class PatchTestWarningError {
+
+    // top-level source directory
+    private static final String TEST_SRC = System.getProperty("test.src");
+
+    // source/destination tree for the test module
+    private static final Path SRC_DIR = Paths.get(TEST_SRC, "src");
+    private static final Path MODS_DIR = Paths.get("mods");
+
+    // source/destination tree for patch tree 1
+    private static final Path SRC1_DIR = Paths.get(TEST_SRC, "src1");
+    private static final Path PATCHES1_DIR = Paths.get("patches1");
+
+    // source/destination tree for patch tree 2
+    private static final Path SRC2_DIR = Paths.get(TEST_SRC, "src2");
+    private static final Path PATCHES2_DIR = Paths.get("patches2");
+
+    // patch path for java.base
+    private static final String PATCHES_PATH =
+        PATCHES1_DIR.resolve("java.base") + File.pathSeparator +
+            PATCHES2_DIR.resolve("java.base");
+
+    // the classes overridden or added with --patch-module
+    private static final String[] CLASSES = {
+
+        // java.base = boot loader
+        "java.base/java.text.Annotation",           // override class
+        "java.base/java.text.AnnotationBuddy",      // add class to package
+        "java.base/java.lang2.Object",              // new package
+
+    };
+
+
+    @BeforeTest
+    public void setup() throws Exception {
+
+        // javac -d mods/test src/test/**
+        boolean compiled= CompilerUtils.compile(SRC_DIR.resolve("test"),
+                                                MODS_DIR.resolve("test"));
+        assertTrue(compiled, "classes did not compile");
+
+        // javac -Xmodule:$MODULE -d patches1/$MODULE patches1/$MODULE/**
+        Path src = SRC1_DIR.resolve("java.base");
+        Path output = PATCHES1_DIR.resolve(src.getFileName());
+        Files.createDirectories(output);
+        String mn = src.getFileName().toString();
+        compiled  = CompilerUtils.compile(src, output, "-Xmodule:" + mn);
+        assertTrue(compiled, "classes did not compile");
+
+        // javac -Xmodule:$MODULE -d patches2/$MODULE patches2/$MODULE/**
+        src = SRC2_DIR.resolve("java.base");
+        output = PATCHES2_DIR.resolve(src.getFileName());
+        Files.createDirectories(output);
+        mn = src.getFileName().toString();
+        compiled  = CompilerUtils.compile(src, output, "-Xmodule:" + mn);
+        assertTrue(compiled, "classes did not compile");
+
+    }
+
+    /**
+     * Test with --patch-module options patching the same module
+     */
+    public void testDuplicateModule() throws Exception {
+        int exitValue =
+            executeTestJava("--patch-module", "java.base=" + PATCHES1_DIR.resolve("java.base"),
+                            "--patch-module", "java.base=" + PATCHES2_DIR.resolve("java.base"),
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", "test/jdk.test.Main")
+                .outputTo(System.out)
+                .errorTo(System.out)
+                // error output by VM
+                .shouldContain("Cannot specify java.base more than once to --patch-module")
+                .getExitValue();
+
+        assertTrue(exitValue != 0);
+    }
+
+    @DataProvider(name = "emptyItem")
+    public Object[][] emptyItems() {
+        String patch1 = PATCHES1_DIR.resolve("java.base").toString();
+        String patch2 = PATCHES2_DIR.resolve("java.base").toString();
+        String pathSep = File.pathSeparator;
+        return new Object[][]{
+
+            { "java.base="+ pathSep + patch1 + pathSep + patch2,            null },
+            { "java.base="+ patch1 + pathSep + pathSep + patch2,            null },
+            { "java.base="+ patch1 + pathSep + patch2 + pathSep + pathSep,  null },
+        };
+    }
+
+    /**
+     * Empty item in a non-empty path list
+     */
+    @Test(dataProvider = "emptyItem")
+    public void testEmptyItem(String value, String msg) throws Exception {
+        // the argument to the test is the list of classes overridden or added
+        String arg = Stream.of(CLASSES).collect(Collectors.joining(","));
+
+        int exitValue =
+            executeTestJava("--patch-module", value,
+                            "--add-exports", "java.base/java.lang2=test",
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", "test/jdk.test.Main", arg)
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .getExitValue();
+
+        assertTrue(exitValue == 0);
+    }
+
+    /**
+     * Test bad module name that should emit a warning
+     */
+    public void testBadName() throws Exception {
+        // the argument to the test is the list of classes overridden or added
+        String arg = Stream.of(CLASSES).collect(Collectors.joining(","));
+
+        int exitValue =
+            executeTestJava("--patch-module", "DoesNotExist=tmp",
+                            "--patch-module", "java.base=" + PATCHES_PATH,
+                            "--add-exports", "java.base/java.lang2=test",
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", "test/jdk.test.Main", arg)
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .shouldContain("WARNING: Unknown module: DoesNotExist specified in --patch-module")
+                .getExitValue();
+
+        assertTrue(exitValue == 0);
+    }
+
+    @DataProvider(name = "badArguments")
+    public Object[][] badArguments() {
+        return new Object[][]{
+
+            // source not found
+            { "=tmp",            "Unable to parse --patch-module <module>=<value>: =tmp" },
+
+            // target not found: check by VM
+            { "java.base",       "Missing '=' in --patch-module specification" },
+            { "foo",             "Missing '=' in --patch-module specification" },
+
+            // target not found
+            { "java.base=",      "Unable to parse --patch-module <module>=<value>: java.base="  },
+            { "java.base=" + File.pathSeparator,
+              "Target must be specified: --patch-module java.base=" + File.pathSeparator }
+        };
+    }
+
+    /**
+     * Test ill-formed argument to --patch-module
+     */
+    @Test(dataProvider = "badArguments")
+    public void testBadArgument(String value, String msg) throws Exception {
+        int exitValue =
+            executeTestJava("--patch-module", value,
+                            "--module-path", MODS_DIR.toString(),
+                            "-m", "test/jdk.test.Main")
+                .outputTo(System.out)
+                .errorTo(System.out)
+                .shouldContain(msg)
+                .getExitValue();
+
+        assertTrue(exitValue != 0);
+    }
+}