Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Jim Laskey (Oracle)]

I’ll followup shortly.


> On Jun 9, 2016, at 11:51 AM, Alan Bateman  wrote:
> 
> 
> On 09/06/2016 12:00, Jim Laskey (Oracle) wrote:
>> :
>> I have changes for the make files as well.
>> 
>> diff -r 6a78ef1d1e58 make/Images.gmk
>> --- a/make/Images.gmkMon May 30 16:17:11 2016 +0200
>> +++ b/make/Images.gmkWed Jun 08 15:24:53 2016 -0300
>> @@ -115,16 +115,16 @@
>>  # Use this file inside the image as target for make rule
>>  JIMAGE_TARGET_FILE := bin/java$(EXE_SUFFIX)
>>  -JLINK_ORDER_RESOURCES := *module-info.class*
>> +JLINK_ORDER_RESOURCES := **module-info.class
>>  ifeq ($(ENABLE_GENERATE_CLASSLIST), true)
>>JLINK_ORDER_RESOURCES += @$(SUPPORT_OUTPUTDIR)/classlist/classlist
>>  endif
>>  JLINK_ORDER_RESOURCES += \
>> -/java.base/java/* \
>> -/java.base/jdk/* \
>> -/java.base/sun/* \
>> -/java.base/com/* \
>> -/jdk.localedata/* \
>> +/java.base/java/** \
>> +/java.base/jdk/** \
>> +/java.base/sun/** \
>> +/java.base/com/** \
>> +/jdk.localedata/** \
>>  #
>>JLINK_TOOL := $(JLINK) --modulepath $(IMAGES_OUTPUTDIR)/jmods \
>> 
>> 
> This looks okay. So are there changes to the usage messages?
> 
> -Alan

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Alan Bateman]

On 09/06/2016 12:00, Jim Laskey (Oracle) wrote:

:
I have changes for the make files as well.

diff -r 6a78ef1d1e58 make/Images.gmk
--- a/make/Images.gmk   Mon May 30 16:17:11 2016 +0200
+++ b/make/Images.gmk   Wed Jun 08 15:24:53 2016 -0300
@@ -115,16 +115,16 @@
  # Use this file inside the image as target for make rule
  JIMAGE_TARGET_FILE := bin/java$(EXE_SUFFIX)
  
-JLINK_ORDER_RESOURCES := *module-info.class*

+JLINK_ORDER_RESOURCES := **module-info.class
  ifeq ($(ENABLE_GENERATE_CLASSLIST), true)
JLINK_ORDER_RESOURCES += @$(SUPPORT_OUTPUTDIR)/classlist/classlist
  endif
  JLINK_ORDER_RESOURCES += \
-/java.base/java/* \
-/java.base/jdk/* \
-/java.base/sun/* \
-/java.base/com/* \
-/jdk.localedata/* \
+/java.base/java/** \
+/java.base/jdk/** \
+/java.base/sun/** \
+/java.base/com/** \
+/jdk.localedata/** \
  #
  
  JLINK_TOOL := $(JLINK) --modulepath $(IMAGES_OUTPUTDIR)/jmods \




This looks okay. So are there changes to the usage messages?

-Alan

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Alan Bateman]

On 09/06/2016 12:52, Chris Hegarty wrote:


:
As things currently stand --hash-modules accepts a regex, and --exclude
accepts a glob. So this change does not affect default behaviour.

It makes sense for --exclude to take a glob ( and optionally a regex ).
I don’t think it makes sense for --hash-modules to take anything other
than a regex.

Are there issues with this?

Just consistency as it's not clear how the user of the tools knows what 
to use. I assume Jim will post an updated patch with the usage messages 
and we can see how it looks.


-Alan

Re: Review Request JDK-8136930 Examine implications for custom launchers, equivalent of java -X options in particular

[Dmitry Dmitriev]

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  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..  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

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Chris Hegarty]

> On 9 Jun 2016, at 12:57, Jim Laskey (Oracle)  wrote:
> 
> 
>> On Jun 9, 2016, at 8:52 AM, Chris Hegarty  wrote:
>> 
>>> 
>>> On 9 Jun 2016, at 11:16, Alan Bateman  wrote:
>>> 
>>> On 08/06/2016 20:45, Jim Laskey (Oracle) wrote:
>>> 
 Consistent use of PathPatterns for jlink, jimage and jmod options.
 
 —optionName=(regex:|glob:|) ?? where  => 
 glob:
 
 http://cr.openjdk.java.net/~jlaskey/8158402/webrev/index.html 
 
 https://bugs.openjdk.java.net/browse/JDK-8158402 
 
 
>>> This look okay to me but two questions:
>>> 
>>> 1. Do any of the usage resources need to be updated?
>>> 
>>> 2. Does this introduce an inconsistency in the jmod tool in that 
>>> --hash-modules takes a regex whereas --exclude takes a pattern that is a 
>>> glob (at least by default, it could be a regex too if prefixed with 
>>> "regex:").
>> 
>> As things currently stand --hash-modules accepts a regex, and --exclude 
>> accepts a glob. So this change does not affect default behaviour.
>> 
>> It makes sense for --exclude to take a glob ( and optionally a regex ).
>> I don’t think it makes sense for --hash-modules to take anything other
>> than a regex.
>> 
>> Are there issues with this?
>> 
>> Jim,   can you please rename GlobConverter to PatternConverter or 
>> ExcludeConverter,
>> or similar.
> 
> diff -r 766e969fc3e0 src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
> --- a/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.javaThu Jun 
> 09 07:55:38 2016 -0300
> +++ b/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.javaThu Jun 
> 09 08:56:46 2016 -0300
> @@ -1072,6 +1072,10 @@
> @Override
> public Pattern convert(String value) {
> try {
> +if (value.startsWith("regex:")) {
> +value = value.substring("regex:".length()).trim();
> +}
> +
> return Pattern.compile(value);
> } catch (PatternSyntaxException e) {
> throw new CommandException("err.bad.pattern", value);
> @@ -1083,7 +1087,7 @@
> @Override public String valuePattern() { return "pattern"; }
> }
> 
> -static class GlobConverter implements ValueConverter {
> +static class PathMatcherConverter implements ValueConverter 
> {
> @Override
> public PathMatcher convert(String pattern) {
> try {
> @@ -1199,7 +1203,7 @@
> OptionSpec excludes
> = parser.accepts("exclude", getMessage("main.opt.exclude"))
> .withRequiredArg()
> -.withValuesConvertedBy(new GlobConverter());
> +.withValuesConvertedBy(new PathMatcherConverter());
> 
> OptionSpec hashModules
> = parser.accepts("hash-modules", 
> getMessage("main.opt.hash-modules”))

That looks fine. Thanks.

-Chris.

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Jim Laskey (Oracle)]

> On Jun 9, 2016, at 8:52 AM, Chris Hegarty  wrote:
> 
>> 
>> On 9 Jun 2016, at 11:16, Alan Bateman  wrote:
>> 
>> On 08/06/2016 20:45, Jim Laskey (Oracle) wrote:
>> 
>>> Consistent use of PathPatterns for jlink, jimage and jmod options.
>>> 
>>> —optionName=(regex:|glob:|) ?? where  => 
>>> glob:
>>> 
>>> http://cr.openjdk.java.net/~jlaskey/8158402/webrev/index.html 
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8158402 
>>> 
>>> 
>> This look okay to me but two questions:
>> 
>> 1. Do any of the usage resources need to be updated?
>> 
>> 2. Does this introduce an inconsistency in the jmod tool in that 
>> --hash-modules takes a regex whereas --exclude takes a pattern that is a 
>> glob (at least by default, it could be a regex too if prefixed with 
>> "regex:").
> 
> As things currently stand --hash-modules accepts a regex, and --exclude 
> accepts a glob. So this change does not affect default behaviour.
> 
> It makes sense for --exclude to take a glob ( and optionally a regex ).
> I don’t think it makes sense for --hash-modules to take anything other
> than a regex.
> 
> Are there issues with this?
> 
> Jim,   can you please rename GlobConverter to PatternConverter or 
> ExcludeConverter,
> or similar.

diff -r 766e969fc3e0 src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
--- a/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java  Thu Jun 09 
07:55:38 2016 -0300
+++ b/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java  Thu Jun 09 
08:56:46 2016 -0300
@@ -1072,6 +1072,10 @@
 @Override
 public Pattern convert(String value) {
 try {
+if (value.startsWith("regex:")) {
+value = value.substring("regex:".length()).trim();
+}
+
 return Pattern.compile(value);
 } catch (PatternSyntaxException e) {
 throw new CommandException("err.bad.pattern", value);
@@ -1083,7 +1087,7 @@
 @Override public String valuePattern() { return "pattern"; }
 }
 
-static class GlobConverter implements ValueConverter {
+static class PathMatcherConverter implements ValueConverter {
 @Override
 public PathMatcher convert(String pattern) {
 try {
@@ -1199,7 +1203,7 @@
 OptionSpec excludes
 = parser.accepts("exclude", getMessage("main.opt.exclude"))
 .withRequiredArg()
-.withValuesConvertedBy(new GlobConverter());
+.withValuesConvertedBy(new PathMatcherConverter());
 
 OptionSpec hashModules
 = parser.accepts("hash-modules", 
getMessage("main.opt.hash-modules"))

> 
> -Chris.

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Chris Hegarty]

> On 9 Jun 2016, at 11:16, Alan Bateman  wrote:
> 
> On 08/06/2016 20:45, Jim Laskey (Oracle) wrote:
> 
>> Consistent use of PathPatterns for jlink, jimage and jmod options.
>> 
>> —optionName=(regex:|glob:|) ?? where  => 
>> glob:
>> 
>> http://cr.openjdk.java.net/~jlaskey/8158402/webrev/index.html 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8158402 
>> 
>> 
> This look okay to me but two questions:
> 
> 1. Do any of the usage resources need to be updated?
> 
> 2. Does this introduce an inconsistency in the jmod tool in that 
> --hash-modules takes a regex whereas --exclude takes a pattern that is a glob 
> (at least by default, it could be a regex too if prefixed with "regex:").

As things currently stand --hash-modules accepts a regex, and --exclude 
accepts a glob. So this change does not affect default behaviour.

It makes sense for --exclude to take a glob ( and optionally a regex ).
I don’t think it makes sense for --hash-modules to take anything other
than a regex.

Are there issues with this?

Jim,   can you please rename GlobConverter to PatternConverter or 
ExcludeConverter,
or similar.

-Chris.

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Jim Laskey (Oracle)]

> On Jun 9, 2016, at 7:16 AM, Alan Bateman  wrote:
> 
> On 08/06/2016 20:45, Jim Laskey (Oracle) wrote:
> 
>> Consistent use of PathPatterns for jlink, jimage and jmod options.
>> 
>> —optionName=(regex:|glob:|) ?? where  => 
>> glob:
>> 
>> http://cr.openjdk.java.net/~jlaskey/8158402/webrev/index.html 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8158402 
>> 
>> 
> This look okay to me but two questions:
> 
> 1. Do any of the usage resources need to be updated?

I have changes for the make files as well. 

diff -r 6a78ef1d1e58 make/Images.gmk
--- a/make/Images.gmk   Mon May 30 16:17:11 2016 +0200
+++ b/make/Images.gmk   Wed Jun 08 15:24:53 2016 -0300
@@ -115,16 +115,16 @@
 # Use this file inside the image as target for make rule
 JIMAGE_TARGET_FILE := bin/java$(EXE_SUFFIX)
 
-JLINK_ORDER_RESOURCES := *module-info.class*
+JLINK_ORDER_RESOURCES := **module-info.class
 ifeq ($(ENABLE_GENERATE_CLASSLIST), true)
   JLINK_ORDER_RESOURCES += @$(SUPPORT_OUTPUTDIR)/classlist/classlist
 endif
 JLINK_ORDER_RESOURCES += \
-/java.base/java/* \
-/java.base/jdk/* \
-/java.base/sun/* \
-/java.base/com/* \
-/jdk.localedata/* \
+/java.base/java/** \
+/java.base/jdk/** \
+/java.base/sun/** \
+/java.base/com/** \
+/jdk.localedata/** \
 #
 
 JLINK_TOOL := $(JLINK) --modulepath $(IMAGES_OUTPUTDIR)/jmods \


> 
> 2. Does this introduce an inconsistency in the jmod tool in that 
> --hash-modules takes a regex whereas --exclude takes a pattern that is a glob 
> (at least by default, it could be a regex too if prefixed with "regex:”).

I was taking Chris’ advisement on this.

> 
> -Alan

Re: RFR: JDK-8158402 - jlink: should use regex for all pattern operations (--order-resources or --exclude-resources)

[Alan Bateman]

On 08/06/2016 20:45, Jim Laskey (Oracle) wrote:


Consistent use of PathPatterns for jlink, jimage and jmod options.

—optionName=(regex:|glob:|) ?? where  => 
glob:

http://cr.openjdk.java.net/~jlaskey/8158402/webrev/index.html 

https://bugs.openjdk.java.net/browse/JDK-8158402 



This look okay to me but two questions:

1. Do any of the usage resources need to be updated?

2. Does this introduce an inconsistency in the jmod tool in that 
--hash-modules takes a regex whereas --exclude takes a pattern that is a 
glob (at least by default, it could be a regex too if prefixed with 
"regex:").


-Alan

Re: Review Request JDK-8136930 Examine implications for custom launchers, equivalent of java -X options in particular

[Alan Bateman]

On 08/06/2016 23:37, Coleen Phillimore wrote:


:

I have to ask why Hotspot convention was violated with this new option 
syntax?  These options don't start with -X and the values aren't 
specified as : separators like the rest? Like 
-Xshare:{dump,auto,on,off}, etc.  Why are these different?
I see Mandy has pointed to -agentlib, -javaagent and others which are 
also handled by the VM.


In general then distinction between so-called standard and -X options 
has been blurred for many years. At some point we need to take a pass 
over the new options and decide whether they should be -X or not.  -X is 
particularly annoying when you start to translate the option names GNU 
style.



:

Someone already asked this but are there tests for all these 
combinations of options?
The main tests for these options, and combinations, are in 
jdk/tests/launcher/modules.


-Alan