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*.
BundleParams:
- 116: Why type-variable "C" instead of the conventional T?
StandardBundlerParam:
- Many unused imports (according to IntelliJ) including some in the
same jdk.jpackage.internal that are not needed.
138: TODO and typo in comment.
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?
747: System.getProperty can require permissions to get properties if run
under a security manager.
CLIHelp:
- 58, 65, 72, 80: Indentation of pLaunchOptions does not line up.
IOUtils:
- 262: why the mix of ProcessBuilder and Runtime.exec - stick to
ProcessBuilder
Log:
"JPACKAGE_DEBUG" environment variable - ? uppercase, documented?
ValidOptions: 46: typo in comment: "in the a"
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.
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.
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.
