RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Philip Race]

BTW there were also a few minor grammar issues in javadoc
eg

   41  * the option named "-singleton" must be specified on jpackager command line.

"the jpackager"


   84      * Registers {@code SingleInstanceListener} for current process.
and
   96      * Registers {@code SingleInstanceListener} for current process.

and
  122      * Unregisters {@code SingleInstanceListener} for current process.
  123      * If the {@code SingleInstanceListener} object is not registered, or
  124      * {@code slistener} is {@code null}, then the unregistration is skipped.


"the current process"

There may be more like that.

and as I mentioned offline, I think the correct word is "deregister" not 
"unregister"
both in comments and method names.

-phill

On 11/12/18, 2:38 PM, Andy Herrick wrote:
>
>
> On 11/12/2018 4:54 PM, Philip Race wrote:
>>
>> On 11/12/18, 1:45 PM, Andy Herrick wrote:
>>>
>>> On 11/12/2018 3:22 PM, Philip Race wrote:
>>>> Adding build-dev back ..
>>>>
>>>> I noticed that module jdk.jpackager.runtime requires java.desktop 
>>>> on all platforms
>>>>
>>>> So far as I can tell this is for a Mac-only support for receiving and
>>>> handling file open events. Probably it only makes sense or gets used
>>>> when the API is used from a running desktop application.
>>>>
>>>> I might ask if we need this at all, but I definitely think it should
>>>> not be required on all platforms if needed only for Mac even if
>>>> we might want it on windows in a future version.
>>>>
>>>> Do we not envisage cases where you need the runtime API for
>>>> some kind of daemon service for which there should be a singleton ?
>>>> Do you really want to force the desktop module to be dragged along
>>>> in such a case ?
>>>>
>>>> I think we should remove this dependency even if it means losing a
>>>> MacOS feature at least for now.
>>>
>>> 1.) we could remove that functionality (I don't even really 
>>> understand the use case for it), and remove the two arg 
>>> registerSingleInstance method from the public interface, and remove 
>>> the requires statement in module-info.java.
>>>
>> I was thinking remove it as above since since even the javadoc for 
>> that method
>> feels obliged to say it is just for macos :-
>>
>>   95     /**
>>   96      * Registers {@code SingleInstanceListener} for current 
>> process.
>>   97      * If the {@code SingleInstanceListener} object is already 
>> registered, or
>>   98      * {@code slistener} is {@code null}, then the registration 
>> is skipped.
>>   99      *
>>  100      * @param slistener the listener to handle the single 
>> instance behaviour.
>>  101      * @param setFileHandler if {@code true}, the listener is 
>> notified when the
>>  102      *         application is asked to open a list of files. If 
>> OS is not MacOS,
>>  103      *         the parameter is ignored.
>>  104      */
>>
>>
>> yuck. If such optional functionality is needed it needs to be done via
>> adding a "doesPlatformSupportFileHandler() method rather than saying 
>> the above.
>>
>> -phil.
>
> OK - I filed JDK-8213756 
> <https://bugs.openjdk.java.net/browse/JDK-8213756> to handle removing 
> this API, fixing the call to new SingleInstanceImpl() to be wrapped in 
> synchronized null check, and fixing comments you referred to .
>
> /Andy
>
>>
>>> or ...
>>>
>>> 2.) we could move that functionality to a platform dependent class, 
>>> having dummy methods of setOpenFileHandler in the windows and linux  
>>> platform dependent class.  Then we could add a 
>>> module-info.java.extra in src/jdk.jpackager.runtime/macos/classes 
>>> with that requires statement.
>>>
>>> /Andy
>>>
>>>>
>>>> -phil.
>>>>
>>>> On 11/11/18, 2:40 PM, Andy Herrick wrote:
>>>>> On 11/9/2018 5:25 PM, Andy Herrick wrote:
>>>>>> This is an update to the Request For Review of the implementation 
>>>>>> of the Java Packager Tool (jpackager) as described in JEP 343: 
>>>>>> Packaging Tool <https://bugs.openjdk.java.net/browse/JDK-8200758>
>>>>>>
>>>>>> This refresh renames the packages used to jdk.jpackager and 
>>>>>> jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
>>>>>> initial set of automated tests, and contains fixes to the 
>>>>>> following issues:
>>>>>>
>>>>>> JDK-8213324 jpackager deletes existing app directory without warning
>>>>>> JDK-8213166 jpackager --argument arg is broken
>>>>>> JDK-8213163 --app-image arg does not work creating exe installers
>>>>>> JDK-8212089 Prepare packager for localization
>>>>>> JDK-8212537 Create method and class description comments for main 
>>>>>> functionality
>>>>>> JDK-8213332 Create minimal automated tests for jpackager
>>>>>> JDK-8213333 Fix issues found in jpackager with automated tests
>>>>>> JDK-8213394 Stop using Log.info() except for expected output.
>>>>>> JDK-8213345 Secondary Launchers broken on mac.
>>>>>> JDK-8213156 rename packages for jpackager
>>>>>> JDK-8213244 Fix all warnings in jpackager java code
>>>>>> JDK-8212143 Remove native code that supports UserJvmOptionsService
>>>>>> JDK-8213162 Association description in Inno Setup cannot contain 
>>>>>> double quotes
>>>>>>
>>>>>> The following additional issues are targeted to be address in the 
>>>>>> next few weeks:
>>>>>> JDK-8212936     Makefile and other improvements for jpackager
>>>>>> JDK-8212164     resolve jre.list and jre.module.list
>>>>>> JDK-8213392     Enhance --help and --version
>>>>>> JDK-8208652     File name is not passed to main() via file 
>>>>>> association on OS X
>>>>>> JDK-8212538     Determine standard way to determine if a Modular jar
>>>>>> JDK-8213558     Create more unit tests
>>>>> Note: The above issues are targeted to 'internal' - meaning we 
>>>>> expect to resolve them in the sandbox before the initial push to 
>>>>> JDK12.
>>>>> Issues targeted to '12' are expected to be fixed after the initial 
>>>>> push.
>>>>>
>>>>> /Andy
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/
>>>>>>
>>>>>> please send feedback to core-libs-dev at openjdk.java.net
>>>>>>
>>>>>> /Andy Herrick
>>>>>>
>>>>>