Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
Looks good. On 11/8/2019 11:38 AM, Andy Herrick wrote: Please review the revised jpackage fix for bug [1] at [3]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). This webrev (webrev.03) addresses feedback from previous version (webrev.02). [1] https://bugs.openjdk.java.net/browse/JDK-8233636 [2] https://cr.openjdk.java.net/~herrick/8233636/webrev.02 [3] https://cr.openjdk.java.net/~herrick/8233636/webrev.03 /Andy
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
Looks good. - Alexey On 11/8/2019 2:38 PM, Andy Herrick wrote: Please review the revised jpackage fix for bug [1] at [3]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). This webrev (webrev.03) addresses feedback from previous version (webrev.02). [1] https://bugs.openjdk.java.net/browse/JDK-8233636 [2] https://cr.openjdk.java.net/~herrick/8233636/webrev.02 [3] https://cr.openjdk.java.net/~herrick/8233636/webrev.03 /Andy
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
Please review the revised jpackage fix for bug [1] at [3]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). This webrev (webrev.03) addresses feedback from previous version (webrev.02). [1] https://bugs.openjdk.java.net/browse/JDK-8233636 [2] https://cr.openjdk.java.net/~herrick/8233636/webrev.02 [3] https://cr.openjdk.java.net/~herrick/8233636/webrev.03 /Andy
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
On 11/7/2019 5:38 PM, Alexey Semenyuk wrote:
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50
I'd suggest to replace
---
if (name.equals("jpackage"))
---
with more robust
---
if (this == JPACKAGE)
---
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html:
Did you drop JPackageCommand.filterOutput() calls intentionally?
yes - this form of filter just removes the first line (where the other
one removes a line starting with some specific strings). When we were
printing the warning ourselves, we would always print it. Now that we
are causing the system to print it by having the "jdk.incubator" package
name prefix, waning is printed when we invoke the tool, but not when we
call into it's API (that warning was printed when we compiled the code
that was linked to that package).
Anyway - these two tests began failing when we changed package names,
because we were filtering out the first line of the output, then not
matching.
side note - I noticed a bug in what is printed (in no arg case):
MSG_Help_no_args=Usage: jpackage \n\
\Use jpackage --help (or -h) for a list of possible options\
there should be no more in Usage.
(will fix this as part of JDK-8233591
)
/Andy
The rest looks good.
- Alexey
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
On 11/7/19 1:45 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). This changes the module name, and base package name from jdk.jpackage to jdk.incubator.jpackage. This removes the "provides" statement in module-info so jdk.incubator.jpackage can not directly be discovered as a ToolProvider, but it also exports jdk.incubator.jpackage which now contains ToolProviderFactory.java which can be used for the same purpose. [1] https://bugs.openjdk.java.net/browse/JDK-8233636 [2] https://cr.openjdk.java.net/~herrick/8233636/webrev.02 make/common/Modules.gmk src/java.base/share/classes/module-info.java - it'd be good to keep the list in alphabetical order jdk.incubator.jpackage.ToolProviderFactory is exported and needs the specification and javadoc. 34 private static ToolProvider provider = new JPackageToolProvider(); Nit: this can be final field. Mandy
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
Hi Andy,
Agree with Alexey comments. Otherwise looks good.
Thanks,
Alexander
On 11/7/2019 2:44 PM, Andy Herrick wrote:
On 11/7/2019 5:38 PM, Alexey Semenyuk wrote:
I guess the link for the review is
https://cr.openjdk.java.net/~herrick/8233636/webrev.02
sry: yes webrev is http://cr.openjdk.java.net/~herrick/8233636/webrev.02
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50
I'd suggest to replace
---
if (name.equals("jpackage"))
---
sure, I can do that.
with more robust
---
if (this == JPACKAGE)
---
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html:
Did you drop JPackageCommand.filterOutput() calls intentionally?
The rest looks good.
- Alexey
On 11/7/2019 4:45 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox
repository (jpackage).
This changes the module name, and base package name from
jdk.jpackage to jdk.incubator.jpackage.
This removes the "provides" statement in module-info so
jdk.incubator.jpackage can not directly be discovered as a
ToolProvider, but it also exports jdk.incubator.jpackage which now
contains ToolProviderFactory.java which can be used for the same
purpose.
[1] https://bugs.openjdk.java.net/browse/JDK-8233636
[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02
/Andy
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
On 11/7/2019 5:38 PM, Alexey Semenyuk wrote:
I guess the link for the review is
https://cr.openjdk.java.net/~herrick/8233636/webrev.02
sry: yes webrev is http://cr.openjdk.java.net/~herrick/8233636/webrev.02
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50
I'd suggest to replace
---
if (name.equals("jpackage"))
---
sure, I can do that.
with more robust
---
if (this == JPACKAGE)
---
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html:
Did you drop JPackageCommand.filterOutput() calls intentionally?
The rest looks good.
- Alexey
On 11/7/2019 4:45 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox
repository (jpackage).
This changes the module name, and base package name from jdk.jpackage
to jdk.incubator.jpackage.
This removes the "provides" statement in module-info so
jdk.incubator.jpackage can not directly be discovered as a
ToolProvider, but it also exports jdk.incubator.jpackage which now
contains ToolProviderFactory.java which can be used for the same
purpose.
[1] https://bugs.openjdk.java.net/browse/JDK-8233636
[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02
/Andy
Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation
I guess the link for the review is
https://cr.openjdk.java.net/~herrick/8233636/webrev.02
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50
I'd suggest to replace
---
if (name.equals("jpackage"))
---
with more robust
---
if (this == JPACKAGE)
---
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html:
Did you drop JPackageCommand.filterOutput() calls intentionally?
The rest looks good.
- Alexey
On 11/7/2019 4:45 PM, Andy Herrick wrote:
Please review the jpackage fix for bug [1] at [2].
This is a fix for the JDK-8200758-branch branch of the open sandbox
repository (jpackage).
This changes the module name, and base package name from jdk.jpackage
to jdk.incubator.jpackage.
This removes the "provides" statement in module-info so
jdk.incubator.jpackage can not directly be discovered as a
ToolProvider, but it also exports jdk.incubator.jpackage which now
contains ToolProviderFactory.java which can be used for the same purpose.
[1] https://bugs.openjdk.java.net/browse/JDK-8233636
[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02
/Andy
