RFR 8192986: Inconsistent handling of exploded modules in jlink

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8192986: Inconsistent handling of exploded modules in jlink

Sundararajan Athijegannathan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192986: Inconsistent handling of exploded modules in jlink

Sundararajan Athijegannathan
Updated: http://cr.openjdk.java.net/~sundar/8192986/webrev.01/

PS. Bug description line was wrong in the test .java file.

-Sundar

On 07/12/17, 8:40 PM, Sundararajan Athijegannathan wrote:
> Please review.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8192986
> Webrev: http://cr.openjdk.java.net/~sundar/8192986/webrev.00/
>
> Thanks,
> -Sundar
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192986: Inconsistent handling of exploded modules in jlink

Claes Redestad
Hi Sundar,

thanks for picking this up so quick!

On 2017-12-07 16:21, Sundararajan Athijegannathan wrote:
> Updated: http://cr.openjdk.java.net/~sundar/8192986/webrev.01/

Looks ok, butunless my understanding is flawed it seems the logic is now
getting more strict about a directory on the module path containing a
well-formed module. Should this be made more graceful, say ignore empty
directories? Maybe just warn about malformed and/or missing modules?

Nits:

JlinkTask: resolves module-info.class twice (resolve once and pass as
parameter?)

ExplodedModuleNameTest:

   58         if (helper == null) {
   59             System.err.println("Test not run");
   60             return;
   61         }

Should this fail the test (by throwing an exception)?

   66         // rename the module containing directory
   67         Path renamedModDir = modDir.resolveSibling("modified_mod8192986");
   68         // copy the content from original directory to modified name directory
   69         copyDir(modDir, renamedModDir);

Any reason not to use Files.move(modDir, renamedModDir|,StandardCopyOption.REPLACE_EXISTING|) instead of copying here?

Thanks!

/Claes

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192986: Inconsistent handling of exploded modules in jlink

Sundararajan Athijegannathan
Hi,

Comments below...

On 07/12/17, 8:54 PM, Claes Redestad wrote:

> Hi Sundar,
>
> thanks for picking this up so quick!
>
> On 2017-12-07 16:21, Sundararajan Athijegannathan wrote:
>> Updated: http://cr.openjdk.java.net/~sundar/8192986/webrev.01/
>
> Looks ok, butunless my understanding is flawed it seems the logic is
> now getting more strict about a directory on the module path
> containing a well-formed module. Should this be made more graceful,
> say ignore empty directories? Maybe just warn about malformed and/or
> missing modules?
I'd prefer stricter checks. But I'd like to hear from others as well...
> Nits:
>
> JlinkTask: resolves module-info.class twice (resolve once and pass as
> parameter?)

Yes, I'll fix that.

>
> ExplodedModuleNameTest:
>
>   58         if (helper == null) {
>   59             System.err.println("Test not run");
>   60             return;
>   61         }
>
> Should this fail the test (by throwing an exception)?

This is similar to other tests. For eg. ModuleNamesOrderTest

>   66         // rename the module containing directory
>   67         Path renamedModDir =
> modDir.resolveSibling("modified_mod8192986");
>   68         // copy the content from original directory to modified
> name directory
>   69         copyDir(modDir, renamedModDir);
>
> Any reason not to use Files.move(modDir,
> renamedModDir|,StandardCopyOption.REPLACE_EXISTING|) instead of
> copying here?
>

https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#move(java.nio.file.Path,%20java.nio.file.Path,%20java.nio.file.CopyOption...)

"To move a file tree may involve copying rather than moving directories
and this can be done using the copy method in conjunction with the
Files.walkFileTree utility method."

Thanks,
-Sundar

> Thanks!
>
> /Claes
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192986: Inconsistent handling of exploded modules in jlink

Sundararajan Athijegannathan
Hi,

* On stricter check for missing module-info.class in a directory: There
is a similar check for non-modular jars.

jlink --add-modules java.base --module-path t.jar  --output foo
Error: Unable to derive module descriptor for t.jar

t.jar in the above command is a non-modular jar. I think it is better to
detect and report non-modular exploded dirs as well.

* Fixed to avoid second resolve for "module-info.class" in JlinkTask.java

Please review updated webrev:
http://cr.openjdk.java.net/~sundar/8192986/webrev.02/

Thanks
-Sundar

On 07/12/17, 9:15 PM, Sundararajan Athijegannathan wrote:

