RFR JDK-8159206 - All jlink tests failing

[Jim Laskey (Oracle)]

> On Jun 11, 2016, at 9:03 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> 
>> On Jun 10, 2016, at 12:20 PM, Jim Laskey (Oracle) <james.laskey at oracle.com> wrote:
>> 
>> http://cr.openjdk.java.net/~jlaskey/8159206/webrev/index.html <http://cr.openjdk.java.net/~jlaskey/8159206/webrev/index.html>https://bugs.openjdk.java.net/browse/JDK-8159206 <https://bugs.openjdk.java.net/browse/JDK-8159206>
>> 
> 
> 
> ResourceFilter.java - since you are in this file, we could take the opportunity to improve the code.
> 
> 54     public ResourceFilter(String[] patterns, boolean exclude) throws IOException {
> 
> ResourceFilter is constructed by several plugins with this pattern:
>   predicate = new ResourceFilter(Utils.listParser.apply(exclude), true);
> 
> or IncludeLocalesPlugin creates the filter with exclude=false:
>   predicate = new ResourceFilter(Utils.listParser.apply(value), false);
> 
> I suggest to change ResourceFilter to define static factory methods such as:
>   static ResourceFilter excludeFilter(List<String> patterns)
>   static ResourceFilter includeFilter(List<String> patterns)
> 
> Alss Util::listParser returns a List<String> instead of String[] and return an empty list when argument == null.
> 
> This pattern is used in multiple places.
> private static final FileSystem JRT_FILE_SYSTEM = Utils.jrtFileSystem();
> PathMatcher matcher = Utils.getPathMatcher(JRT_FILE_SYSTEM, line);
> 
> You can use Utils::getPathMatcher(String pattern) and cache JRT_FILE_SYSTEM in Utils class.


Done.  Webrev updated.


> 
>  62                     if (file.exists()) {
> 
> Should it fail if the file does not exist?

Resistance from Claes and build team on this.  Silent fail keeps jlink scripts simple.  I added a warning message if the file is missing.


> 
> test/tools/jlink/plugins/CompressorPluginTest.java
> 106                 "*Exception.class,^*IOException.class");
> 
> The original test has includes and excludes.  Perhaps you can use regex syntax here.  New test cases with regex: and glob: would be good to add (the existing tests cover the default without specifying the syntax).

Left test alone.  Added variations of patterns to the ResourceFilterTest (proper place.)

> 
> I also reviewed this patch together with JDK-8158402 which causes this regression:
>  http://cr.openjdk.java.net/~jlaskey/8158402/webrev
> 
> My suggestion on the help message 
> 
> $ jmod —-help
> 
>  --exclude <pattern>                Exclude files, given as a PATTERN    
>  --hash-modules <pattern> 
> 
> The default for exclude pattern is glob-syntax pattern whereas hash-modules is a regex pattern of module names.  It may be okay to overload —-exclude option to accept “regex:<pattern>” and the description needs to be made clear.  Alternative is to have a different option to take regex. 
> 
> Maybe 
> —-exclude [syntax:]<pattern> 
> --hash-modules <pattern>
> 
> and specify the valid syntax.
> 
> $ jlink —-list-plugins
> 
> Plugin Name: exclude-files
> Option: --exclude-files=<files to exclude | files of excluded files>
> Description: Specify files to exclude. e.g.: *.diz, /java.base/native/client/*
> 
> Plugin Name: exclude-resources
> Option: --exclude-resources=<resources to exclude | file of excluded resources>
> Description: Specify resources to exclude. e.g.: *.jcov, */META-INF/*
> 
> Plugin Name: order-resources
> Option: --order-resources=<paths in priority order | file with resource paths>
> Description: Order resources. e.g.: */module-info.class,/java.base/java/lang/*
> 
> The option and examples need update to use syntax:pattern and states what are valid syntax and the default.
> 
> $ jimage —-help
> 
>  --filter                          Filter entries for list or extract
>                                    Ex. /java.base/*, */module-info.class
> 
> should be 
>   —-filter [syntax:]pattern>
> 
> I assume you meant to give example “/java.base/**” to list all entries in java.base.  Maybe show a regex example.

Since [syntax:]<pattern> is used in many many places, I added a general description of patterns to jmod/jimage/jlink and kept the individual options description simple.


> 
> Notice that jlink uses “=“ as the option separator whereas jmod and jimage use space - just to mention it and CLI option consistency is on the list to be examined.

It seems both “=" and space work (on the Mac at least.)  Claes mentioned that =/space was problematic on Windows.

> 
> Mandy
>