Hello Mandy,
As I understand all these options are tested by following tests:
jdk/test/tools/launcher/modules. I.e. no additional tests are required
for moving these options to the VM(except 2 new tests)?
You only adds 2 additional tests:
RuntimeArguments.java - to check that these options are VM options
IgnoreModulePropertiesTest.java - to check that java throws an exception
for invalid values and that corresponding properties are ignored.
I have a question about one part of the IgnoreModulePropertiesTest.java
test:
38 // Test that the specified property and its value are
ignored. If the VM accepted
39 // the property and value then an exception would be thrown
because the value is
40 // bogus for that property. But, since the property is
ignored no exception is
41 // thrown.
42 public static void testProperty(String prop, String value)
throws Exception {
43 ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
44 "-D" + prop + "=" + value, "-version");
45 OutputAnalyzer output = new OutputAnalyzer(pb.start());
46 output.shouldContain("java version ");
47 output.shouldHaveExitValue(0);
48
49 // Ensure that the property and its value aren't available.
50 if (System.getProperty(prop) != null) {
51 throw new RuntimeException(
52 "Unexpected non-null value for property " + prop);
53 }
54 }
Lines 49-53 verifies that property not exist, but this property is
obtained from the main test and not from the launched vm(launched with
"-D" + prop + "=" + value). Is it intended?
Thanks,
Dmitry
On 08.06.2016 19:35, Mandy Chung wrote:
Updated webrev with Harold’s latest VM change incorporating the review comments:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.01/index.html
Harold - the revised VM change looks okay to me.
Minor one: you define the following:
202 #define MODULE_PATH_PROPERTY "-Djdk.module.path”
204 #define MODULE_UPGRADE_PATH_PROPERTY "-Djdk.module.upgrade.path"
It may be good to consider having #define for all module property names and
used consistently.
Mandy
On Jun 3, 2016, at 11:47 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8136930/webrev.00/
-modulepath, -addmods, -limitmods, -XaddExports, -XaddReads, -Xpatch are java
launcher options in the current implementation. Custom launchers will have to
use -D to set some system properties to configure module system. Different
ways to configure module system is confusing and not friendly for environments
using both java launcher and custom launchers.
This patch pushes the handling of the module options into the VM. That will
avoid the confusion between launcher and VM options and avoids needing to use
system properties. All launcher implementations can configure the module
system via JNI Invocation API setting these options in a unified way. The
options and syntax remain the same as specified in JEP 261.
For the non-repeating options, like the other VM options, the last one wins.
The current implementation communicates the options to the module system
through system properties, as a private interface, and these system properties
will be removed once they are read during the module system initialization.
These system properties are reserved as private interface and they will be
ignored if they are set via -D in the command line. Harold implements the
hotspot change and can explain further details.
This patch will impact existing tests and scripts that set the system properties for
example to break encapsulation in the command line e.g.
-Djdk.launcher.addexports.<N>. They will need to be updated to replace the use
of -D with the appropriate module option e.g. -XaddExports. Since they are new
options in JDK 9, use -XX:+IgnoreUnrecognizedVMOptions if they need to be ignored by
earlier releases.
Mandy
