[modules-dev] Review request for 6605077, "jrepo dependencies"
Dave Bristor
David.Bristor at Sun.COM
Mon Jul 14 14:08:21 PDT 2008
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.
> 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
>>
>>
>
More information about the modules-dev
mailing list