Thanks for sponsoring the patch.. Best, Diego
On Tue, Jun 19, 2012 at 7:15 AM, Chris Hegarty <chris.hega...@oracle.com>wrote: > To close the loop on this, here is a link to the changeset: > > > http://hg.openjdk.java.net/**jdk8/tl/jdk/rev/efc2791d7c5d<http://hg.openjdk.java.net/jdk8/tl/jdk/rev/efc2791d7c5d> > > Thanks for this valuable contribution Diego. > > -Chris > > > On 16/06/12 05:20, Diego Belfer wrote: > >> Hi Chris, >> >> Here is a new patch containing new version of the tests. The new >> versions generate a the Jar on the fly as we discussed. >> >> Let me know if there is anything else you think should be improved. >> >> Best, >> Diego >> >> On Thu, Jun 14, 2012 at 7:32 AM, Chris Hegarty <chris.hega...@oracle.com >> <mailto:chris.hegarty@oracle.**com <chris.hega...@oracle.com>>> wrote: >> >> Diego, >> >> It's not too difficult to create jars on the fly using the Jar API. >> Here is a small example that I think would work nice in this case. >> Files created ( and paths are relative to the jtreg scratch, or >> working dir if running outside of jtreg ). >> >> Do you think you could use similar to create the jars for your test? >> >> createJar("a.jar", jarAList); >> createJar("b.jar", jarBList); >> ....... >> >> static void createJar(String jarName, Map<String,String> contents) >> throws Exception >> { >> try (FileOutputStream aJar = new FileOutputStream(jarName); >> JarOutputStream jos = new JarOutputStream(aJar)) { >> Set<Entry<String,String>> entries = contents.entrySet(); >> for (Entry<String,String> entry : entries) >> writeJarEntry(jos, entry.getKey(), >> entry.getValue().getBytes("__**ASCII")); >> >> } >> } >> >> static void writeJarEntry(JarOutputStream jos, String name, >> byte[] data) >> throws Exception >> { >> JarEntry entry = new JarEntry(name); >> jos.putNextEntry(entry); >> jos.write(data); >> } >> >> static final Map<String,String> jarAList = new HashMap<>(); >> static final Map<String,String> jarBList = new HashMap<>(); >> static { >> jarAList.put("com/foo/__**resource1.txt", "some random data"); >> jarAList.put("com/bar/__**resource2.txt", "some more random >> data!"); >> jarAList.put("com/baz/__**resource3.txt", "even more random >> data!!!"); >> jarBList.put("x/y/resourceA.__**dat", "Hello there"); >> jarBList.put("x/y/resourceB.__**dat", "Goodbye"); >> jarBList.put("x/y/resourceC.__**dat", >> "Hello\nfrom\nb\ndot\njar"); >> >> } >> >> Thanks, >> -Chris. >> >> >> On 14/06/2012 03:20, Diego Belfer wrote: >> >> Hi Chris, >> >> There is no way to generate a jar without directory entries >> using the >> jar tool; there is no option for that. What do you think is the >> best way >> to handle this ? >> I don't want to re-implement the logic for creating a jar using the >> JarOutputStream class. >> >> Do you think it is possible to add a new option to the Jar tool >> Main >> class to exclude directory entries? The option does not need to be >> exposed by the command line tool to final users if this an issue, >> although I think it may be useful for them too. >> >> Best, >> Diego >> >> >> On Wed, Jun 13, 2012 at 7:12 PM, Diego Belfer <dbel...@gmail.com >> <mailto:dbel...@gmail.com> >> <mailto:dbel...@gmail.com <mailto:dbel...@gmail.com>>> wrote: >> >> Chris, >> >> I was thinking of something similar. I will create the jars and >> their contents using Java code. There is no need of creating >> real >> class files, using a few resource files and some directories >> will be >> enough. >> >> Best, >> Diego >> >> >> On Wed, Jun 13, 2012 at 6:46 PM, chris hegarty >> <chris.hega...@oracle.com >> <mailto:chris.hegarty@oracle.**com<chris.hega...@oracle.com> >> > >> <mailto:chris.hegarty@oracle._**_com >> <mailto:chris.hegarty@oracle.**com <chris.hega...@oracle.com>>>> >> wrote: >> >> Diego, >> >> I'm thinking that some of the trivial source files, to >> compile >> and built into the jars, could be simply created and >> written by >> the test itself, rather than checking them all in. If >> this makes >> it cleaner. I really don't like all the file in >> test/sun/misc/JarIndex/____**metaInfFilenames, but at >> >> least it is >> >> quite understandable. >> >> -Chris. >> >> >> On 13/06/2012 20:36, Diego Belfer wrote: >> >> Hi Chris, >> >> That's right. The only non-cleanup change is the one >> in the >> merge. >> >> Regarding the test case, I will re-write them in >> order to >> generate the >> jars on fly. I'd scanned the jdk/test folder and >> found a few >> jars, >> that's why I included them. I have seen your test >> case, I >> will use it >> as a sample. >> >> I had not seen your comment in the bug report. Maybe >> there >> are other >> cases which trigger the InvalidJarIndexException, >> but, as >> far as I could >> see, the validIndex method checks that at least one >> entry of >> the jar >> matches the target path found in the index. If >> directory >> entries are not >> present in the jar, stripped paths generated during the >> merge and used >> by the index will return jars which may not contain >> entries >> for them, >> triggering the exception. >> When all directory entries are present, if a jar >> contains an >> entry for >> "xxx/yyy/resource.file", it will contain entries for "xxx", >> "xxx/yyy" >> and "xxx/yyy/resource.file". >> >> >> Best, >> Diego >> >> >> On Wed, Jun 13, 2012 at 12:05 PM, Chris Hegarty >> <chris.hega...@oracle.com >> <mailto:chris.hegarty@oracle.**com<chris.hega...@oracle.com> >> > >> <mailto:chris.hegarty@oracle._**_com >> <mailto:chris.hegarty@oracle.**com <chris.hega...@oracle.com>>> >> <mailto:chris.hegarty@oracle. <mailto:chris.hegarty@oracle.>** >> ____com >> >> >> <mailto:chris.hegarty@oracle._**_com >> <mailto:chris.hegarty@oracle.**com <chris.hega...@oracle.com>>>>> >> wrote: >> >> Hi Diego, >> >> Thanks for picking up this bug. >> >> I think your changes look fine. Mainly cleanup >> except >> for add -> >> addExplicit/addMapping in merge, right? BTW the >> cleanup >> makes this >> more readable. >> >> Unfortunately, the tests you created require >> checking in >> a binary >> jar file. This is a real no no for the OpenJDK, we >> really need to >> create these jars on the fly. I did similar for >> test/sun/misc/JarIndex/______**metaInfFilenames/, >> >> but I >> >> really wish I >> >> generated the source files for these tests >> rather than >> checking in >> so many pointless files. >> >> I can look at helping with writing suitable >> tests for this. >> >> >> > That's because I was using jars containing "directory >> entries" >> > (I was unaware that jar files may not include them) >> >> Strangely I added the comment "Remove >> directories from >> jar files >> being indexed." to the workaround section of the >> bug. >> You seem to be >> seeing the opposite, right? >> >> -Chris. >> >> >> >> On 13/06/2012 06:11, Diego Belfer wrote: >> >> Hi, >> >> I have finally reproduced the >> InvalidJarIndexException bug as >> reported in >> the ticket. I mentioned in a previous email, >> that >> the only way >> I'd found >> for getting the error was to use an invalid >> index file >> (INDEX.LIST), which >> did not have any sense. That's because I was >> using >> jars containing >> "directory entries" (I was unaware that jar files may not >> include them) >> >> After reviewing the URLClasspath$JarLoader >> class and the >> validIndex method, >> I notice it is possible to get the exception >> for a >> Jar file >> which does not >> include directory entries. In order to >> trigger the >> issue, the >> Jar must be >> referenced by an intermediary INDEX.LIST and >> the >> intermediary >> Jar index >> should have been merged to its parent index. >> Although, jar tool >> includes >> directory entries in the generated jar files, >> Eclipse default >> option for >> exporting jars does not include them (AFAIK), >> so >> this might be >> quite common. >> >> I have created a new PATCH which includes an >> additional test >> case which >> uses the URLClassLoader to trigger the >> InvalidIndexException. >> >> The patch is attached, please consider it >> for review. >> >> Best, >> Diego Belfer [muralx] >> >> >> >> On Mon, Jun 11, 2012 at 4:47 PM, Diego >> Belfer<dbel...@gmail.com <mailto:dbel...@gmail.com> >> <mailto:dbel...@gmail.com <mailto:dbel...@gmail.com>> >> <mailto:dbel...@gmail.com <mailto:dbel...@gmail.com> >> <mailto:dbel...@gmail.com <mailto:dbel...@gmail.com>>>> wrote: >> >> >> >> Hi, >> >> Here is a patch that fixes the merge >> method of >> the JarIndex. >> This bug was >> reported as the cause of the bug 6901992. >> Although, I was >> not able to >> reproduce the BUG itself >> (InvalidJarIndexException), I did >> verified that >> the method had a bug, and resources/classes >> where not found >> in a jarIndex >> with merged contents. >> >> If you think it is possible to commit >> this fix >> without actually >> reproducing the original bug report, please >> consider this >> patch for review. >> >> Thanks, >> Diego Belfer [muralx] >> >> >> >> >> >> >>
