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

[Jim Laskey (Oracle)]

+1


> On Dec 7, 2017, at 11:27 PM, Sundararajan Athijegannathan 
>  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
>>> 

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

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

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

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

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