|
Thanks for the helpful review, Bryan.
1 - Yes, I removed this and an unused import from an experiment I had done previously. 2 & 3 - A great suggestion on 3. I pulled out the formatting code and put it into a new method, formatURL. This also normalizes the path separators in the jar/directory locations, so the duplicates are properly removed for 2. 4 - Added additional javadoc comments to mergeZips. Let me know if you think this needs any more refinement, otherwise I'd like to commit it soon. Hi Andrew. Thank you for considering all my feedback. Your new patch looks very good to me. I have only one final concern. When I was spot-testing your patch, I noticed that, while the mergeZips method appears to strip duplicates out of zip1 entirely, and it strips out of zip2 any entries that also exist in zip1, it does not strip duplicates which exist twice in zip2.
For example, try running java -cp derby.jar:db2jcc.jar:db2jcc.jar:derbytools.jar sysinfo. I believe you will see info about db2jcc.jar being printed twice. I think this can be fixed by changing the zip2 processing loop in mergeZips so that, when it searches to see if the current entry duplicates an existing entry, it searches the entries in "v", rather than searching the entries in zip1. I've attached a proposed version 3 of the patch. The only substantive change between this version and your version is as follows: < + for (int k = 0; k < zip1.length; k++) --- > + for (int k = 0; k < v.size(); k++) 265c265,266 < + if (zip1[k] == null) --- > + ZipInfoProperties z = (ZipInfoProperties)v.get(k); > + if (z == null) 267c268 < + if (zip2[j].getLocation().equals(zip1[k].getLocation())) --- > + if (zip2[j].getLocation().equals(z.getLocation())) Good catch, Bryan!
I'm attaching derby-826-v4.diff, which includes your changes plus the following: - removes the check for z == null in the second loop of mergeZips(). We already checked that elements being added to the vector are not null in the first loop. - catches possible NullPointerExceptions if loadZipFromResource() returns a null array. This is unlikely, but possible, if somehow sysinfo is able to run but is isolated from the info properties files so that it cannot load them using getResourceAsStream() or they don't exist. The corresponding diff in getAllInfo() is: + // No info properties files found, but here we are in sysinfo. + // Avoid an NPE in mergeZips by creating a ZipInfoProperties array + // with the location of the sysinfo that is currently executing. + if (zips == null) + { + zips = new ZipInfoProperties[1]; + ZipInfoProperties zip = new ZipInfoProperties(ProductVersionHolder.getProductVersionHolderFromMyEnv(org.apache.derby.tools.sysinfo.TOOLS)); + zip.setLocation(getFileWhichLoadedClass(new Main().getClass())); + zips[0] = zip; + } This is looking pretty airtight. Do you see any other potential problems? > Do you see any other potential problems?
Nope, this patch looks great; please check this puppy in! Committed derby-826-v.diff with revision 394334.
sysinfo now shows derby.jar information even if it's not explicitly in the classpath
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Minor
sysinfo does not report the version of derby.jar if the class does not explictly contain it.
Related to
DERBY-668, this fixes the regular (non -cp) part of that issue.I will commit this early next week if there are no concerns.