Hi Andrew, Thanks for the review!
On Wed, 2019-08-28 at 18:15 +0100, Andrew John Hughes wrote: > On 26/08/2019 14:23, Severin Gehwolf wrote: > > Hi, > > > > Could I please get a review of this MUSCLE header files update in > > OpenJDK 8u? I'd like to backport this bug as it's also going to be in > > Oracle JDK 8u231 (equiv to OpenJDK 8u232) as well. The OpenJDK 11 patch > > applies almost cleanly post path-unshuffelling. Changes which didn't > > apply were a copyright header update in pcsc.c. I've omitted these. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8218780 > > webrev: > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/01/webrev/ > > JDK 11u changeset: > > https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/0bcc59a50c88 > > > > There is going to be a follow-up fix adding back COPYING via > > JDK-8226607 which I propose for backport to OpenJDK 8u as well. > > > > Testing: smartcardio sanity and build on Linux x86_64 (Fedora 30 and RHEL 6) > > > > Thanks to Aleksey Shipilev who helped test this patch. > > > > Thoughts? > > > > Thanks, > > Severin > > > > Most of this looks good. I was a little confused at first because the > patch in your webrev looks quite different to the 11u changeset. > However, once applied locally to the 8u repo, the diff between the two > was as suggested and the PCSC library files (those in MUSCLE) were > identical. I don't know what webrev did in creating that patch. > > The bit I don't understand is why you've decided to drop the copyright > header change on the floor. Just because the original in 8u has 2014, > while the original in 11u had 2015, does not make the change inapplicable. OK. I see. I wasn't sure how to deal with copyright year updates. I've added the copyright update back. The patch is now identical to JDK 11u (modulo differing copyright year base: 2014 - jdk 8 - vs. 2018 - jdk 11): http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/02/webrev > A better alternative may actually be to backport JDK-8207233 [0] first > which is a nice little cleanup patch. I suspect this patch would then > apply cleanly as these PCSC files are rarely touched. > > [0] https://bugs.openjdk.java.net/browse/JDK-8207233 Hmm, this seems overkill just to get the copyright hunk to apply. I'd prefer to keep this dependency out of scope for this patch. While a nice clean-up it's not without risk backporting that too. Thoughts? Thanks, Severin
