Review Request JDK-8160286: jmod hash is creating unlinkable modules.

[Andrej Golovnin]

Hi Mandy,

src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java

806         final Optional<String> moduleName;  // a specific module
to record hashes

I thought that using Optional in a class field is considered a misuse
of the API. The field hashesBuilder can also be null. But you don't
wrap it into an Optional. So why it is needed to wrap moduleName into
Optional?

Best regards,
Andrej Golovnin

On Fri, Jan 13, 2017 at 6:13 AM, Mandy Chung <mandy.chung at oracle.com> wrote:
> Updated webrev:
>    http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.01/
>
> I did a further clean up and ModuleHashesBuilder now has one constructor.
> Also updated the comment to avoid using the term “base”.  Due to the
> refactoring, I moved the location of the tmp jmod file to be in the
> tmp directory rather than the same directory of the jmod file;
> otherwise, it would confuse the ModuleFinder with the tmp file.
>
> I added a few more test cases that remind me that a packaged user
> module can’t tie with a specific JDK build since a module requiring
> a JDK module that would be a leaf node of the subgraph rather than
> that module.  The `jar` tool locating JMOD files on the module path
> allows to tie a modular JAR with a module requiring it but packaged
> as JMOD file (e.g. with native library).
>
> Mandy
>
>> On Jan 12, 2017, at 7:52 AM, Mandy Chung <mandy.chung at oracle.com> wrote:
>>
>>
>>> On Jan 12, 2017, at 7:08 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>>
>>> On 11/01/2017 23:47, Mandy Chung wrote:
>>>
>>>> Webrev:
>>>>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8160286/webrev.00/
>>>>
>>>> jmod and jar -—hash-modules option to specify a pattern of modules
>>>> to be hashed in the module M being created.  It records the modules
>>>> that depend on M directly and indirectly.
>>>>
>>> This looks quite good.  At some point then we'll need to move the tool support out of jdk.internal.module but is something for another day.
>>>
>>> For ModuleHashesBuilder then it might be useful to put a comment on the constructors as it's not immediately obvious why both are needed. Also I wonder if we should use a term other than "base" for the modules that don't have references to other modules in the sub-graph (they are sort of leaf modules in the sub-graph).  A typo at L96 "in topological orders" -> "order”.
>>
>> I’ll take a pass and update the comments.  I can see “base” can be confusing.
>>
>>> One of the changes in this patch is that the `jar` tool will locate JMOD files on the module path. I assume this is to provide flexibility to those creating a modular JAR that want to tie it to a specific JDK build. I guess it's okay but I suspect will not be widely used.
>>
>> Right and another case is packaged module with a native library (security providers). It will not be widey used.
>>
>> Mandy
>