Thank you, Mandy!
Comments inline.
> On Mar 6, 2017, at 6:00 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
>
>
>> On Mar 6, 2017, at 5:27 PM, Alexandre (Shura) Iline
>> <alexandre.il...@oracle.com> wrote:
>>
>> Hi,
>>
>> Could you please review the suggested fox for:
>> https://bugs.openjdk.java.net/browse/JDK-8159523
>>
>> There has been a bit of discussion on jigsaw-dev, but it perhaps make sense
>> to include core-libs-dev.
>>
>> There have been some fixes since the review was published, so we are now at
>> revision #4:
>> http://cr.openjdk.java.net/~shurailine/8159523/webrev.04/
>>
>
> I reviewed webrev.04 which is in a good shape. Here are my comments:
>
> W.r.t. the JavaTask methods:
>
> 110 new JavaTask()
> 111 .addExports("java.base", "jdk.internal.reflect")
> 112 .vmOptions("-version")
> 113 .run();
>
> I prefer it to use <module>/<package> to represent the source in the same way
> as the command-line argument to `—-add-exports` option, as Alan suggests.
I see a point in doing that. At the same time I want to point out that there
will be cases when the user will be forced to do a concatenation such as
.addExports(JAVA_BASE + “/“ + JDK_MISC, ALL_UNNAMED)
i.e. JAVA_BASE is a constant which is used multiple times in the code.
This not a deal breaker - I will change it in the next iteration.
> Also specifying the target explicitly makes the test clearer what it does.
>
> .addExports(“java.base/jdk.internal.reflect”, “ALL-UNNAMED”)
Makes sense.
I hope though that you are not against having a constant for “ALL-UNNAMED”,
such as Task.ALL_UNNAMED. A lot of people, myself included prefer usage of
constants over the copy-pasted value.
>
> Similiarly, module(String mid) takes the argument passed to `—-module`.
>
> addReads(String source, String… targets)
> I suggest to require a non-empty targets. i.e. ALL-UNNAMED must be
> specified.
>
> `—-add-modules` is a repeating option and JavaTask::addModules does not
> support that.
OK.
>
> 148 .addModules("jdk.naming.dns,jdk.compiler”)
>
> This should be “addModules(“jdk.naming.dns”, “jdk.compiler”)”
>
> JavaTask should be extended for the new `-—add-opens` option
Yes.
>
> classArgs(String… args) might be better to rename to mainArgs(String… args).
OK.
>
> modulePath, upgradeModulePath, classPath - maybe just take Path arguments.
>
> 136 .shouldContain(Task.OutputKind.STDERR, "IllegalAccessError");
> 164 .shouldContain(Task.OutputKind.STDOUT, "specified more than
> once”);
>
> It may be useful to add “stdoutShouldContain” and “stderrShouldContain”
> method, like what ProcessTools provides.
There will be a further discussion on this - Igor will be summarizing it in a
separate e-mail. What he will be suggesting is
.stderr().shouldContain(“IllegalAccessError”)
or something similar.
>
> ignoreStandardOptions()
> ignoreStandardModuleOptions()
> It might be confusing what “standard options” are. They are the options
> configured by jtreg and inherited from the test run.
>
> What about “doNotInheritTestOptions” or “doNotInheritTestRuntimeOptions”?
Sounds good to me.
>
> It seems better for JavaTask constructor to take a boolean/enum flag. Instead
> of a public JavaTask constructor, what about JavaTask.newTask(Enum) or
> JavaTask.newTaskWithInheritTestRuntimeOptions() explicit factory methods?
> Either way is fine with me.
>
> public JavaTask ignoreStandardOption(String option, int parameterCount) {
> public JavaTask filterStandardOption(Function<List<String>, List<String>>
> filter) {
> - should these methods be private?
Yes for filterStandardOption(…).
I meant ignoreStandardOption(String, int) as a public API. Some options are
supported by JavaOptions, but there could be others which are not. But I agree
that we can hide it for now - there may never be a case for it.
>
> what if classArgs and module are both called? The builder should detect that
> and throw IAE to help troubleshooting since the main class should either be
> in a named module or just class name.
Yes.
>
> TaskError - Alan suggests to be TaskException extends RuntimeException
> instead.
OK.
>
> AbstractTask.java
>
> System.out.println("Running " + pb.command().stream().map(s -> "\"" + s +
> "\"").collect(Collectors.joining(",")));
>
> - it would be useful to print the command-line that I can cut-n-paste to a
> shell and rerun.
Makes sense.
>
> i.e. no need to wrap with double quotes except -classpath and --module-path
> etc. Use space rather than “,” as separator.
>
> Task.Mode.* is defined to launch via different mechanism. For java launcher,
> it has only one way. For jar, jlink, jmod and other tools, we can now use
> java.util.ToolProvider. It might be useful if we had a JlinkTask in the
> future. We should either drop Task.Mode or update its comments.
>
> Should we remove the use of JDKToolFinder since I assume jdk.testlibrary.task
> will replace jdk.testlibrary.* helper classes in the future.
Probably also make it private for now. Or remove - we can re-introduce it later.
>
> I suggest the package name be “jdk.testlibrary.task” singular rather than
> plural.
ok
It will also need to go to the top-level test library, to allow HS team to use
it.
Shura
>
> Mandy
>
