> On Jun 11, 2016, at 9:03 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>
>
>> On Jun 10, 2016, at 12:20 PM, Jim Laskey (Oracle) <james.las...@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
>
