[modules-dev] Review request for 6605077, "jrepo dependencies"

[Mandy Chung]

Hi Dave,

Looks good in general.  Here are my review comments:

sun/module/repository/URLModuleInfo.java
     line 61:  interesting to see "^" used here
     - this is not your change but do you mind fixing this to use "||" 
(OR sign) as part of your fix?

sun/module/tools/ImportTraverser.java
     line 120: prefer to see the OR sign instead of a bitwise XOR in an 
if-statement.
     line 102 & 126: would it be better to have this constructor 
requiring a non-null VersionConstraint?  The caller should supply 
VersionConstraint.DEFAULT instead of null.   Also, JRepo.java is passing 
the default version constraint instead of null.
     line 145: repo in the visit(Repository, Module) method is not 
used.  Look like better to rename visit(Repository, Module) method to a 
different method name e.g. traverse(Module m) to begin visiting the 
module and its imports.
     line 71-92: this method installing a module in a temporary 
repository is not used in your fix.   Under what circumstances do we 
need to use this method? I suggest to remove this method.

sun/module/tools/JRepo.java
   line 292: this line has extra whitespaces in front of "rc = ..."
   line 326: I think instead of checking "srcLoc == null" to determine a 
bootstrap repository, you should check md.getRepository() == 
Repository.getBootstrapRepository().
   line 378: IllegalArgumentException is an runtime exception which is 
not needed in the method signature.  Is this change for documentation 
purpose?  Can you add @throws in the comment instead?
   line 635-637: I think it's useful to add a method in the 
ImportTraverser class to determine if any module has been visited (e.g. 
int visitedModuleCount())
   line 694 & 710: should we check the name starting with "java.se" 
instead of equals?  We also have a "java.se.core" module, right?
   line 697 & 713: I suggest to define a string constant variable for 
"    " and then line 713 will use the length instead of hardcoding 4.
   line 673: should be "module archives in the repository" instead of 
"modules in the repository"

Mandy
  
Dave Bristor wrote:

>Hi folks,
>
>This work adds a "dependencies" command to jrepo, which will show, via 
>indentation, all the dependencies of a module.
>
>http://bugs.sun.com/view_bug.do?bug_id=6605077
>http://webrev.invokedynamic.info/bristor/6605077-01/
>
>For example, here's the output from one of the testcases:
>
>m1-1.1
>     java.se-1.7
>     m2-0.0-default
>         java.se-1.7
>         m4-1.6
>             java.se-1.7
>     m3-2.0
>         java.se-1.7
>         m4-1.6
>
>That is, m1-1.1 depends on java.se-1.7, m2-0.0-default, and m3-2.0.  Cycles 
>are handled.  The output does not include the imports of java.se; that is 
>possible if one uses the '-j' flag.  The '-v' flag causes the source location 
>of the containing repository to be printed.  With -j and -v, the above becomes:
>
>ependencies for m1-1.1:
>m1-1.1 
>file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/
>     java.se-1.7 bootstrap
>         java.se.core-1.7 bootstrap
>         corba-3.0 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.xml-1.4 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.xml.bind-2.0 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.xml.ws-2.0 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.xml.soap-1.3 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.annotation-1.0 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.annotation.processing-1.0 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.script-1.0 bootstrap
>             java.se.core-1.7 bootstrap
>         javax.tools-1.0 bootstrap
>             java.se.core-1.7 bootstrap
>     m2-0.0-default 
>file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/
>         java.se-1.7 bootstrap
>         m4-1.6 
>file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/
>             java.se-1.7 bootstrap
>     m3-2.0 
>file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/
>         java.se-1.7 bootstrap
>         m4-1.6 
>file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/
>
>(That output might not paginate well in email; the source locations are on the 
>same lines as the module names.)
>
>The testing approach is to leverage RunMTest.  I've modified it to allow us to 
>specify exactly what it means to "run a test".  By default, it uses the 
>currently-checked-in behavior.  For testing "jrepo dependencies", it uses a 
>different behavior; see JRepoDependenciesTest.MyFactory and 
>JRepoDependenciesTest.MyTestDescription.
>
>The testing is fairly simple, just comparing known-good output with what is 
>generated on a test run.  I hope that one day SQE can do better ;-)
>
>Unrelated, but there's fix to a typo in sun/module/repository/URLModuleInfo.java.
>
>I have not yet integrated the latest API changes.  But neither have I seen a 
>JPRT run on that integration, nor on the ServiceLoader changes I pushed 
>yesterday.  Is our auto-JPRT script running?
>
>Thanks,
>	Dave
>_______________________________________________
>modules-dev mailing list
>modules-dev at openjdk.java.net
>http://mail.openjdk.java.net/mailman/listinfo/modules-dev
>  
>