search

ibmruntimes / openj9-openjdk-jdk11

Extensions for OpenJDK 11 for Eclipse OpenJ9
GNU General Public License v2.0
31 stars 112 forks source link

Fix and rename compareTwoFiles method in PackTestZip64 #615

Closed r30shah closed 1 year ago

r30shah commented 1 year ago

PackTestZip64 was reading jar file into the BufferedInputStream and comparing streams to verify if the jar files created by running pack and unpack command with reference is same or not. This is incorrect way to compare jar files as entries could be laid out differently in the jar file in case the implementation is using hardware support for compressing data. This commit fixes the compare routine (renamed to compareTwoJarFiles) by iterating through individiual entries in jar files and comparing them.

Signed-off-by: Rahil Shah rahil@ca.ibm.com

pshipton commented 1 year ago

Verified via https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/1874/

r30shah commented 1 year ago

@pshipton / @keithc-ca Given that you have reviewed similar changes in past, can I please get review on this changes?

keithc-ca commented 1 year ago

Please first propose the desired change in https://github.com/ibmruntimes/openj9-openjdk-jdk, then back-ports as necessary.

r30shah commented 1 year ago

@keithc-ca this test was written for pack200 which are limited to jdk11 and jdk8 lts versions. It wouldn't be there in JDK17 or other actively supported JDK versions or master JDK repo.

pshipton commented 1 year ago

If the equivalent testing exists in jdk8, we don't run it as part of sanity.openjdk or extended.openjdk afaik, so it's not a concern atm.

keithc-ca commented 1 year ago

The commit message and the description here should be updated to better describe the intent of this change, which to my mind, is more about correcting the comparison of jar files than the addition of a method.

r30shah commented 1 year ago

@keithc-ca I have also changed the commit message and hence the PR description. I do not expect this fundamental change to cause any issue with the test, though will checking it again on internal jenkins (Hence marking it WIP till I have it verified). Would appreciate if you can review again.

keithc-ca commented 1 year ago

Please launch a grinder like the one mentioned in https://github.com/ibmruntimes/openj9-openjdk-jdk11/pull/615#issuecomment-1402799119 and share the link here.

r30shah commented 1 year ago

I can not launch grinder on OpenJ9 jenkins, @pshipton can you help with that ? For sanity though I have launched one in internal jenkins (/job/Grinder/30737/console),

pshipton commented 1 year ago

https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/1895/

pshipton commented 1 year ago

Previous grinder passed. If there are more changes, it can just be restarted.

r30shah commented 1 year ago

@keithc-ca Grinders from @pshipton in https://openj9-jenkins.osuosl.org/view/Test/job/Grinder/1895/ passed and so as the one I launched in internal jenkins. If you are good with changes, can we merge this one?

keithc-ca commented 1 year ago

If the equivalent testing exists in jdk8, we don't run it as part of sanity.openjdk or extended.openjdk afaik, so it's not a concern atm.

The test does appear to exist, but, as you noted, we don't run it. Perhaps it's sufficient to test pack200 in jdk11.