On 5/7/2019 4:39 PM, Roger Riggs wrote:
Hi,
Additional comments/observations on the jpackage webrev.
Cleanup opportunities for more maintainable code.
When using the ToolProvider API in an application that has a security
manager,
what permissions must be available? Properties, files, ...?
Main:
91: "processArguments" seems misnamed for the method that does
*everything*.
I quite agree. Perhaps this can be done in conjunction with JDK-8223322
<https://bugs.openjdk.java.net/browse/JDK-8223322> which will revise the
flow of Main and ToolProvider.
(adding comment in JDK-8223322)
<https://bugs.openjdk.java.net/browse/JDK-8223322>
BundleParams:
- 116: Why type-variable "C" instead of the conventional T?
I don't know the history of this, but it seems reasonable to change to
"T" like other classes.
(added as a line item 19 in JDK-8223241
<https://bugs.openjdk.java.net/browse/JDK-8223241>)
StandardBundlerParam:
- Many unused imports (according to IntelliJ) including some in the
same jdk.jpackage.internal that are not needed.
already listed as item 1 in JDK-8223241
<https://bugs.openjdk.java.net/browse/JDK-8223241>
138: TODO and typo in comment.
remove todo - this has been tested
(added line item 20 in JDK-8223241
<https://bugs.openjdk.java.net/browse/JDK-8223241>)
148: when does the pathSep get replaced with spaces? Will that cause
an issue in re-parsing the string?
661-662: escaped is never set to true, so escaping is not supported?
need to look into this - (added to JDK-8223334)
747: System.getProperty can require permissions to get properties if
run under a security manager.
Many of the operations performed by jpackage too would require
all-permissions if run under security manager.
CLIHelp:
- 58, 65, 72, 80: Indentation of pLaunchOptions does not line up.
I don't see what you mean here. Looks lined up to me
IOUtils:
- 262: why the mix of ProcessBuilder and Runtime.exec - stick to
ProcessBuilder
added to this case to JDK-8223334
Log:
"JPACKAGE_DEBUG" environment variable - ? uppercase, documented?
implemented as strictly upper case, what do we have to do to document ?
ValidOptions: 46: typo in comment: "in the a"
several problems with this comment - added to JDK-8223241
<https://bugs.openjdk.java.net/browse/JDK-8223241>
VersionExtractor.java seems to be dead code.
Dead code in RelativeFileset: upshift(), contains(), copy constructor().
Platform.java:
- should use System Runtime.Version class.
Params.java: Dead code does not seem to be used in DeployParams
(and would probably be better as a map than an individual object).
setParams is unused.
new JBS issue to remove dead code. (JDK-8223586
<https://bugs.openjdk.java.net/browse/JDK-8223586>)
DeployParams:
- If there are accessor methods, use them!
- Lots of unused methods. (According to IntelliJ)
- setSystemWide should use primitive boolean, not Boolean;
conversions will be done as needed.
- Ditto installDirChooser
- Ditto SignBundle flag
Many .java files that pass a map of parameters use the argument name
"p" but using "params"
would communicate better and be more consistent across the different
files.
LinuxDebBundler and LinuxRpmBundler should share more code.
MacAppBundler.java:
- list of categories will need maintenance to stay in sync with
Apple. Can the list be derived from the installed Mac tool chain?
- or don't validate the argument until it is built and let the mac
app builder do the checking.
All above added to JDK-8223586
<https://bugs.openjdk.java.net/browse/JDK-8223586>
/Andy
Regards, Roger
On 04/29/2019 05:58 PM, Andy Herrick wrote:
jpackage reviewers:
We hope to move JEP 343 to "Proposed to Target" next week, so we
would like expedite the review process as much as possible.
