The quote is causing test failure on Mac, I’ll apply following fix
diff --git a/test/jdk/tools/launcher/ArgsEnvVar.java
b/test/jdk/tools/launcher/ArgsEnvVar.java
--- a/test/jdk/tools/launcher/ArgsEnvVar.java
+++ b/test/jdk/tools/launcher/ArgsEnvVar.java
@@ -285,7 +285,7 @@
}
// check that specifying --module and --module-path with file works
- tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs\"");
+ tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "@cmdargs");
tr.contains("[--hello]");
if (!tr.testStatus) {
System.out.println(tr);
@@ -294,7 +294,7 @@
// check with reversed --module-path and --module in the arguments
file, this will fail, --module= is terminating
File argFile1 = createArgFile("cmdargs1", List.of(MODULE_OPTION,
"--module-path", MODS_DIR.toString(), "--hello"));
- tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs1\"");
+ tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "@cmdargs1");
tr.checkNegative();
if (!tr.testStatus) {
System.out.println(tr);
Cheers,
Henry
> On Dec 11, 2019, at 10:29 PM, Henry Jen <[email protected]> wrote:
>
> Nikola,
>
> The change looks fine to me as well, I’ll run the test and push it before
> RDP1 if everything is good with Kumar as reviewer.
>
> Cheers,
> Henry
>
>
>> On Dec 11, 2019, at 2:26 PM, Kumar Srinivasan <[email protected]> wrote:
>>
>> Hi Nikola,
>>
>> Thank you for making the changes. Looks good to me.
>>
>> Kumar Srinivasan
>> PS: my new and official email address: [email protected]
>>
>>
>> On Wed, Dec 11, 2019 at 10:04 AM Nikola Grcevski
>> <[email protected]> wrote:
>> Thank you again for reviewing my code Kumar!
>>
>> I have applied your refactoring suggestions. Using the array approach as you
>> suggested made the test code a lot more tidier. I’m attaching the updated
>> patch after my signature and the external webrev is here for reference:
>>
>> https://grcevski.github.io/JDK-8234076/webrev/
>>
>> Cheers,
>> Nikola
>>
>> diff -r bd436284147d src/java.base/share/native/libjli/args.c
>> --- a/src/java.base/share/native/libjli/args.c Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/src/java.base/share/native/libjli/args.c Wed Dec 11 12:09:29 2019
>> -0500
>> @@ -130,6 +130,8 @@
>> }
>> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>> stopExpansion = JNI_TRUE;
>> + } else if (JLI_StrCCmp(arg, "--module=") == 0) {
>> + idx = argsCount;
>> }
>> } else {
>> if (!expectingNoDashArg) {
>> @@ -449,6 +451,7 @@
>> return JLI_StrCmp(arg, "-jar") == 0 ||
>> JLI_StrCmp(arg, "-m") == 0 ||
>> JLI_StrCmp(arg, "--module") == 0 ||
>> + JLI_StrCCmp(arg, "--module=") == 0 ||
>> JLI_StrCmp(arg, "--dry-run") == 0 ||
>> JLI_StrCmp(arg, "-h") == 0 ||
>> JLI_StrCmp(arg, "-?") == 0 ||
>> diff -r bd436284147d src/java.base/windows/native/libjli/java_md.c
>> --- a/src/java.base/windows/native/libjli/java_md.c Wed Nov 20 08:12:14
>> 2019 +0800
>> +++ b/src/java.base/windows/native/libjli/java_md.c Wed Dec 11 12:09:29
>> 2019 -0500
>> @@ -34,6 +34,7 @@
>> #include <sys/stat.h>
>> #include <wtypes.h>
>> #include <commctrl.h>
>> +#include <assert.h>
>>
>> #include <jni.h>
>> #include "java.h"
>> @@ -1015,6 +1016,17 @@
>>
>> // sanity check, match the args we have, to the holy grail
>> idx = JLI_GetAppArgIndex();
>> +
>> + // First arg index is NOT_FOUND
>> + if (idx < 0) {
>> + // The only allowed value should be NOT_FOUND (-1) unless another
>> change introduces
>> + // a different negative index
>> + assert (idx == -1);
>> + JLI_TraceLauncher("Warning: first app arg index not found, %d\n",
>> idx);
>> + JLI_TraceLauncher("passing arguments as-is.\n");
>> + return NewPlatformStringArray(env, strv, argc);
>> + }
>> +
>> isTool = (idx == 0);
>> if (isTool) { idx++; } // skip tool name
>> JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx,
>> stdargs[idx].arg);
>> diff -r bd436284147d test/jdk/tools/launcher/ArgsEnvVar.java
>> --- a/test/jdk/tools/launcher/ArgsEnvVar.java Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/test/jdk/tools/launcher/ArgsEnvVar.java Wed Dec 11 12:09:29 2019
>> -0500
>> @@ -37,6 +37,8 @@
>> import java.util.List;
>> import java.util.Map;
>> import java.util.regex.Pattern;
>> +import java.nio.file.Paths;
>> +import java.nio.file.Path;
>>
>> public class ArgsEnvVar extends TestHelper {
>> private static File testJar = null;
>> @@ -153,6 +155,7 @@
>> List.of("-jar", "test.jar"),
>> List.of("-m", "test/Foo"),
>> List.of("--module", "test/Foo"),
>> + List.of("--module=test/Foo"),
>> List.of("--dry-run"),
>> List.of("-h"),
>> List.of("-?"),
>> @@ -241,6 +244,69 @@
>> verifyOptions(List.of("--add-exports",
>> "java.base/jdk.internal.misc=ALL-UNNAMED", "-jar", "test.jar"), tr);
>> }
>>
>> +
>> + @Test
>> + // That that we can correctly handle the module longform argument option
>> + // when supplied in an argument file
>> + public void modulesInArgsFile() throws IOException {
>> + File cwd = new File(".");
>> + File testModuleDir = new File(cwd, "modules_test");
>> +
>> + createEchoArgumentsModule(testModuleDir);
>> +
>> + Path SRC_DIR = Paths.get(testModuleDir.getAbsolutePath(), "src");
>> + Path MODS_DIR = Paths.get(testModuleDir.getAbsolutePath(), "mods");
>> +
>> + // test module / main class
>> + String MODULE_OPTION = "--module=test/launcher.Main";
>> + String TEST_MODULE = "test";
>> +
>> + // javac -d mods/test src/test/**
>> + TestResult tr = doExec(
>> + javacCmd,
>> + "-d", MODS_DIR.toString(),
>> + "--module-source-path", SRC_DIR.toString(),
>> + "--module", TEST_MODULE);
>> +
>> + if (!tr.isOK()) {
>> + System.out.println("test did not compile");
>> + throw new RuntimeException("Error: modules test did not
>> compile");
>> + }
>> +
>> + // verify the terminating ability of --module= through environment
>> variables
>> + File argFile = createArgFile("cmdargs", List.of("--module-path",
>> MODS_DIR.toString(), MODULE_OPTION, "--hello"));
>> + env.put(JDK_JAVA_OPTIONS, "@cmdargs");
>> + tr = doExec(env, javaCmd);
>> + tr.checkNegative();
>> + tr.contains("Error: Option " + MODULE_OPTION + " in @cmdargs is not
>> allowed in environment variable JDK_JAVA_OPTIONS");
>> + if (!tr.testStatus) {
>> + System.out.println(tr);
>> + throw new RuntimeException("test fails");
>> + }
>> +
>> + // check that specifying --module and --module-path with file works
>> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs\"");
>> + tr.contains("[--hello]");
>> + if (!tr.testStatus) {
>> + System.out.println(tr);
>> + throw new RuntimeException("test fails");
>> + }
>> +
>> + // check with reversed --module-path and --module in the arguments
>> file, this will fail, --module= is terminating
>> + File argFile1 = createArgFile("cmdargs1", List.of(MODULE_OPTION,
>> "--module-path", MODS_DIR.toString(), "--hello"));
>> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs1\"");
>> + tr.checkNegative();
>> + if (!tr.testStatus) {
>> + System.out.println(tr);
>> + throw new RuntimeException("test fails");
>> + }
>> +
>> + // clean-up
>> + argFile.delete();
>> + argFile1.delete();
>> + recursiveDelete(testModuleDir);
>> + }
>> +
>> public static void main(String... args) throws Exception {
>> init();
>> ArgsEnvVar a = new ArgsEnvVar();
>> diff -r bd436284147d test/jdk/tools/launcher/TestHelper.java
>> --- a/test/jdk/tools/launcher/TestHelper.java Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/test/jdk/tools/launcher/TestHelper.java Wed Dec 11 12:09:29 2019
>> -0500
>> @@ -500,6 +500,40 @@
>> return Locale.getDefault().getLanguage().equals("en");
>> }
>>
>> + /**
>> + * Helper method to initialize a simple module that just prints the
>> passed in arguments
>> + */
>> + static void createEchoArgumentsModule(File modulesDir) throws
>> IOException {
>> + if (modulesDir.exists()) {
>> + recursiveDelete(modulesDir);
>> + }
>> +
>> + modulesDir.mkdirs();
>> +
>> + File srcDir = new File(modulesDir, "src");
>> + File modsDir = new File(modulesDir, "mods");
>> + File testDir = new File(srcDir, "test");
>> + File launcherTestDir = new File(testDir, "launcher");
>> + srcDir.mkdir();
>> + modsDir.mkdir();
>> + testDir.mkdir();
>> + launcherTestDir.mkdir();
>> +
>> + String[] moduleInfoCode = { "module test {}" };
>> + createFile(new File(testDir, "module-info.java"),
>> Arrays.asList(moduleInfoCode));
>> +
>> + String[] moduleCode = {
>> + "package launcher;",
>> + "import java.util.Arrays;",
>> + "public class Main {",
>> + "public static void main(String[] args) {",
>> + "System.out.println(Arrays.toString(args));",
>> + "}",
>> + "}"
>> + };
>> + createFile(new File(launcherTestDir, "Main.java"),
>> Arrays.asList(moduleCode));
>> + }
>> +
>> /*
>> * A class to encapsulate the test results and stuff, with some ease
>> * of use methods to check the test results.
>> diff -r bd436284147d test/jdk/tools/launcher/TestSpecialArgs.java
>> --- a/test/jdk/tools/launcher/TestSpecialArgs.java Wed Nov 20 08:12:14
>> 2019 +0800
>> +++ b/test/jdk/tools/launcher/TestSpecialArgs.java Wed Dec 11 12:09:29
>> 2019 -0500
>> @@ -246,6 +246,10 @@
>> if (!tr.contains("Error: Could not find or load main class
>> AbsentClass")) {
>> throw new RuntimeException("Test Fails");
>> }
>> +
>> + // Make sure we handle correctly the module long form (--module=)
>> + tr = doExec(javaCmd, "-XX:NativeMemoryTracking=summary",
>> "--module=jdk.compiler/com.sun.tools.javac.Main", "--help");
>> + ensureNoWarnings(tr);
>> }
>>
>> void ensureNoWarnings(TestResult tr) {
>> diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
>> --- a/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Nov 20
>> 08:12:14 2019 +0800
>> +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Dec 11
>> 12:09:29 2019 -0500
>> @@ -29,6 +29,7 @@
>> * jdk.jlink
>> * @build BasicTest jdk.test.lib.compiler.CompilerUtils
>> * @run testng BasicTest
>> + * @bug 8234076
>> * @summary Basic test of starting an application as a module
>> */
>>
>> @@ -40,6 +41,8 @@
>>
>> import jdk.test.lib.compiler.CompilerUtils;
>> import jdk.test.lib.process.ProcessTools;
>> +import jdk.test.lib.process.OutputAnalyzer;
>> +import jdk.test.lib.Utils;
>>
>> import org.testng.annotations.BeforeTest;
>> import org.testng.annotations.Test;
>> @@ -70,6 +73,8 @@
>> // the module main class
>> private static final String MAIN_CLASS = "jdk.test.Main";
>>
>> + // for Windows specific launcher tests
>> + static final boolean IS_WINDOWS = System.getProperty("os.name",
>> "unknown").startsWith("Windows");
>>
>> @BeforeTest
>> public void compileTestModule() throws Exception {
>> @@ -259,4 +264,87 @@
>> assertTrue(exitValue != 0);
>> }
>>
>> +
>> + /**
>> + * Helper method that creates a ProcessBuilder with command line
>> arguments
>> + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
>> + */
>> + private ProcessBuilder createProcessWithLauncherDebugging(String...
>> cmds) {
>> + ProcessBuilder pb =
>> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
>> + pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
>> +
>> + return pb;
>> + }
>> +
>> + /**
>> + * Test the ability for the Windows launcher to do proper application
>> argument
>> + * detection and expansion, when using the long form module option and
>> all passed in
>> + * command line arguments are prefixed with a dash.
>> + *
>> + * These tests are not expected to work on *nixes, and are ignored.
>> + */
>> + public void testWindowsWithLongFormModuleOption() throws Exception {
>> + if (!IS_WINDOWS) {
>> + return;
>> + }
>> +
>> + String dir = MODS_DIR.toString();
>> + String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
>> + // We should be able to find the argument --help as an application
>> argument
>> + ProcessTools.executeProcess(
>> + createProcessWithLauncherDebugging(
>> + "--module-path=" + dir,
>> + "--module=" + mid,
>> + "--help"))
>> + .outputTo(System.out)
>> + .errorTo(System.out)
>> + .shouldContain("F--help");
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> <...src/test>/*.java --help
>> + // We should be able to see argument expansion happen
>> + ProcessTools.executeProcess(
>> + createProcessWithLauncherDebugging(
>> + "--module-path=" + dir,
>> + "--module=" + mid,
>> + SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
>> + "--help"))
>> + .outputTo(System.out)
>> + .errorTo(System.out)
>> + .shouldContain("F--help")
>> + .shouldContain("module-info.java");
>> + }
>> +
>> +
>> + /**
>> + * Test that --module= is terminating for VM argument processing just
>> like --module
>> + */
>> + public void testLongFormModuleOptionTermination() throws Exception {
>> + String dir = MODS_DIR.toString();
>> + String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> + // The first --module= will terminate the VM arguments processing.
>> The second pair of module-path and module will be
>> + // deemed as application arguments
>> + OutputAnalyzer output = ProcessTools.executeProcess(
>> + createProcessWithLauncherDebugging(
>> + "--module-path=" + dir,
>> + "--module=" + mid,
>> + "--module-path=" + dir,
>> + "--module=" + mid))
>> + .outputTo(System.out)
>> + .errorTo(System.out)
>> + .shouldContain("argv[ 0] = '--module-path=" + dir)
>> + .shouldContain("argv[ 1] = '--module=" + mid);
>> +
>> + if (IS_WINDOWS) {
>> + output.shouldContain("F--module-path=" +
>> dir).shouldContain("F--module=" + mid);
>> + }
>> +
>> + // java --module=$TESTMODULE/$MAINCLASS --module-path=mods
>> + // This command line will not work as --module= is terminating and
>> the module will be not found
>> + int exitValue = exec("--module=" + mid, "--module-path" + dir);
>> + assertTrue(exitValue != 0);
>> + }
>> }
>>
>> From: Kumar Srinivasan <[email protected]>
>> Sent: December 10, 2019 8:43 PM
>> To: Nikola Grcevski <[email protected]>
>> Cc: Henry Jen <[email protected]>; Alan Bateman
>> <[email protected]>; [email protected]
>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>
>> [Resend; cc'ing the group]
>>
>> Hi Nikola,
>>
>> Generally looks great to me. I would have moved initModulesDir to
>> TestHelper.createSimpleModule(File dir).
>> TestHelper.java contains a collection of test related utilities needed by
>> the launcher, it will help someone
>> else use it to create a simple module.
>>
>> I would have rewritten this
>> + ArrayList<String> scratchpad = new ArrayList<>();
>> + scratchpad.add("module test {}");
>> + createFile(new File(testDir, "module-info.java"), scratchpad);
>> + scratchpad.clear();
>> + scratchpad.add("package launcher;");
>> + scratchpad.add("import java.util.Arrays;");
>> + scratchpad.add("public class Main {");
>> + scratchpad.add("public static void main(String[] args) {");
>> + scratchpad.add("System.out.println(Arrays.toString(args));");
>> + scratchpad.add("}");
>> + scratchpad.add("}");
>> + createFile(new File(launcherTestDir, "Main.java"), scratchpad);
>>
>> as follows:
>> String a1[] = {"module test{}"};
>> createFile(new File(testDir, "module-info.java", Arrays.asList(a1));
>>
>> String a2[] = {
>> "package launcher;",
>> ....
>> ....
>> }
>> createFile(new File(launcherTestDir, "Main.java"), Arrays.asList(a2));
>>
>> This might make the code neater.
>>
>> Besides that, looks very good.
>>
>> Thanks
>> Kumar
>>
>>
>> On Mon, Dec 9, 2019 at 1:58 PM Nikola Grcevski
>> <mailto:[email protected]> wrote:
>> Hi Henry and Kumar,
>>
>> Thank you again for the review! I have added the fix to isTerminalOpt and
>> used both of your suggestions to make new tests.
>> With native memory tracking enabled, I could actually see a crash on both
>> Linux and Windows without the suggested fix.
>>
>> I tested the changes again on both Linux and Windows, and the new unit tests
>> fail if isTerminalOpt doesn’t check for –module=
>> as per Henry's suggestion.
>>
>> I'm attaching the new patch (my apologies for the size) at the bottom of
>> this email after my signature. If I haven't covered
>> certain aspects in the new tests please let me know, I'm more than happy to
>> extend them further. I've updated the webrev
>> to reflect the latest patch if it's easier to read:
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824589979&sdata=XcriPrQ%2B9rypJN6IShAKU2%2Fz5sZE%2BMCWT3gHyVg3Y3s%3D&reserved=0
>>
>> Thanks again!
>> Nikola
>>
>> diff -r bd436284147d src/java.base/share/native/libjli/args.c
>> --- a/src/java.base/share/native/libjli/args.c Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/src/java.base/share/native/libjli/args.c Mon Dec 09 16:08:54 2019
>> -0500
>> @@ -130,6 +130,8 @@
>> }
>> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>> stopExpansion = JNI_TRUE;
>> + } else if (JLI_StrCCmp(arg, "--module=") == 0) {
>> + idx = argsCount;
>> }
>> } else {
>> if (!expectingNoDashArg) {
>> @@ -449,6 +451,7 @@
>> return JLI_StrCmp(arg, "-jar") == 0 ||
>> JLI_StrCmp(arg, "-m") == 0 ||
>> JLI_StrCmp(arg, "--module") == 0 ||
>> + JLI_StrCCmp(arg, "--module=") == 0 ||
>> JLI_StrCmp(arg, "--dry-run") == 0 ||
>> JLI_StrCmp(arg, "-h") == 0 ||
>> JLI_StrCmp(arg, "-?") == 0 ||
>> diff -r bd436284147d src/java.base/windows/native/libjli/java_md.c
>> --- a/src/java.base/windows/native/libjli/java_md.c Wed Nov 20 08:12:14
>> 2019 +0800
>> +++ b/src/java.base/windows/native/libjli/java_md.c Mon Dec 09 16:08:54
>> 2019 -0500
>> @@ -34,6 +34,7 @@
>> #include <sys/stat.h>
>> #include <wtypes.h>
>> #include <commctrl.h>
>> +#include <assert.h>
>>
>> #include <jni.h>
>> #include "java.h"
>> @@ -1015,6 +1016,17 @@
>>
>> // sanity check, match the args we have, to the holy grail
>> idx = JLI_GetAppArgIndex();
>> +
>> + // First arg index is NOT_FOUND
>> + if (idx < 0) {
>> + // The only allowed value should be NOT_FOUND (-1) unless another
>> change introduces
>> + // a different negative index
>> + assert (idx == -1);
>> + JLI_TraceLauncher("Warning: first app arg index not found, %d\n",
>> idx);
>> + JLI_TraceLauncher("passing arguments as-is.\n");
>> + return NewPlatformStringArray(env, strv, argc);
>> + }
>> +
>> isTool = (idx == 0);
>> if (isTool) { idx++; } // skip tool name
>> JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx,
>> stdargs[idx].arg);
>> diff -r bd436284147d test/jdk/tools/launcher/ArgsEnvVar.java
>> --- a/test/jdk/tools/launcher/ArgsEnvVar.java Wed Nov 20 08:12:14 2019
>> +0800
>> +++ b/test/jdk/tools/launcher/ArgsEnvVar.java Mon Dec 09 16:08:54 2019
>> -0500
>> @@ -37,6 +37,8 @@
>> import java.util.List;
>> import java.util.Map;
>> import java.util.regex.Pattern;
>> +import java.nio.file.Paths;
>> +import java.nio.file.Path;
>>
>> public class ArgsEnvVar extends TestHelper {
>> private static File testJar = null;
>> @@ -153,6 +155,7 @@
>> List.of("-jar", "test.jar"),
>> List.of("-m", "test/Foo"),
>> List.of("--module", "test/Foo"),
>> + List.of("--module=test/Foo"),
>> List.of("--dry-run"),
>> List.of("-h"),
>> List.of("-?"),
>> @@ -241,6 +244,101 @@
>> verifyOptions(List.of("--add-exports",
>> "java.base/jdk.internal.misc=ALL-UNNAMED", "-jar", "test.jar"), tr);
>> }
>>
>> + /**
>> + * Helper method to initialize a simple module that just prints the
>> passed in arguments
>> + */
>> + private void initModulesDir(File modulesDir) throws IOException {
>> + if (modulesDir.exists()) {
>> + recursiveDelete(modulesDir);
>> + }
>> +
>> + modulesDir.mkdirs();
>> +
>> + File srcDir = new File(modulesDir, "src");
>> + File modsDir = new File(modulesDir, "mods");
>> + File testDir = new File(srcDir, "test");
>> + File launcherTestDir = new File(testDir, "launcher");
>> + srcDir.mkdir();
>> + modsDir.mkdir();
>> + testDir.mkdir();
>> + launcherTestDir.mkdir();
>> +
>> + ArrayList<String> scratchpad = new ArrayList<>();
>> + scratchpad.add("module test {}");
>> + createFile(new File(testDir, "module-info.java"), scratchpad);
>> + scratchpad.clear();
>> + scratchpad.add("package launcher;");
>> + scratchpad.add("import java.util.Arrays;");
>> + scratchpad.add("public class Main {");
>> + scratchpad.add("public static void main(String[] args) {");
>> + scratchpad.add("System.out.println(Arrays.toString(args));");
>> + scratchpad.add("}");
>> + scratchpad.add("}");
>> + createFile(new File(launcherTestDir, "Main.java"), scratchpad);
>> + }
>> +
>> + @Test
>> + // That that we can correctly handle the module longform argument option
>> + // when supplied in an argument file
>> + public void modulesInArgsFile() throws IOException {
>> + File cwd = new File(".");
>> + File testModuleDir = new File(cwd, "modules_test");
>> +
>> + initModulesDir(testModuleDir);
>> +
>> + Path SRC_DIR = Paths.get(testModuleDir.getAbsolutePath(), "src");
>> + Path MODS_DIR = Paths.get(testModuleDir.getAbsolutePath(), "mods");
>> +
>> + // test module / main class
>> + String MODULE_OPTION = "--module=test/launcher.Main";
>> + String TEST_MODULE = "test";
>> +
>> + // javac -d mods/test src/test/**
>> + TestResult tr = doExec(
>> + javacCmd,
>> + "-d", MODS_DIR.toString(),
>> + "--module-source-path", SRC_DIR.toString(),
>> + "--module", TEST_MODULE);
>> +
>> + if (!tr.isOK()) {
>> + System.out.println("test did not compile");
>> + throw new RuntimeException("Error: modules test did not
>> compile");
>> + }
>> +
>> + // verify the terminating ability of --module= through environment
>> variables
>> + File argFile = createArgFile("cmdargs", List.of("--module-path",
>> MODS_DIR.toString(), MODULE_OPTION, "--hello"));
>> + env.put(JDK_JAVA_OPTIONS, "@cmdargs");
>> + tr = doExec(env, javaCmd);
>> + tr.checkNegative();
>> + tr.contains("Error: Option " + MODULE_OPTION + " in @cmdargs is not
>> allowed in environment variable JDK_JAVA_OPTIONS");
>> + if (!tr.testStatus) {
>> + System.out.println(tr);
>> + throw new RuntimeException("test fails");
>> + }
>> +
>> + // check that specifying --module and --module-path with file works
>> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs\"");
>> + tr.contains("[--hello]");
>> + if (!tr.testStatus) {
>> + System.out.println(tr);
>> + throw new RuntimeException("test fails");
>> + }
>> +
>> + // check with reversed --module-path and --module in the arguments
>> file, this will fail, --module= is terminating
>> + File argFile1 = createArgFile("cmdargs1", List.of(MODULE_OPTION,
>> "--module-path", MODS_DIR.toString(), "--hello"));
>> + tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs1\"");
>> + tr.checkNegative();
>> + if (!tr.testStatus) {
>> + System.out.println(tr);
>> + throw new RuntimeException("test fails");
>> + }
>> +
>> + // clean-up
>> + argFile.delete();
>> + argFile1.delete();
>> + recursiveDelete(testModuleDir);
>> + }
>> +
>> public static void main(String... args) throws Exception {
>> init();
>> ArgsEnvVar a = new ArgsEnvVar();
>> diff -r bd436284147d test/jdk/tools/launcher/TestSpecialArgs.java
>> --- a/test/jdk/tools/launcher/TestSpecialArgs.java Wed Nov 20 08:12:14
>> 2019 +0800
>> +++ b/test/jdk/tools/launcher/TestSpecialArgs.java Mon Dec 09 16:08:54
>> 2019 -0500
>> @@ -246,6 +246,10 @@
>> if (!tr.contains("Error: Could not find or load main class
>> AbsentClass")) {
>> throw new RuntimeException("Test Fails");
>> }
>> +
>> + // Make sure we handle correctly the module long form (--module=)
>> + tr = doExec(javaCmd, "-XX:NativeMemoryTracking=summary",
>> "--module=jdk.compiler/com.sun.tools.javac.Main", "--help");
>> + ensureNoWarnings(tr);
>> }
>>
>> void ensureNoWarnings(TestResult tr) {
>> diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
>> --- a/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Nov 20
>> 08:12:14 2019 +0800
>> +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.java Mon Dec 09
>> 16:08:54 2019 -0500
>> @@ -29,6 +29,7 @@
>> * jdk.jlink
>> * @build BasicTest jdk.test.lib.compiler.CompilerUtils
>> * @run testng BasicTest
>> + * @bug 8234076
>> * @summary Basic test of starting an application as a module
>> */
>>
>> @@ -40,6 +41,8 @@
>>
>> import jdk.test.lib.compiler.CompilerUtils;
>> import jdk.test.lib.process.ProcessTools;
>> +import jdk.test.lib.process.OutputAnalyzer;
>> +import jdk.test.lib.Utils;
>>
>> import org.testng.annotations.BeforeTest;
>> import org.testng.annotations.Test;
>> @@ -70,6 +73,8 @@
>> // the module main class
>> private static final String MAIN_CLASS = "jdk.test.Main";
>>
>> + // for Windows specific launcher tests
>> + static final boolean IS_WINDOWS =
>> System.getProperty("https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fos.name&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824589979&sdata=kgi%2B5Ob%2FqX45oTtLSTalwXeQqIct1EUD1k30KOddm5c%3D&reserved=0";,
>> "unknown").startsWith("Windows");
>>
>> @BeforeTest
>> public void compileTestModule() throws Exception {
>> @@ -259,4 +264,87 @@
>> assertTrue(exitValue != 0);
>> }
>>
>> +
>> + /**
>> + * Helper method that creates a ProcessBuilder with command line
>> arguments
>> + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
>> + */
>> + private ProcessBuilder createProcessWithLauncherDebugging(String...
>> cmds) {
>> + ProcessBuilder pb =
>> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
>> + pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
>> +
>> + return pb;
>> + }
>> +
>> + /**
>> + * Test the ability for the Windows launcher to do proper application
>> argument
>> + * detection and expansion, when using the long form module option and
>> all passed in
>> + * command line arguments are prefixed with a dash.
>> + *
>> + * These tests are not expected to work on *nixes, and are ignored.
>> + */
>> + public void testWindowsWithLongFormModuleOption() throws Exception {
>> + if (!IS_WINDOWS) {
>> + return;
>> + }
>> +
>> + String dir = MODS_DIR.toString();
>> + String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
>> + // We should be able to find the argument --help as an application
>> argument
>> + ProcessTools.executeProcess(
>> + createProcessWithLauncherDebugging(
>> + "--module-path=" + dir,
>> + "--module=" + mid,
>> + "--help"))
>> + .outputTo(System.out)
>> + .errorTo(System.out)
>> + .shouldContain("F--help");
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> <...src/test>/*.java --help
>> + // We should be able to see argument expansion happen
>> + ProcessTools.executeProcess(
>> + createProcessWithLauncherDebugging(
>> + "--module-path=" + dir,
>> + "--module=" + mid,
>> + SRC_DIR.resolve(TEST_MODULE).toString() + "file://*.java",
>> + "--help"))
>> + .outputTo(System.out)
>> + .errorTo(System.out)
>> + .shouldContain("F--help")
>> + .shouldContain("module-info.java");
>> + }
>> +
>> +
>> + /**
>> + * Test that --module= is terminating for VM argument processing just
>> like --module
>> + */
>> + public void testLongFormModuleOptionTermination() throws Exception {
>> + String dir = MODS_DIR.toString();
>> + String mid = TEST_MODULE + "/" + MAIN_CLASS;
>> +
>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> --module-path=mods --module=$TESTMODULE/$MAINCLASS
>> + // The first --module= will terminate the VM arguments processing.
>> The second pair of module-path and module will be
>> + // deemed as application arguments
>> + OutputAnalyzer output = ProcessTools.executeProcess(
>> + createProcessWithLauncherDebugging(
>> + "--module-path=" + dir,
>> + "--module=" + mid,
>> + "--module-path=" + dir,
>> + "--module=" + mid))
>> + .outputTo(System.out)
>> + .errorTo(System.out)
>> + .shouldContain("argv[ 0] = '--module-path=" + dir)
>> + .shouldContain("argv[ 1] = '--module=" + mid);
>> +
>> + if (IS_WINDOWS) {
>> + output.shouldContain("F--module-path=" +
>> dir).shouldContain("F--module=" + mid);
>> + }
>> +
>> + // java --module=$TESTMODULE/$MAINCLASS --module-path=mods
>> + // This command line will not work as --module= is terminating and
>> the module will be not found
>> + int exitValue = exec("--module=" + mid, "--module-path" + dir);
>> + assertTrue(exitValue != 0);
>> + }
>> }
>>
>>
>> From: Kumar Srinivasan <mailto:[email protected]>
>> Sent: December 7, 2019 10:28 PM
>> To: Henry Jen <mailto:[email protected]>
>> Cc: Nikola Grcevski <mailto:[email protected]>; Alan Bateman
>> <mailto:[email protected]>; mailto:[email protected]
>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>
>> Hi,
>>
>> The launcher fix looks good, despite the launcher fix being simple, please
>> note,
>> it sometimes requires a lot more effort to write a test for it, please read
>> on....
>>
>> Henry excellent catch wrt. isTerminalOp the launcher!!!, that tickled my
>> memory and I recalled
>> this change for VM's native memory tracking.
>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fhg.openjdk.java.net%2Fjdk9%2Fjdk9%2Fjdk%2Ffile%2F37d1442d53bc%2Ftest%2Ftools%2Flauncher%2FTestSpecialArgs.java%23l243&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824599960&sdata=P3l7Y932pcZ14WwbszC0%2BIN%2BUcS2QrAizwT0Idqa6yQ%3D&reserved=0
>> Inspecting the above changeset, we might have to add another test for JVM
>> native memory tracking,
>> since this is a terminal argument.
>>
>> Maybe TestHelper could create a simple module, which prints its args,
>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk%2Ffile%2F31882abe1494%2Ftest%2Fjdk%2Ftools%2Flauncher%2FTestHelper.java&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824599960&sdata=iaKDsz3aVTdEumV4eOMj5SbA9WtSHT6ADA4KEMoqwgw%3D&reserved=0
>>
>> Kumar Srinivasan
>>
>> On Fri, Dec 6, 2019 at 2:44 PM Henry Jen
>> <mailto:mailto:[email protected]> wrote:
>> Thanks for the work, the test case covers that when —module= is used before
>> other options, which is good.
>>
>> But we need another to make sure that when —module= is put in an argument
>> file or environment variable, that should not be allowed. The fix is simple,
>> we need to add that to isTerminalOp() method.
>>
>> Cheers,
>> Henry
>>
>>> On Dec 6, 2019, at 12:28 PM, Nikola Grcevski
>>> <mailto:mailto:[email protected]> wrote:
>>>
>>> Thank you Henry!
>>>
>>> I have modified the fix in checkArg to use JLI_StrCCmp as suggested below
>>> and I have extended the BasicTest.java (in
>>> test/jdk/tools/launcher/modules/basic) with few additional tests.
>>>
>>> I don't have author status yet, therefore I'm attaching the patch right
>>> after my signature. I also updated my external webrev if that's easier to
>>> review
>>> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrcevski.github.io%2FJDK-8234076%2Fwebrev%2F&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824609890&sdata=KrUiucM4CY4G1aADtTMhHwJxr3e7ATJDMMxX5uG3b14%3D&reserved=0)
>>>
>>> Thanks again to everyone for your help with this bug. If there are any
>>> additional comments or suggestions please let me know.
>>> Nikola
>>>
>>> diff -r bd436284147d src/java.base/share/native/libjli/args.c
>>> --- a/src/java.base/share/native/libjli/args.c Wed Nov 20 08:12:14
>>> 2019 +0800
>>> +++ b/src/java.base/share/native/libjli/args.c Fri Dec 06 15:11:36
>>> 2019 -0500
>>> @@ -130,6 +130,8 @@
>>> }
>>> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>>> stopExpansion = JNI_TRUE;
>>> + } else if (JLI_StrCCmp(arg, "--module=") == 0) {
>>> + idx = argsCount;
>>> }
>>> } else {
>>> if (!expectingNoDashArg) {
>>> diff -r bd436284147d src/java.base/windows/native/libjli/java_md.c
>>> --- a/src/java.base/windows/native/libjli/java_md.c Wed Nov 20 08:12:14
>>> 2019 +0800
>>> +++ b/src/java.base/windows/native/libjli/java_md.c Fri Dec 06 15:11:36
>>> 2019 -0500
>>> @@ -34,6 +34,7 @@
>>> #include <sys/stat.h>
>>> #include <wtypes.h>
>>> #include <commctrl.h>
>>> +#include <assert.h>
>>>
>>> #include <jni.h>
>>> #include "java.h"
>>> @@ -1015,6 +1016,17 @@
>>>
>>> // sanity check, match the args we have, to the holy grail
>>> idx = JLI_GetAppArgIndex();
>>> +
>>> + // First arg index is NOT_FOUND
>>> + if (idx < 0) {
>>> + // The only allowed value should be NOT_FOUND (-1) unless another
>>> change introduces
>>> + // a different negative index
>>> + assert (idx == -1);
>>> + JLI_TraceLauncher("Warning: first app arg index not found, %d\n",
>>> idx);
>>> + JLI_TraceLauncher("passing arguments as-is.\n");
>>> + return NewPlatformStringArray(env, strv, argc);
>>> + }
>>> +
>>> isTool = (idx == 0);
>>> if (isTool) { idx++; } // skip tool name
>>> JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx,
>>> stdargs[idx].arg);
>>> diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
>>> --- a/test/jdk/tools/launcher/modules/basic/BasicTest.java Wed Nov 20
>>> 08:12:14 2019 +0800
>>> +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.java Fri Dec 06
>>> 15:11:36 2019 -0500
>>> @@ -29,6 +29,7 @@
>>> * jdk.jlink
>>> * @build BasicTest jdk.test.lib.compiler.CompilerUtils
>>> * @run testng BasicTest
>>> + * @bug 8234076
>>> * @summary Basic test of starting an application as a module
>>> */
>>>
>>> @@ -40,6 +41,8 @@
>>>
>>> import jdk.test.lib.compiler.CompilerUtils;
>>> import jdk.test.lib.process.ProcessTools;
>>> +import jdk.test.lib.process.OutputAnalyzer;
>>> +import jdk.test.lib.Utils;
>>>
>>> import org.testng.annotations.BeforeTest;
>>> import org.testng.annotations.Test;
>>> @@ -70,6 +73,8 @@
>>> // the module main class
>>> private static final String MAIN_CLASS = "jdk.test.Main";
>>>
>>> + // for Windows specific launcher tests
>>> + static final boolean IS_WINDOWS =
>>> System.getProperty("https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fos.name&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824609890&sdata=3FmeinWv8eFAn8IWUozJWE2OKKaOP9QUFV75TIpu%2Bjk%3D&reserved=0";,
>>> "unknown").startsWith("Windows");
>>>
>>> @BeforeTest
>>> public void compileTestModule() throws Exception {
>>> @@ -259,4 +264,87 @@
>>> assertTrue(exitValue != 0);
>>> }
>>>
>>> +
>>> + /**
>>> + * Helper method that creates a ProcessBuilder with command line
>>> arguments
>>> + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
>>> + */
>>> + private ProcessBuilder createProcessWithLauncherDebugging(String...
>>> cmds) {
>>> + ProcessBuilder pb =
>>> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
>>> + pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
>>> +
>>> + return pb;
>>> + }
>>> +
>>> + /**
>>> + * Test the ability for the Windows launcher to do proper application
>>> argument
>>> + * detection and expansion, when using the long form module option and
>>> all passed in
>>> + * command line arguments are prefixed with a dash.
>>> + *
>>> + * These tests are not expected to work on *nixes, and are ignored.
>>> + */
>>> + public void testWindowsWithLongFormModuleOption() throws Exception {
>>> + if (!IS_WINDOWS) {
>>> + return;
>>> + }
>>> +
>>> + String dir = MODS_DIR.toString();
>>> + String mid = TEST_MODULE + "/" + MAIN_CLASS;
>>> +
>>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
>>> + // We should be able to find the argument --help as an application
>>> argument
>>> + ProcessTools.executeProcess(
>>> + createProcessWithLauncherDebugging(
>>> + "--module-path=" + dir,
>>> + "--module=" + mid,
>>> + "--help"))
>>> + .outputTo(System.out)
>>> + .errorTo(System.out)
>>> + .shouldContain("F--help");
>>> +
>>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>>> <...src/test>/*.java --help
>>> + // We should be able to see argument expansion happen
>>> + ProcessTools.executeProcess(
>>> + createProcessWithLauncherDebugging(
>>> + "--module-path=" + dir,
>>> + "--module=" + mid,
>>> + SRC_DIR.resolve(TEST_MODULE).toString() + "file://*.java",
>>> + "--help"))
>>> + .outputTo(System.out)
>>> + .errorTo(System.out)
>>> + .shouldContain("F--help")
>>> + .shouldContain("module-info.java");
>>> + }
>>> +
>>> +
>>> + /**
>>> + * Test that --module= is terminating for VM argument processing just
>>> like --module
>>> + */
>>> + public void testLongFormModuleOptionTermination() throws Exception {
>>> + String dir = MODS_DIR.toString();
>>> + String mid = TEST_MODULE + "/" + MAIN_CLASS;
>>> +
>>> + // java --module-path=mods --module=$TESTMODULE/$MAINCLASS
>>> --module-path=mods --module=$TESTMODULE/$MAINCLASS
>>> + // The first --module= will terminate the VM arguments processing.
>>> The second pair of module-path and module will be
>>> + // deemed as application arguments
>>> + OutputAnalyzer output = ProcessTools.executeProcess(
>>> + createProcessWithLauncherDebugging(
>>> + "--module-path=" + dir,
>>> + "--module=" + mid,
>>> + "--module-path=" + dir,
>>> + "--module=" + mid))
>>> + .outputTo(System.out)
>>> + .errorTo(System.out)
>>> + .shouldContain("argv[ 0] = '--module-path=" + dir)
>>> + .shouldContain("argv[ 1] = '--module=" + mid);
>>> +
>>> + if (IS_WINDOWS) {
>>> + output.shouldContain("F--module-path=" +
>>> dir).shouldContain("F--module=" + mid);
>>> + }
>>> +
>>> + // java --module=$TESTMODULE/$MAINCLASS --module-path=mods
>>> + // This command line will not work as --module= is terminating and
>>> the module will be not found
>>> + int exitValue = exec("--module=" + mid, "--module-path" + dir);
>>> + assertTrue(exitValue != 0);
>>> + }
>>> }
>>>
>>>
>>> -----Original Message-----
>>> From: Henry Jen <mailto:mailto:[email protected]>
>>> Sent: December 6, 2019 12:03 AM
>>> To: Nikola Grcevski <mailto:mailto:[email protected]>
>>> Cc: Kumar Srinivasan <mailto:mailto:[email protected]>; Alan Bateman
>>> <mailto:mailto:[email protected]>;
>>> mailto:mailto:[email protected]
>>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>>
>>>
>>>> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski
>>>> <mailto:mailto:[email protected]> wrote:
>>>>
>>>> Hello all again!
>>>>
>>>> Based on the suggestion by Kumar I made the following small patch to
>>>> checkArg src/java.base/share/native/libjli/args.c:
>>>>
>>>> --- a/src/java.base/share/native/libjli/args.c
>>>> +++ b/src/java.base/share/native/libjli/args.c
>>>> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
>>>> }
>>>> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>>>> stopExpansion = JNI_TRUE;
>>>> + } else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
>>>
>>> I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with
>>> origin crash prevention for -1 is a safe approach and most compatible to
>>> current behavior.
>>>
>>> BTW, we need the patch to be hosted on
>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fcr.openjdk.java.net&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824619848&sdata=iuIbGg8zzbN7EPOUkKuXfzAUknZDXckOdwBX2XhwGrE%3D&reserved=0
>>> or you can attach the patch to the review thread if you are not yet an
>>> author.
>>>
>>> Cheers,
>>> Henry
>>>
>>>
>>>> + idx = argsCount;
>>>> }
>>>> } else {
>>>> if (!expectingNoDashArg) {
>>>>
>>>> The change is in common code and simply checks for the usage of --module=
>>>> and deems the next argument as the start of the application arguments. I
>>>> can confirm that using -m instead of --module doesn't allow for the =
>>>> separator to be used, so we only need to check for --module= if this is a
>>>> desired change.
>>>>
>>>> I tested with various combinations on the command line and I couldn't find
>>>> a set of arguments ordering that breaks the terminating quality of
>>>> --module.
>>>>
>>>> I also performed series of tests to try to break the argument expansion on
>>>> Windows with this change, but it worked in all instances. The testcase is
>>>> working correctly with this change, as well as the javac launcher command
>>>> as proposed by Kumar (java --module-path=mods
>>>> --module=jdk.compiler/com.sun.tools.javac.Main *.java).
>>>>
>>>> I ran all the launcher tests on both Windows and Linux and all tests pass.
>>>>
>>>> Please let me know if this is a preferred approach to address this bug or
>>>> if you think there's a potential problem with the change. If this is an
>>>> acceptable fix I can create new webrev with set of tests for the various
>>>> edge cases I tried, and new launcher specific tests that ensure argument
>>>> expansion is performed on Windows with this module usage.
>>>>
>>>> Thank you,
>>>> Nikola
>>>>
>>>> -----Original Message-----
>>>> From: Henry Jen <mailto:mailto:[email protected]>
>>>> Sent: December 4, 2019 8:26 PM
>>>> To: Kumar Srinivasan <mailto:mailto:[email protected]>; Alan Bateman
>>>> <mailto:mailto:[email protected]>; Nikola Grcevski
>>>> <mailto:mailto:[email protected]>
>>>> Cc: mailto:mailto:[email protected]
>>>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>>>
>>>>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan
>>>>> <mailto:mailto:[email protected]> wrote:
>>>>>
>>>>> Hi Nikola,
>>>>>
>>>>> It looks ok to me, I will leave it to Henry and Alan to bless this.
>>>>>
>>>>> IMHO: I think we should fix it correctly now than later, I don't
>>>>> think it is all that difficult AFAICT all the launcher has to do is
>>>>> identify "--module==", then the next argument is the first index.
>>>>>
>>>>
>>>> I don’t disagree, if we can decide whether —module= is allowed. Based on
>>>> my understanding and the document for java[1], the —module= is not
>>>> necessarily correct.
>>>>
>>>> If we decided to accept it, the fix would be make sure the index set
>>>> correctly as Kumar suggested, and the fix can be relatively simple.
>>>>
>>>> FWIW, it’s still possible the index is NOT_FOUND if there is no main class
>>>> specified, but that should never cause crash as if no main class is found,
>>>> the launcher should either execute other terminal argument or display help.
>>>>
>>>> I agree the fix is not complete as it only make sure no crash can happen.
>>>> It doesn’t actually make —module= illegal and show help in case of that.
>>>> At this point, there is a discrepancy in launcher code regard —module
>>>> usage, and we need to fix that.
>>>>
>>>> I’ll let Alan to comment about the —module option usage.
>>>>
>>>> The webrev looks good to me, although I would like to see the test be more
>>>> close to other arguments processing test, but since this can only be
>>>> reproduced with —module= usage, I assume this is not bad.
>>>>
>>>> [1]
>>>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs
>>>> .https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Foracle.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824619848&sdata=362LaE5nipNO0eVmKa%2BmTTwhhBCFfAGdFMFHd%2Bumx4A%3D&reserved=0
>>>> &data=02%7C01%7CNikola.Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824629806&sdata=7VMZnWzyp9ZSG%2FzVaxUMFUuqeikdR%2FIvDGg2p0AqRTU%3D&reserved=0
>>>> e9b608d77a0999a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63711205
>>>> 3962510892&sdata=bh5Phj2Ti%2B%2FWKLK9VfWXIFXVfTRDBOjSkYTkrQ5k%2FvY
>>>> %3D&reserved=0
>>>>
>>>>> Kumar
>>>>>
>>>>> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski
>>>>> <mailto:mailto:[email protected]> wrote:
>>>>> Hi Henry and Kumar,
>>>>>
>>>>> Thanks again for your comments! I have updated the test to be part of
>>>>> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve
>>>>> the same amount of testing. I added a new test method inside
>>>>> BasicTest.java and tested on both Windows and Linux.
>>>>>
>>>>> Please find my updated webrev here for your review:
>>>>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrc
>>>>> e
>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvski.github.io&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824629806&sdata=WMlvgPM2Ixt0WaZiD61ZLjRmSgRL3UDmrNITh7H%2Ft38%3D&reserved=0
>>>>> c
>>>>> evski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824629806&sdata=7VMZnWzyp9ZSG%2FzVaxUMFUuqeikdR%2FIvDGg2p0AqRTU%3D&reserved=0
>>>>> f
>>>>> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=ee0dSSSJ
>>>>> %
>>>>> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=0
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Nikola
>>>>>
>>>>> -----Original Message-----
>>>>> From: Henry Jen <mailto:mailto:[email protected]>
>>>>> Sent: December 3, 2019 11:39 AM
>>>>> To: Kumar Srinivasan <mailto:mailto:[email protected]>
>>>>> Cc: Nikola Grcevski <mailto:mailto:[email protected]>; Alan
>>>>> Bateman
>>>>> <mailto:mailto:[email protected]>;
>>>>> mailto:mailto:[email protected]
>>>>> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
>>>>>
>>>>> Kumar,
>>>>>
>>>>> Great to have you look at this, you are correct, this patch doesn’t
>>>>> address the wildcard expansion issue, but only to address the potential
>>>>> crash if a main class is not specified as Nikola pointed out.
>>>>>
>>>>> We definitely need a follow up to fix wildcard expansion. The pointer to
>>>>> simplify the test is helpful, it would make the test more obvious.
>>>>>
>>>>> Cheers,
>>>>> Henry
>>>>>
>>>>>> On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan
>>>>>> <mailto:mailto:[email protected]> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Sorry for chiming in late in the review process, for what it's worth....
>>>>>>
>>>>>> 1. It is not at all clear to me if this solution is correct, yes it
>>>>>> averts the problem of not finding the main-class
>>>>>> and subsequent crash, but it does not address wildcard arguments
>>>>>> expansion.
>>>>>>
>>>>>> What if we have
>>>>>> % java --module-path=mods
>>>>>> --module=jdk.compiler/com.sun.tools.javac.Main *.java
>>>>>> Where jdk.compiler is a java compiler implementation (javac).
>>>>>> The user would expect the above compiler module to build all the .java
>>>>>> files in that directory,
>>>>>> and this fix will not address that.
>>>>>>
>>>>>> Some background:
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbu
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgs.openjdk.java.net&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824639758&sdata=QyeJDvywLhjIe02uwSYhV2mI2ERcgOhObjk2lDWF8oQ%3D&reserved=0
>>>>>> .Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824639758&sdata=mH19URBhYu0rW%2Bpsl91V9e3TcwPD%2BDX0YSNbXao5H8o%3D&reserved=0
>>>>>> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=b
>>>>>> 0wl3x8AVS%2BJIrHHG5ivM%2FZtfgn2IRno2AauSVcRWlg%3D&reserved=0
>>>>>> Please see all the related bugs in the above JIRA issue.
>>>>>>
>>>>>> Paragraph #6 in this interview surmises the wild card behavior on
>>>>>> Windows:
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fwww.
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fprinceton.edu&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824649719&sdata=K1mAbE9L7km1jXGixjKlWpUFsEpSKxwpsi%2Bqby35nlo%3D&reserved=0
>>>>>> 2%7C01%7CNikola.Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824649719&sdata=%2FUxEEDQrTgIk2Y%2Bd%2FPz67ocYVqRXncW%2FsITD%2BWpemSY%3D&reserved=0
>>>>>> 92228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797
>>>>>> 544&sdata=D1gprSmaQp1v9BB07EmYsX1%2FF4n9CGXMl8yIDJdnQ%2Bg%3D&
>>>>>> ;reserved=0
>>>>>>
>>>>>> 2. Though the arguments related tests are in Aaarghs.java the modules
>>>>>> related tests for the launcher are at:
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fhg
>>>>>> .https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopenjdk.java.net&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824659675&sdata=ED1VtZLjyMmo3uRMRQ02rMCeeHrk0HIlQO8mv2tnPKA%3D&reserved=0
>>>>>> Ftools%2Flauncher%2Fmodules%2Fbasic&data=02%7C01%7CNikola.Grcevs
>>>>>> ki%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824659675&sdata=HvTbDg9uHe5WDu0OVcH%2BPF5mgYHWv2OUHYAWY2EheFo%3D&reserved=0
>>>>>> 41af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=L4SzTviE
>>>>>> WgKoBrrZ2nU%2BPJFhairYv8ZPDqW%2FNtAXEN0%3D&reserved=0
>>>>>> Using the modules test framework may make the test simpler.
>>>>>>
>>>>>> Kumar Srinivasan
>>>>>>
>>>>>>
>>>>>> On Mon, Dec 2, 2019 at 11:34 AM Nikola Grcevski
>>>>>> <mailto:mailto:[email protected]> wrote:
>>>>>> Hi Alan and Henry,
>>>>>>
>>>>>> Thank you for reviewing my email! Henry's observation matches mine, the
>>>>>> shared common code for all platforms in checkArg
>>>>>> (src/java.base/share/native/libjli/args.c) can potentially leave the
>>>>>> firstAppArgIndex static set to -1, if all passed command line arguments
>>>>>> are prefixed with a dash. Later on the arguments are validated, however
>>>>>> we might crash before then on Windows because of the negative index
>>>>>> access. In this case, the customer command was valid (modules usage) and
>>>>>> the program runs successfully.
>>>>>>
>>>>>> I created a webrev here for the change, including a new test in
>>>>>> Arrrghs.java:
>>>>>>
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgr
>>>>>> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fcevski.github.io&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824669630&sdata=fi870VNLFmOgySkWY1LJJ2qBSKZkdTaIEW6ytg%2FMHDA%3D&reserved=0
>>>>>> .Grcevski%https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2F40microsoft.com&data=02%7C01%7CNikola.Grcevski%40microsoft.com%7Cefcfbecd0afc44ef1ee808d77ddb7496%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637116253824669630&sdata=XX0qBV1e5ucqPSL%2B4YciHNXR29evREuIRDFwTSxaqHU%3D&reserved=0
>>>>>> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544&sdata=e
>>>>>> e0dSSSJ%2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3D&reserved=0
>>>>>>
>>>>>> I copied the test validation and assertion style of other code inside
>>>>>> the test class.
>>>>>>
>>>>>> Please let me know if you have any comments or suggestions.
>>>>>>
>>>>>> Thanks again!
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Henry Jen <mailto:mailto:[email protected]>
>>>>>> Sent: December 2, 2019 12:26 PM
>>>>>> To: Alan Bateman <mailto:mailto:[email protected]>
>>>>>> Cc: Nikola Grcevski <mailto:mailto:[email protected]>;
>>>>>> mailto:mailto:[email protected]
>>>>>> Subject: [EXTERNAL] Re: JDK-8234076 bug fix candidate
>>>>>>
>>>>>> The fix looks reasonable to me, basically, if the command argument
>>>>>> format is not legal, it’s possible we won’t find the main class when
>>>>>> doing argument file expansion, and the index is -1 which could cause
>>>>>> crash on Windows platform for the wildcard processing.
>>>>>>
>>>>>> I think we should add a test case for this, probably in
>>>>>> test/jdk/tools/launcher/Arrrghs.java.
>>>>>>
>>>>>> As I understand it, the argument validation is done in a later stage
>>>>>> after calling JLI_Launch.
>>>>>>
>>>>>> Cheers,
>>>>>> Henry
>>>>>>
>>>>>>
>>>>>>> On Dec 2, 2019, at 2:12 AM, Alan Bateman
>>>>>>> <mailto:mailto:[email protected]> wrote:
>>>>>>>
>>>>>>> On 20/11/2019 19:42, Nikola Grcevski wrote:
>>>>>>>> :
>>>>>>>>
>>>>>>>> Please let me know if this approach is acceptable for the current bug
>>>>>>>> report and I'll make a webrev and include appropriate launcher tests.
>>>>>>>> I was thinking the tests should do extra validation on the output from
>>>>>>>> _JAVA_LAUNCHER_DEBUG on Windows.
>>>>>>>>
>>>>>>> I think you're in the right area but I would have expected the arg
>>>>>>> index to 0 here. Henry Jen (cc'ed) might have some comments on this.
>>>>>>>
>>>>>>> -Alan
>
