<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Dave Bristor wrote:
<blockquote cite="mid:487BC045.4040107@sun.com" type="cite">
<pre wrap="">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:
</pre>
<blockquote type="cite">
<pre wrap="">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?
</pre>
</blockquote>
<pre wrap=""><!---->
That would not be right: ^ is XOR, not OR, and exclusive-or is intended, so
it's correct as-is.
</pre>
<blockquote type="cite">
<pre wrap="">sun/module/tools/ImportTraverser.java
line 120: prefer to see the OR sign instead of a bitwise XOR in an
if-statement.
</pre>
</blockquote>
<pre wrap=""><!---->
Same as with URLMOduleInfo.java
</pre>
<blockquote type="cite">
<pre wrap=""> 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.
</pre>
</blockquote>
<pre wrap=""><!---->
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.
</pre>
<blockquote type="cite">
<pre wrap=""> 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.
</pre>
</blockquote>
<pre wrap=""><!---->
The "repo" parameter is used in line 156.
</pre>
<blockquote type="cite">
<pre wrap=""> 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.
</pre>
</blockquote>
<pre wrap=""><!---->
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.
</pre>
</blockquote>
Don't know yet, as this *may* be required for the static analysis tool
we discussed.<br>
We can comment it out for now. And uncomment it when we really need it.<br>
<br>
Kumar<br>
<br>
<blockquote cite="mid:487BC045.4040107@sun.com" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">sun/module/tools/JRepo.java
line 292: this line has extra whitespaces in front of "rc = ..."
</pre>
</blockquote>
<pre wrap=""><!---->
Fixed.
</pre>
<blockquote type="cite">
<pre wrap=""> line 326: I think instead of checking "srcLoc == null" to determine a
bootstrap repository, you should check md.getRepository() ==
Repository.getBootstrapRepository().
</pre>
</blockquote>
<pre wrap=""><!---->
Fixed.
</pre>
<blockquote type="cite">
<pre wrap=""> 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?
</pre>
</blockquote>
<pre wrap=""><!---->
Fixed.
</pre>
<blockquote type="cite">
<pre wrap=""> 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())
</pre>
</blockquote>
<pre wrap=""><!---->
We already have
boolean visited(Module m)
What's the use case for knowing the count of visits?
</pre>
<blockquote type="cite">
<pre wrap=""> line 694 & 710: should we check the name starting with "java.se"
instead of equals? We also have a "java.se.core" module, right?
</pre>
</blockquote>
<pre wrap=""><!---->
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.
</pre>
<blockquote type="cite">
<pre wrap=""> line 697 & 713: I suggest to define a string constant variable for
" " and then line 713 will use the length instead of hardcoding 4.
</pre>
</blockquote>
<pre wrap=""><!---->
Fixed.
</pre>
<blockquote type="cite">
<pre wrap=""> line 673: should be "module archives in the repository" instead of
"modules in the repository"
</pre>
</blockquote>
<pre wrap=""><!---->
Fixed.
</pre>
<blockquote type="cite">
<pre wrap="">Mandy
Dave Bristor wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi folks,
This work adds a "dependencies" command to jrepo, which will show, via
indentation, all the dependencies of a module.
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/view_bug.do?bug_id=6605077">http://bugs.sun.com/view_bug.do?bug_id=6605077</a>
<a class="moz-txt-link-freetext" href="http://webrev.invokedynamic.info/bristor/6605077-01/">http://webrev.invokedynamic.info/bristor/6605077-01/</a>
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
<a class="moz-txt-link-freetext" href="file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/">file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/</a>
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
<a class="moz-txt-link-freetext" href="file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/">file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/</a>
java.se-1.7 bootstrap
m4-1.6
<a class="moz-txt-link-freetext" href="file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/">file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/</a>
java.se-1.7 bootstrap
m3-2.0
<a class="moz-txt-link-freetext" href="file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/">file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/</a>
java.se-1.7 bootstrap
m4-1.6
<a class="moz-txt-link-freetext" href="file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/">file:/java/tl/libs/bristor/ws/jrepo/jdk/test/java/module/modinit/tmp_mtest/version/version3.mtest/</a>
(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
<a class="moz-txt-link-abbreviated" href="mailto:modules-dev@openjdk.java.net">modules-dev@openjdk.java.net</a>
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/mailman/listinfo/modules-dev">http://mail.openjdk.java.net/mailman/listinfo/modules-dev</a>
</pre>
</blockquote>
</blockquote>
<pre wrap=""><!---->_______________________________________________
modules-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:modules-dev@openjdk.java.net">modules-dev@openjdk.java.net</a>
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/mailman/listinfo/modules-dev">http://mail.openjdk.java.net/mailman/listinfo/modules-dev</a>
</pre>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Kumar Srinivasan
Sun Microsystems, Java Software.
408-276-7586
</pre>
</body>
</html>