> Hi,
>
> Comments below...
>
> On 07/12/17, 8:54 PM, Claes Redestad wrote:
>> Hi Sundar,
>>
>> thanks for picking this up so quick!
>>
>> On 2017-12-07 16:21, Sundararajan Athijegannathan wrote:
>>> Updated: http://cr.openjdk.java.net/~sundar/8192986/webrev.01/
>>
>> Looks ok, butunless my understanding is flawed it seems the logic is
>> now getting more strict about a directory on the module path
>> containing a well-formed module. Should this be made more graceful,
>> say ignore empty directories? Maybe just warn about malformed and/or
>> missing modules?
> I'd prefer stricter checks. But I'd like to hear from others as well...
>> Nits:
>>
>> JlinkTask: resolves module-info.class twice (resolve once and pass as
>> parameter?)
>
> Yes, I'll fix that.
>
>>
>> ExplodedModuleNameTest:
>>
>>   58         if (helper == null) {
>>   59             System.err.println("Test not run");
>>   60             return;
>>   61         }
>>
>> Should this fail the test (by throwing an exception)?
>
> This is similar to other tests. For eg. ModuleNamesOrderTest
>
>>   66         // rename the module containing directory
>>   67         Path renamedModDir =
>> modDir.resolveSibling("modified_mod8192986");
>>   68         // copy the content from original directory to modified
>> name directory
>>   69         copyDir(modDir, renamedModDir);
>>
>> Any reason not to use Files.move(modDir,
>> renamedModDir|,StandardCopyOption.REPLACE_EXISTING|) instead of
>> copying here?
>>
>
> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#move(java.nio.file.Path,%20java.nio.file.Path,%20java.nio.file.CopyOption...)
>
>
> "To move a file tree may involve copying rather than moving
> directories and this can be done using the copy method in conjunction
> with the Files.walkFileTree utility method."
>
> Thanks,
> -Sundar
>
>> Thanks!
>>
>> /Claes
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192986: Inconsistent handling of exploded modules in jlink

Claes Redestad


On 2017-12-08 04:27, Sundararajan Athijegannathan wrote:
>
> * Fixed to avoid second resolve for "module-info.class" in JlinkTask.java
>
> Please review updated webrev:
> http://cr.openjdk.java.net/~sundar/8192986/webrev.02/

+1

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192986: Inconsistent handling of exploded modules in jlink

Jim Laskey (Oracle)
In reply to this post by Sundararajan Athijegannathan
+1


> On Dec 7, 2017, at 11:27 PM, Sundararajan Athijegannathan <[hidden email]> wrote:
>
> Hi,
>
> * On stricter check for missing module-info.class in a directory: There is a similar check for non-modular jars.
>
> jlink --add-modules java.base --module-path t.jar  --output foo
> Error: Unable to derive module descriptor for t.jar
>
> t.jar in the above command is a non-modular jar. I think it is better to detect and report non-modular exploded dirs as well.
>
> * Fixed to avoid second resolve for "module-info.class" in JlinkTask.java
>
> Please review updated webrev: http://cr.openjdk.java.net/~sundar/8192986/webrev.02/
>
> Thanks
> -Sundar
>
> On 07/12/17, 9:15 PM, Sundararajan Athijegannathan wrote:
>> Hi,
>>
>> Comments below...
>>
>> On 07/12/17, 8:54 PM, Claes Redestad wrote:
>>> Hi Sundar,
>>>
>>> thanks for picking this up so quick!
>>>
>>> On 2017-12-07 16:21, Sundararajan Athijegannathan wrote:
>>>> Updated: http://cr.openjdk.java.net/~sundar/8192986/webrev.01/
>>>
>>> Looks ok, butunless my understanding is flawed it seems the logic is now getting more strict about a directory on the module path containing a well-formed module. Should this be made more graceful, say ignore empty directories? Maybe just warn about malformed and/or missing modules?
>> I'd prefer stricter checks. But I'd like to hear from others as well...
>>> Nits:
>>>
>>> JlinkTask: resolves module-info.class twice (resolve once and pass as parameter?)
>>
>> Yes, I'll fix that.
>>
>>>
>>> ExplodedModuleNameTest:
>>>
>>>  58         if (helper == null) {
>>>  59             System.err.println("Test not run");
>>>  60             return;
>>>  61         }
>>>
>>> Should this fail the test (by throwing an exception)?
>>
>> This is similar to other tests. For eg. ModuleNamesOrderTest
>>
>>>  66         // rename the module containing directory
>>>  67         Path renamedModDir = modDir.resolveSibling("modified_mod8192986");
>>>  68         // copy the content from original directory to modified name directory
>>>  69         copyDir(modDir, renamedModDir);
>>>
>>> Any reason not to use Files.move(modDir, renamedModDir|,StandardCopyOption.REPLACE_EXISTING|) instead of copying here?
>>>
>>
>> https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#move(java.nio.file.Path,%20java.nio.file.Path,%20java.nio.file.CopyOption...)
>>
>> "To move a file tree may involve copying rather than moving directories and this can be done using the copy method in conjunction with the Files.walkFileTree utility method."
>>
>> Thanks,
>> -Sundar
>>
>>> Thanks!
>>>
>>> /Claes
>>>