Having a gradle task to ensure that class files are valid bytecode seems like a good idea. It may not be 100% bulletproof since we yet rely on another tool/library to perform the verification but still having an extra check will not hurt if it is lightweight.
It makes sense to enforce a certain JDK version/build during the release. In that case I would suggest also having some gradle logic perform this validation and not just text in the release instructions. Automating the release procedure would be great but to do that it is required to have reproducible builds [1]. See [2] for more background around this topic. Best, Stamatis [1] https://cwiki.apache.org/confluence/display/SECURITY/Reproducible+Builds [2] https://lists.apache.org/thread/qkstx4o9qw90fxzqcfnp69w33h7vw2yg On Fri, Jan 26, 2024 at 11:27 PM Guillaume Masse <masse.guilla...@narrative.io.invalid> wrote: > > Hi Julian, > > I don't think it's necessary to require JDK > 8. Here is what I propose: > > 1) I can add a gradle task to make sure each classfile from the release > jars are valid bytecode. > > 2) I think we need to make sure we compile with the latest build available. > Per https://lists.apache.org/thread/nrt4ysoc14p20sq23z744jyfqh1bznyh it was > compiled with 8u371 the 10 of November 2023. At the time the latest build > of java 8 was 391, so that's 2 builds behind: > > 371 > 18 April 2023 > 381 > 18 July 2023 > 391 > 17 October 2023 > 401 > 16 January 2024 > > I can't be sure that this was the root cause for the broken classfile. > > 3) Let's make the release procedure automatic / part of the CI. I can > assist on this task, but I will need help from commiters since I don't > have any credentials for maven central. > > Guillaume > > > On Fri, Jan 26, 2024 at 2:44 PM Julian Hyde <jhyde.apa...@gmail.com> wrote: > > > These JDK bugs seem to be fixed in more recent java versions. Should we > > mandate that releases are built on a recent JDK (say 17 or 21)? Currently > > we mandate JDK 8. > > > > > > > On Jan 26, 2024, at 9:03 AM, Ruben Q L <rube...@gmail.com> wrote: > > > > > > I have the impression that this might be caused by a bug in the JDK, see > > > similar issues: > > > https://gitlab.ow2.org/asm/asm/-/issues/317789 > > > https://bugs.openjdk.org/browse/JDK-8144185 > > > https://bugs.openjdk.org/browse/JDK-8187805 > > > > > > > > > > > > On Thu, Dec 21, 2023 at 8:58 PM Julian Hyde <jh...@apache.org> wrote: > > > > > >> I think I was mistaken about Guava. I agree with Benchao's analysis > > >> and #2 seems to be caused by something other than Guava. > > >> > > >> On Thu, Dec 21, 2023 at 3:53 AM Benchao Li <libenc...@apache.org> > > wrote: > > >>> > > >>> There are two exceptions here: > > >>> > > >>> # 1 > > >>> java.lang.ArrayIndexOutOfBoundsException: Index 65536 out of bounds > > >>> for length 297 > > >>> at org.objectweb.asm.ClassReader.readLabel(ClassReader.java:2695) > > >>> at org.objectweb.asm.ClassReader.createLabel(ClassReader.java:2711) > > >>> at > > >> org.objectweb.asm.ClassReader.readTypeAnnotations(ClassReader.java:2777) > > >>> at org.objectweb.asm.ClassReader.readCode(ClassReader.java:1929) > > >>> at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1515) > > >>> at org.objectweb.asm.ClassReader.accept(ClassReader.java:745) > > >>> at org.objectweb.asm.ClassReader.accept(ClassReader.java:425) > > >>> at remapper.bug.RemapperTest.runTest(RemapperTest.java:60) > > >>> at remapper.bug.RemapperTest.calcite36(RemapperTest.java:36) > > >>> > > >>> # 2 > > >>> java.lang.IllegalArgumentException: Invalid end label (must be visited > > >> first) > > >>> at > > >> > > org.objectweb.asm.util.CheckMethodAdapter.checkLabel(CheckMethodAdapter.java:1453) > > >>> at > > >> > > org.objectweb.asm.util.CheckMethodAdapter.visitLocalVariableAnnotation(CheckMethodAdapter.java:996) > > >>> at > > >> > > org.objectweb.asm.MethodVisitor.visitLocalVariableAnnotation(MethodVisitor.java:757) > > >>> at > > >> > > org.objectweb.asm.commons.MethodRemapper.visitLocalVariableAnnotation(MethodRemapper.java:257) > > >>> at org.objectweb.asm.ClassReader.readCode(ClassReader.java:2614) > > >>> at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1515) > > >>> at org.objectweb.asm.ClassReader.accept(ClassReader.java:745) > > >>> at org.objectweb.asm.ClassReader.accept(ClassReader.java:425) > > >>> at remapper.bug.RemapperTest.runTest(RemapperTest.java:53) > > >>> at remapper.bug.RemapperTest.calcite36WithCheck(RemapperTest.java:39) > > >>> > > >>> > > >>> Sorry that I'm not clear in my previous email. My testing results are > > >>> all about #1. > > >>> > > >>> I tested 1.34.0/1.35.0/1.36.0 with JDK8 on both Linux and Mac for #2, > > >>> they all reproduced the problem. Hence this is not a problem of > > >>> platform, and also not a problem of 1.35.0->1.36.0, I guess this is an > > >>> asm usage problem related to JDK8's output. > > >>> > > >>> In summary, > > >>> for #1 problem, it seems to be a problem of the Guava version we're > > >>> compiling against, > > >>> for #2 problem, it seems not to be a problem since both > > >>> 1.34.0/1.35.0/1.36.0 can reproduce it (I assume that this does not > > >>> affect your usage since you only spotted this after 1.36.0). > > >>> > > >>> Guillaume Masse <masse.guilla...@narrative.io.invalid> 于2023年12月21日周四 > > >> 03:29写道： > > >>>> > > >>>> calcite-1.35.0 with JDK8(1.8.0_391) on MacOS with M1 Pro chip: can > > >> reproduce > > >>>> > > >>>> RemapperTest > calcite35WithCheck() FAILED > > >>>> java.lang.IllegalArgumentException: Invalid start label (must be > > >>>> visited first) > > >>>> at > > >> > > org.objectweb.asm.util.CheckMethodAdapter.checkLabel(CheckMethodAdapter.java:1453) > > >>>> at > > >> > > org.objectweb.asm.util.CheckMethodAdapter.visitLocalVariableAnnotation(CheckMethodAdapter.java:995) > > >>>> at > > >> > > org.objectweb.asm.MethodVisitor.visitLocalVariableAnnotation(MethodVisitor.java:757) > > >>>> at > > >> > > org.objectweb.asm.commons.MethodRemapper.visitLocalVariableAnnotation(MethodRemapper.java:257) > > >>>> at > > >> org.objectweb.asm.ClassReader.readCode(ClassReader.java:2614) > > >>>> at > > >> org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1515) > > >>>> at org.objectweb.asm.ClassReader.accept(ClassReader.java:745) > > >>>> at org.objectweb.asm.ClassReader.accept(ClassReader.java:425) > > >>>> at remapper.bug.RemapperTest.runTest(RemapperTest.java:53) > > >>>> at > > >> remapper.bug.RemapperTest.calcite35WithCheck(RemapperTest.java:32) > > >>>> > > >>>> > > >> > > https://github.com/MasseGuillaume/asm-remapper-bug/blob/main/SqlFunctions-1.35.0.class > > >>>> > > >>>> If you kept the .class for your tests, can you try to run javap -p -c > > >> -v > > >>>> > > >>>> On Wed, Dec 20, 2023 at 1:11 PM Julian Hyde <jhyde.apa...@gmail.com> > > >> wrote: > > >>>> > > >>>>> The version of Guava that you use at compile time is important. If > > >> it’s > > >>>>> earlier than 20 (-ish) the byte codes will be different because of > > >> the > > >>>>> presence/absence of certain varargs methods. > > >>>>> > > >>>>> > > >>>>>> On Dec 19, 2023, at 7:28 PM, Benchao Li <libenc...@apache.org> > > >> wrote: > > >>>>>> > > >>>>>> I tried to reproduce this using Guillaume's > > >>>>>> project(https://github.com/MasseGuillaume/asm-remapper-bug), and > > >> the > > >>>>>> findings are below: > > >>>>>> > > >>>>>> - compiled latest main branch > > >>>>>> (50a20824c4536450dcae963b5e757cf4bbc7e406) with JDK8(1.8.0_391) on > > >>>>>> MacOS with M2 chip: reproduced > > >>>>>> - compiled latest main branch > > >>>>>> (50a20824c4536450dcae963b5e757cf4bbc7e406) with JDK8(1.8.0_391) on > > >>>>>> Linux with x64 architecture: reproduced > > >>>>>> - compiled latest main branch > > >>>>>> (50a20824c4536450dcae963b5e757cf4bbc7e406) with JDK11(1.11.0_21) on > > >>>>>> MacOS with M2 chip: cannot reproduce > > >>>>>> - compiled latest main branch > > >>>>>> (50a20824c4536450dcae963b5e757cf4bbc7e406) with JDK17(1.17.0_7) on > > >>>>>> MacOS with M2 chip: cannot reproduce > > >>>>>> > > >>>>>> It seems unrelated to platforms, and I assume it should not either, > > >>>>>> since Java's bytecode should be platform independent. > > >>>>>> > > >>>>>> @Guillaume You said above that with 1.8.0_371-b11 on macos x64, you > > >>>>>> cannot reproduce the problem, can you confirm that? Per my testing, > > >>>>>> JDK8's output could all reproduce the problem > > >>>>>> > > >>>>>> Guillaume Masse <masse.guilla...@narrative.io.invalid> > > >> 于2023年12月18日周一 > > >>>>> 23:13写道： > > >>>>>>> > > >>>>>>> If I build 1.36.0 and 1.35.0 with the same java version/build > > >> javap can > > >>>>>>> read the classfiles. Since the release process is manual it's > > >> really > > >>>>> hard > > >>>>>>> to know what causes the problem. My solution is to build and > > >> release > > >>>>> each > > >>>>>>> Calcite version myself and host it on our own S3 repository. > > >>>>>>> > > >>>>>>> On Mon, Dec 18, 2023, 8:56 AM Ruben Q L <rube...@gmail.com> > > >> wrote: > > >>>>>>> > > >>>>>>>> Hello, > > >>>>>>>> > > >>>>>>>> My project also recently upgraded to Calcite 1.36, and we are > > >> facing > > >>>>> the > > >>>>>>>> exact same issue when trying to shade with ASM. > > >>>>>>>> > > >>>>>>>> @Guillame , I can see that > > >>>>> https://gitlab.ow2.org/asm/asm/-/issues/318008 > > >>>>>>>> has > > >>>>>>>> been closed, but I can't really understand why, since the > > >> explanation > > >>>>>>>> mentions SqlFunctions-1.35.0, but the issue is with 1.36, do you > > >> have > > >>>>> more > > >>>>>>>> info on that? > > >>>>>>>> > > >>>>>>>> Best, > > >>>>>>>> Ruben > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> On Fri, Nov 24, 2023 at 10:09 PM Guillaume Masse > > >>>>>>>> <masse.guilla...@narrative.io.invalid> wrote: > > >>>>>>>> > > >>>>>>>>> We run spark in AWS EMR and it overrides the classpath, so we > > >> don't > > >>>>> have > > >>>>>>>>> control over it: > > >>>>>>>>> > > >>>>>>>>> Sparks will pull guava 14.0.1: > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>> > > >> > > https://github.com/apache/spark/blob/master/dev/deps/spark-deps-hadoop-3-hive-2.3#L68 > > >>>>>>>>> > > >>>>>>>>> That's why we need to shade guava. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> On Fri, Nov 24, 2023 at 4:31 PM Julian Hyde <jh...@apache.org> > > >> wrote: > > >>>>>>>>> > > >>>>>>>>>> The version of Guava we compile against is relevant. In 1.35 we > > >>>>>>>>>> compiled against Guava 19.0; in 1.36 we compiled against Guava > > >>>>>>>>>> 32.1.3-jre. The effect would be the same if we compiled > > >> against any > > >>>>>>>>>> version of Guava 20.0 or higher, due to the addition of > > >>>>>>>>>> Preconditions.checkArgument methods that in earlier Guava > > >> versions > > >>>>>>>>>> would be handled by varargs method. > > >>>>>>>>>> > > >>>>>>>>>> See https://issues.apache.org/jira/browse/CALCITE-5477 (which > > >> was > > >>>>>>>>>> fixed in 1.35) and > > >>>>> https://issues.apache.org/jira/browse/CALCITE-5763 > > >>>>>>>>>> (which was fixed in 1.36 and essentially reverted 5477). > > >>>>>>>>>> > > >>>>>>>>>> Julian > > >>>>>>>>>> > > >>>>>>>>>> On Fri, Nov 24, 2023 at 10:20 AM Guillaume Masse > > >>>>>>>>>> <masse.guilla...@narrative.io.invalid> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>> Hi Benchao Li, > > >>>>>>>>>>> > > >>>>>>>>>>> thanks for giving me more details like the java version, > > >>>>>>>>>>> > > >>>>>>>>>>> I compiled with 1.8.0_371-b11 on macos x64 (emulated) and I > > >> was > > >>>>> still > > >>>>>>>>>> able > > >>>>>>>>>>> to transform the classfile. The bug must come from something > > >> else. > > >>>>>>>> Was > > >>>>>>>>>> this > > >>>>>>>>>>> compiled on your personal machine? Was it on > > >> windows/linux/mac? > > >>>>>>>> What's > > >>>>>>>>>> the > > >>>>>>>>>>> processor architecture? > > >>>>>>>>>>> > > >>>>>>>>>>> 1.35.0 also pases the transformation and the same piece of > > >> code is > > >>>>>>>>> there. > > >>>>>>>>>>> How was 1.35.0 released? > > >>>>>>>>>>> > > >>>>>>>>>>> Thanks! > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> On Thu, Nov 23, 2023 at 9:59 PM Benchao Li < > > >> libenc...@apache.org> > > >>>>>>>>> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>>> Guillaume, > > >>>>>>>>>>>> > > >>>>>>>>>>>> The binary release are always compiled with JDK8, this is a > > >>>>>>>> required > > >>>>>>>>>>>> procedure[1]. > > >>>>>>>>>>>> > > >>>>>>>>>>>> For 1.36.0, the JDK version I'm using is: > > >>>>>>>>>>>> $ java -version > > >>>>>>>>>>>> java version "1.8.0_371" > > >>>>>>>>>>>> Java(TM) SE Runtime Environment (build 1.8.0_371-b11) > > >>>>>>>>>>>> Java HotSpot(TM) 64-Bit Server VM (build 25.371-b11, mixed > > >> mode) > > >>>>>>>>>>>> > > >>>>>>>>>>>> [1] > > >>>>>>>>>> > > >>>>> > > >> https://calcite.apache.org/docs/howto.html#making-a-release-candidate > > >>>>>>>>>>>> > > >>>>>>>>>>>> Guillaume Masse <masse.guilla...@narrative.io.invalid> > > >>>>>>>>> 于2023年11月24日周五 > > >>>>>>>>>>>> 07:22写道： > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Hi all, > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> We recently upgraded to calcite 1.36.0 and it broke our > > >>>>>>>> deployment > > >>>>>>>>>>>> process. > > >>>>>>>>>>>>> We are using java asm <https://asm.ow2.io/> to shade > > >> (rename > > >>>>>>>>>> packages > > >>>>>>>>>>>> from > > >>>>>>>>>>>>> the bytecode) since there are multiple dependencies clash > > >> when > > >>>>>>>>> using > > >>>>>>>>>> with > > >>>>>>>>>>>>> Apache Spark. For example, Apache Spark is using a much > > >> older > > >>>>>>>>>> version of > > >>>>>>>>>>>>> guava. If I build calcite locally with Java Correto > > >> 17.0.7-amzn I > > >>>>>>>>> can > > >>>>>>>>>>>>> transform calcite's bytecode without any problem. Where can > > >> I > > >>>>>>>> find > > >>>>>>>>>> the > > >>>>>>>>>>>> java > > >>>>>>>>>>>>> version used to compiled calcite? Is there a process in > > >> place to > > >>>>>>>>>> have a > > >>>>>>>>>>>>> consistent java version? > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> https://gitlab.ow2.org/asm/asm/-/issues/318008 > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> -- > > >>>>>>>>>>>>> Guillaume Massé > > >>>>>>>>>>>>> [Gee-OHM] > > >>>>>>>>>>>>> (马赛卫) > > >>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>>> -- > > >>>>>>>>>>>> > > >>>>>>>>>>>> Best, > > >>>>>>>>>>>> Benchao Li > > >>>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> -- > > >>>>>> > > >>>>>> Best, > > >>>>>> Benchao Li > > >>>>> > > >>>>> > > >>> > > >>> > > >>> > > >>> -- > > >>> > > >>> Best, > > >>> Benchao Li > > >> > > > >
