RFR: JDK-8223325: Improve wix sources generated by jpackage

[Alexey Semenyuk]

Little bit late, Andy has pushed the patch already. Please find my 
comment below.

On 10/18/2019 6:15 PM, Alexander Matveev wrote:
> Hi Alexey,
>
> http://cr.openjdk.java.net/~asemenyuk/8223325/webrev.00/src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageFile.java.frames.html 
>
> Line 180: man -> main
>
> http://cr.openjdk.java.net/~asemenyuk/8223325/webrev.00/src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java.frames.html 
>
> createXml() should we force it to UTF-8? Like we did in MacPkgBundler.
XMLStreamWriter.writeStartDocument() without arguments should create 
`<?xml version=“1.0” encoding=“utf-8”?>` xml declaration.
I'm not sure though that UTF-8 encoding would be used for underlying 
java.io.Writer. I agree this should be reworked to make sure we save xml 
in file with UTF-8 encoding.

> http://cr.openjdk.java.net/~asemenyuk/8223325/webrev.00/src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WixTool.java.html 
>
> findWixInstallDirs() I think we used to check PATH variable as well 
> for Wix. 
The same approach is used in WixTool. But instead of parsing PATH 
variable jpackage tries to launch light.exe or candle.exe without 
specifying full paths to them first, i.e. it checks if they are in PATH 
without parsing value of PATH variable. In case of failure it checks for 
well known install dirs. Please look at WixTool.find() method for the 
logic of locating WiX tools.

> In any case this needs to be fixed. We cannot assume "C:\Program Files". 
Agree.

> This folder can be on different drive, so it is better to use Windows 
> API to get path to "Program Files". 
There is environment variable for program files - PROGRAMFILES. However 
it is not always set (e.g. jtreg resets it). Another approach would be 
to query registry value for the path to program files folder. So there 
are different options.
I just decided not to touch "C:\Program Files" as the patch is already 
quite messy.

I created follow up for your findings - 
https://bugs.openjdk.java.net/browse/JDK-8232646.

- Alexey

> Also, user might install Wix into different folder. I think it is 
> better to file a bug and improve it eventually.
>
> Looks good overall.
>
> Thanks,
> Alexander
>
> On 10/17/19 3:24 PM, Alexey Semenyuk 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).
>>
>> - moved code creating WiX sources for app image from WinMsiBundler in 
>> a separate class - WixSourcesBuilder;
>> - put at most one file in component in WiX sources;
>> - use StAX to create WiX sources;
>> - added IOUtils.createXml function to simplify xml creation;
>> - added basic javadoc to AppImageFile class;
>> - added support for use of icons in file associations test for better 
>> coverage;
>> - fixed the bug with not excluded `.jpackage.xml` from installed app 
>> image on Windows;
>> - improved test output isolation when jpackage tests are executed not 
>> with jtreg framework;
>> - bugfix of MainClassTest test class;
>>
>> - Alexey
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8223325
>>
>> [2] http://cr.openjdk.java.net/~asemenyuk/8223325/webrev.00/
>>
>