Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal: JDK-8200758: Packaging Tool]

[Kevin Rushforth]

> For the first point, it means that jpackager should use jopt for the argument parsing (to be fully compatible with the GNU style of options).
> For the second point, it means to change a lot of code that may break because it's less mechanical than introducing try-with-resources.

This seems quite a reasonable suggestion. Thanks.

-- Kevin


On 7/7/2018 7:10 AM, forax at univ-mlv.fr wrote:
>
> ----- Mail original -----
>> De: "Kevin Rushforth" <kevin.rushforth at oracle.com>
>> À: "Remi Forax" <forax at univ-mlv.fr>
>> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "Alexey Semenyuk" <alexey.Semenyuk at oracle.com>, "Andy Herrick"
>> <andy.herrick at oracle.com>
>> Envoyé: Samedi 7 Juillet 2018 15:47:01
>> Objet: Re: Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal: JDK-8200758: Packaging Tool]
>> Hi Remy,
>>
>> Thank you for taking a look.
>>
>> Yes, the javapackager code that forms the basis for the jpackager
>> prototype was initially developed on older JDKs and evolved from there.
>> I'm sure the improvements you suggest are all good ones, and it doesn't
>> seem like it would be too hard to address the most important of them,
>> especially the try-with-resources or anything else that would affect the
>> robustness of the tool. As long as we do address the robustness issues,
>> I think it is more important to get the feature set right, and make sure
>> that the public interfaces -- the command line options and ToolProvider
>> interface -- are clean. I don't see the need to rewrite the tool or take
>> an extra couple of months to modernize all of the implementation to use
>> JDK 11 APIs everywhere.
>>
>> Also, I don't agree that jpackager is too large for jdk/sandbox or that
>> it needs it own project. The jdk/sandbox is perfect for new modules /
>> new tools that don't impact other parts of the JDK.
>>
>> -- Kevin
> Hi Kevin,
> like you, i don't think that a full rewrite is necessary, as you said having the right public 'interfaces' is enough,
> but reducing the size the duplicated code (with the JDK and internally) is as important in my opinion.
>
> For the first point, it means that jpackager should use jopt for the argument parsing (to be fully compatible with the GNU style of options).
> For the second point, it means to change a lot of code that may break because it's less mechanical than introducing try-with-resources.
>
> regards,
> Rémi
>
>>
>> On 7/6/2018 3:07 PM, Remi Forax wrote:
>>> I've just taking a look at the patch,
>>> i don't see how this can be integrated soon, the code is consistently
>>> inconsistent as one of my colleague would say, even the coding conventions are
>>> not respected.
>>> i believe that's it's because the code have been written first in Java 6 an
>>> without refactoring was moved to use Java 7, 8, 9, 10 and 11.
>>>
>>> The I/O code still using java.io.File for some parts, no try-with-resources so
>>> most of the try/finally are not safe,
>>> a lot of code like new BufferedWriter(new FileWriter(file)) instead of
>>> Files.newBufferedWriter, etc. The code should use the package java.nio.file,
>>> and not the old java.io,
>>> most of the code try to manage the exception right were they appear instead of
>>> propagating them so there are too many try/catch,
>>> a lot of catch are ignored which is a code smell, some codes use the internal
>>> logger (jdk.packager.internal.Log), but a lot of codes doesn't,
>>> for the collection code, there is a lot of copy of data small structures (which
>>> suggest that published collections are not immutable),
>>> there are dubious @SuppressWarnings("unchecked"), some or them are due to the
>>> fact that the code use Class as a type token instead of using lambdas,
>>> Stream are not used when they should (to avoid multiple copy between data
>>> structures) and streams that need to be closed are not (the result of
>>> Files.list by example),
>>> there are usual "don't do that in Java" like a loop using an integer index to
>>> traverse a List without knowing if it's a random access list or not,
>>> there is a lot of nullchecks instead of using Optional,
>>> a lot of code initialize local variables to null which is a code smell (and a
>>> side effect of having a lot of try/catch but not only),
>>> constructors should not do work, just initialization, use static factory method
>>> instead (so you will not have to debug half constructed objects),
>>> the code uses BigIntegers to parse a bundle version, just in case,
>>> the code uses an AtomicReference as a box that a lambda can mutate, instead of
>>> wrapping the exception into a runtime and unwrapping it at call site,
>>> The code of jdk.packager.internal.IOUtils should be updated to use methods of
>>> the JDK 11 and methods like readFully should be replaced by the JDK's one.
>>> listOfPathToString and setOfStringToString are just WTF, like in
>>> getRedistributableModules(), where the variable stream is an Optional,
>>> A class like Platform should be used everywhere to do platform specific stuff, a
>>> lot of code still use String matching (the version parsing should use
>>> System.Version).
>>> All the argument parsing should be delegated to JOpt (the one integrated with
>>> the JDK), so it will be consistent with the other JDK tools,
>>> There is a UnsupportedPlatformException but a Platform can be UNKOWN ??
>>>
>>> I will stop here, my point is that there is a lot of cleaning that should appear
>>> before the code is integrated into the JDK and given the size of the code, i
>>> wonder if it's not better to start an OpenJDK project for it and iterate on the
>>> code before trying to include it in the JDK. For me, the code is too big to use
>>> the jdk/sandbox.
>>>
>>> regards,
>>> Rémi
>>>
>>> ----- Mail original -----
>>>> De: "Kevin Rushforth" <kevin.rushforth at oracle.com>
>>>> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>>>> Cc: "Alexey Semenyuk" <alexey.Semenyuk at oracle.com>, "Andy Herrick"
>>>> <andy.herrick at oracle.com>
>>>> Envoyé: Vendredi 6 Juillet 2018 22:14:29
>>>> Objet: Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal:
>>>> JDK-8200758: Packaging Tool]
>>>> An initial prototype of the jpackager tool has been pushed to a new
>>>> 'JDK-8200758-branch' branch in the JDK sandbox [1]. If anyone is
>>>> interested in taking a look, you can clone it as follows:
>>>>
>>>>       hg clone http://hg.openjdk.java.net/jdk/sandbox
>>>>       cd ./sandbox
>>>>       hg update JDK-8200758-branch
>>>>
>>>> I plan to reply to the feedback already provided, and update the JEP [2]
>>>> next week some time, but in the mean time if you have additional
>>>> questions or comments, feel free to reply.
>>>>
>>>> -- Kevin
>>>>
>>>> [1] http://hg.openjdk.java.net/jdk/sandbox/shortlog/JDK-8200758-branch
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8200758
>>>>
>>>>
>>>> On 6/27/2018 3:30 PM, Kevin Rushforth wrote:
>>>>> We're aiming to get this into JDK 12 early enough so that an EA build
>>>>> would be available around the time JDK 11 ships. That will allow you
>>>>> to take a jlinked image with JDK 11 and package it up using (the EA)
>>>>> jpackager.
>>>>>
>>>>> We will create a development branch in the JDK sandbox [1] some time
>>>>> in the next week or so so you can follow the development.
>>>>>
>>>>> Also, thank you to those who have provided feedback. I'll reply to
>>>>> feedback soon and then incorporate it into an updated JEP.
>>>>>
>>>>> -- Kevin