ConsenSysMesh / cava

ConsenSys core libraries for Java & Kotlin
Apache License 2.0
84 stars 34 forks source link

Backward compatibility with Java 8 broken #123

Closed atoulme closed 5 years ago

atoulme commented 5 years ago

As reported by users, backwards-compatibility with Java 8 is broken.

We will fix this ASAP.

cleishm commented 5 years ago

Odd. It should be compiled to 1.8 compatibility: https://github.com/ConsenSys/cava/blob/master/build.gradle#L159-L160

Are there descriptions as to what the issue is?

zilm13 commented 5 years ago

@cleishm we are using it as dependency in https://github.com/harmony-dev/beacon-chain-java and getting this error when switched to versions after 0.6.0-5618C0-snapshot

java.lang.NoSuchMethodError: java.lang.Math.multiplyExact(JI)J

    at net.consensys.cava.ssz.SSZ.listLengthPrefix(SSZ.java:707)
    at net.consensys.cava.ssz.SSZ.encodeHashListTo(SSZ.java:659)
    at net.consensys.cava.ssz.SSZ.encodeHashList(SSZ.java:646)
    at org.ethereum.beacon.ssz.type.BytesPrimitive.encodeList(BytesPrimitive.java:95)
    at org.ethereum.beacon.ssz.SSZCodecRoulette.lambda$resolveEncodeFunction$2(SSZCodecRoulette.java:60)
    at org.ethereum.beacon.ssz.SSZSerializer.encode(SSZSerializer.java:97)
    at org.ethereum.beacon.ssz.BytesSerializer.encode(BytesSerializer.java:23)
    at org.ethereum.beacon.ssz.SSZSerializerTest.explicitAnnotationsAndLoggerTest(SSZSerializerTest.java:115)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
    at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
    at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
    at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

I got it when running https://github.com/harmony-dev/beacon-chain-java/blob/master/ssz/src/test/java/org/ethereum/beacon/ssz/SSZSerializerTest.java

cleishm commented 5 years ago

Odd. java.lang.Math.multiplyExact is available in Java 8. Does this only happen in your IDE? Perhaps check your Intellij setup?

zilm13 commented 5 years ago

@cleishm same thing:

org.ethereum.beacon.ssz.SSZSerializerTest > simpleTest FAILED
    java.lang.NoSuchMethodError: java.lang.Math.multiplyExact(JI)J
        at net.consensys.cava.ssz.SSZ.listLengthPrefix(SSZ.java:707)
        at net.consensys.cava.ssz.SSZ.encodeHashListTo(SSZ.java:659)
        at net.consensys.cava.ssz.SSZ.encodeHashList(SSZ.java:646)
        at org.ethereum.beacon.ssz.type.BytesPrimitive.encodeList(BytesPrimitive.java:95)
        at org.ethereum.beacon.ssz.SSZCodecRoulette.lambda$resolveEncodeFunction$2(SSZCodecRoulette.java:60)
        at org.ethereum.beacon.ssz.SSZSerializer.encode(SSZSerializer.java:97)
        at org.ethereum.beacon.ssz.BytesSerializer.encode(BytesSerializer.java:23)
        at org.ethereum.beacon.ssz.SSZSerializerTest.simpleTest(SSZSerializerTest.java:88)
cleishm commented 5 years ago

I can't reproduce this:

% git diff
diff --git a/versions.gradle b/versions.gradle
index b38a05e..fb690d1 100644
--- a/versions.gradle
+++ b/versions.gradle
@@ -1,6 +1,6 @@
 ext {
   // After that version snapshots are compiled with new JDK w/o full backward compatibility
-  cavaVersion = '0.6.0-5618C0-snapshot'
+  cavaVersion = '0.6.0'
 }

 dependencyManagement {
% echo $JAVA_HOME
/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home
% which java
/Library/Java/JavaVirtualMachines/jdk1.8.0_181.jdk/Contents/Home/bin/java
% java -version
java version "1.8.0_181"
Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)
% ./gradlew --no-daemon clean ssz:build
To honour the JVM settings for this build a new JVM will be forked. Please consider using the daemon: https://docs.gradle.org/4.10.3/userguide/gradle_daemon.html.
Daemon will be stopped at the end of the build stopping after processing
Parallel execution with configuration on demand is an incubating feature.

> Task :ssz:compileJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :ssz:compileTestJava
/Users/cleishm/work/beacon-chain-java/ssz/src/test/java/org/ethereum/beacon/ssz/SSZHasherTest.java:18: warning: HexBin is internal proprietary API and may be removed in a future release
import static com.sun.org.apache.xerces.internal.impl.dv.util.HexBin.decode;
                                                             ^
/Users/cleishm/work/beacon-chain-java/ssz/src/test/java/org/ethereum/beacon/ssz/SSZSerializerTest.java:23: warning: HexBin is internal proprietary API and may be removed in a future release
import static com.sun.org.apache.xerces.internal.impl.dv.util.HexBin.decode;
                                                             ^
/Users/cleishm/work/beacon-chain-java/ssz/src/test/java/org/ethereum/beacon/ssz/fixtures/AttestationRecord.java:10: warning: Utility is internal proprietary API and may be removed in a future release
import static com.sun.org.apache.bcel.internal.classfile.Utility.toHexString;
                                                        ^
3 warnings

BUILD SUCCESSFUL in 5s
19 actionable tasks: 11 executed, 8 up-to-date

Are you sure your IDE isn't using a java version prior to Java 8? The multiplyExact methods are part of Java 8 after all, so compiling with Java 8 shouldn't report them as missing.

atoulme commented 5 years ago

If you use 0.6.0, which was built on my machine with a JDK 8 in a Docker container, you will not experience the issue. You might be able to reproduce with snapshots that are built on CircleCI with JDK11.

I'll get on with it soon. Having too much fun exploring Scuttlebutt at the moment.

cleishm commented 5 years ago

I'll check with one of those

zilm13 commented 5 years ago

@cleishm try ./gradlew clean ssz:test --info, I guess gradle is not running tests with your line, just compiling it

cleishm commented 5 years ago

Ok - I confirmed it using build 530016. There's a new flag from Java 9 to enable cross-compiling, which we'll need to set.

However, as @atoulme noted, this is currently only an issue for snapshot builds released by CI. You should be fine using the released 0.6.0 version.

Also, Java 11 is now the stable LTS version. You should consider dropping Java 8 for any new projects.

zilm13 commented 5 years ago

@cleishm cool, I will try release. When I've started there were no cava-ssz in release, so I used snapshot. As for switching to Java 11 - we may consider it, but I'm not sure all users are already with Java 11, we should prefer greater compatibility rather than ease of use.

cleishm commented 5 years ago

Fixed in #127

atoulme commented 5 years ago

Thanks! I'll merge into master now. Not sure we need a 0.6.1 build since the release should be without this problem.