Hi Kevin,
See below.
On 5/2/2019 5:38 PM, Kevin Rushforth wrote:
Here are a few follow-on comments. As with my earlier comments, none
of these need to be addressed prior to integration.
1. I found a few more classes that do I/O and could benefit from using
try-with-resources:
IOUtils, LinuxAppImageBuilder, LinuxDebBundler, LinuxRpmBundler,
MacAppImageBuilder, etc.
JLinkBundlerHelper.java:
2. JRE_MODULES_FILENAME and SERVER_JRE_MODULES_FILENAME are unused
(obsolete) and should be removed.
VersionExtractor.java:
3. The isLessThan method only looks at MAJOR.MINOR so might not be
flexible enough for some applications
Currently it is being used for InnoSetup and Wixs and we only need to
compare major.minor, so should be fine for now.
LinuxAppBundler.java:
3. Several places where non-public (package-scope) API is exported
publicly; these should all be package-scope itself or else
BundlerParamInfo should be public
LinuxAppImageBuilder.java:
3. createUtf8File is unused (I went looking because I was curious how
and why we would use such a method). I see similarly-unused methods of
the same name in the Mac and Windows AppImageBuilder classes.
MacAppImageBuilder.java:
4. Line 818: is the following still needed?
|| p.toString().contains(
"/Contents/MacOS/JavaAppletPlugin")
WindowsDefender.java + WindowsRegistry.java:
5. Is the check and warning for Windows Defender really needed? Have
we seen problems as a result of it running while jpackage is building
an app installer?
Yes, it is still needed. Resource update fails sometimes and I was able
to reproduce this issue. However, not very often.
Thanks,
Alexander
