[modules-dev] Review request for 6605077, "jrepo dependencies"
Kumar Srinivasan
Kumar.Srinivasan at Sun.COM
Mon Jul 14 14:23:16 PDT 2008
Dave Bristor wrote:
> Hi Mandy, thanks for the feedback. I incorporated the suggestions as
> appropriate, and have fired off a JPRT run. When that has passed I'll
> generate a new webrev.
>
> Kumar, a question in-line for you.
>
> Thanks,
> Dave
>
> Mandy Chung wrote:
>
>> 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?
>>
>
> That would not be right: ^ is XOR, not OR, and exclusive-or is intended, so
> it's correct as-is.
>
>
>> sun/module/tools/ImportTraverser.java
>> line 120: prefer to see the OR sign instead of a bitwise XOR in an
>> if-statement.
>>
>
> Same as with URLMOduleInfo.java
>
>
>> 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.
>>
>
> I prefer allowing null; this allows less coupling from the caller to modules
> code. I.e., with your suggestion, the caller must directly reference the
> VersionConstraint class.
>
>
>> 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.
>>
>
> The "repo" parameter is used in line 156.
>
>
>> 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.
>>
>
> I *think* Kumar asked for it, as he expects to also write code which will
> invoke this API.
>
> Kumar, please let me know if it's not necessary and I'll remove it.
>
Don't know yet, as this *may* be required for the static analysis tool
we discussed.
We can comment it out for now. And uncomment it when we really need it.
Kumar
>
>> sun/module/tools/JRepo.java
>> line 292: this line has extra whitespaces in front of "rc = ..."
>>
>
> Fixed.
>
>
>> line 326: I think instead of checking "srcLoc == null" to determine a
>> bootstrap repository, you should check md.getRepository() ==
>> Repository.getBootstrapRepository().
>>
>
> Fixed.
>
>
>> 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?
>>
>
> Fixed.
>
>
>> 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())
>>
>
> We already have
> boolean visited(Module m)
> What's the use case for knowing the count of visits?
>
>
>> line 694 & 710: should we check the name starting with "java.se"
>> instead of equals? We also have a "java.se.core" module, right?
>>
>
> Yes, but this is intended to stop the traversal precisely at the java.se
> module, so that nothing under it, regardless of name, is traversed. If there
> were some other module whose name started with "java.se" that is not under
> "java.se", we would want to traverse it. Maybe that's an illegal module name?
> I'm not sure. But currently it's intentional as-is.
>
>
>> line 697 & 713: I suggest to define a string constant variable for
>> " " and then line 713 will use the length instead of hardcoding 4.
>>
>
> Fixed.
>
>
>> line 673: should be "module archives in the repository" instead of
>> "modules in the repository"
>>
>
> Fixed.
>
>
>> 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
>>>
>>>
>>>
> _______________________________________________
> modules-dev mailing list
> modules-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/modules-dev
>
--
Kumar Srinivasan
Sun Microsystems, Java Software.
408-276-7586
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/modules-dev/attachments/20080714/f62b440f/attachment.html
More information about the modules-dev
mailing list