eclipse-vertx / vertx-codegen

Vert.x code generator for asynchronous polyglot APIs
Apache License 2.0
103 stars 90 forks source link

Use multi release jar so TreeTypeInternal is not visible with JDK9 #161

Closed vietj closed 3 years ago

pratikpparikh commented 6 years ago

@vietj any update on this?

vietj commented 6 years ago

@gunnarmorling what's your opinion on the best way to solve this ?

pratikpparikh commented 6 years ago

@gunnarmorling it is a small world, I ran into this when doing our moditect enhancement for gradle plugin. I am really blocked by this for jdk image so any help you guys can render to solve this will be helpful.

@vietj Thank for looking at this.

vietj commented 6 years ago

we are but we would like to have help on this :-)

gunnarmorling commented 6 years ago

@vietj can you provide some more context on this one? Is TreeTypeInternal a type you want to remove going forward? AFAIK, you can't remove anything with an MR JAR, only override or add. You might consider to move that type into some legacy/compat/bridge module which people could optionally depend on during some kind of transitioning phase, but then I've not enough insight on what's the issue really is.

vietj commented 6 years ago

@gunnarmorling TreeTypeInternal uses com.sun classes because the JDK classes can't retrieve annotations at compilation on some elements (e.g Handler<AsyncResult<@Nullable String>>). Since Java 9 these limitations have been fixed in the compiler so we don't need this class. So we need this class in Java 8 exclusively and we can't provide a bridge because most of our users are on Java 8 and it should work out of the box.

One possibility is to replace this class in the MR jar by no-op implementation in /META-INF/versions/9/..../TreeTypeInternal.class.

gunnarmorling commented 6 years ago

Yeah, this could work. Or thinking about it, couldn't you base TreeTypeInternal in JDK 9 (via MR JAR as you say) on the correct compiler API? I.e. it'd use the work-around of using internal classes in JDK 8 and the right API in JDK 9. That way you don't need to switch yourself between using TreeTypeInternal and not using it based on the JDK version, the MR mechanism would do this for you.

vietj commented 6 years ago

we still use the normal API in JDK9, it uses an fallback mechanism, so first we try the JDK API and if it does not work we use the internals.

pratikpparikh commented 6 years ago

@vietj I think what @gunnarmorling is saying that the library can use the multi-release. The JDK-8 version to include the internal logic and for JDK-9 version we don't use it at all. If we keep it as the fallback mechanism then we might have the same issue of com.sun being included.

vietj commented 6 years ago

ok, so that's more or less what I initially said

pratikpparikh commented 6 years ago

@vietj I think that should work. if you can please have this fixed I can test this out very quickly.

gunnarmorling commented 6 years ago

Ok, just checked and com.sun.source.tree is still exported by the jdk.compiler module, so you should be fine by sticking to your fallback, although it should probably not be needed.

vietj commented 6 years ago

@gunnarmorling can you clarify ?

vietj commented 6 years ago

so what would be the proper way to have this i nthe build ?

On 15 Mar 2018, at 17:14, Gunnar Morling notifications@github.com wrote:

Ok, just checked and com.sun.source.tree is still exported by the jdk.compiler module, so you should be fine by sticking to your fallback, although it should probably not be needed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-codegen/issues/161#issuecomment-373433179, or mute the thread https://github.com/notifications/unsubscribe-auth/AANxis0nI_0sXMXVP0bx54vOIzv2aNhKks5tepNMgaJpZM4SDWMO.

gunnarmorling commented 6 years ago

@vietj I've provided a PR here: https://github.com/vert-x3/vertx-codegen/pull/168.

Note this doesn't add the changes to the build to produce an MR JAR, but it puts the code into the right places to support that. The idea is to have two flavors of the same class (one in the root dir, one under META-INF/versions/9), and the right one of the two will be enabled based on the version of the runtime.

For that I renamed the existing TreeTypeInternal; if you're concerned about that because of backwards compatability, you could keep that name and use it for the mirror-based implementation, too (this was an inner private class before, so it's fine to rename that one).

vietj commented 6 years ago

I reverted this PR because it creates a JAR with an invalid index. any idea @gunnarmorling ?

gunnarmorling commented 6 years ago

Doesn't ring a bell right now. Do you have any exception / error message to share?

vietj commented 6 years ago

we get this in vertx-core after this merge:

Starting test: DeploymentTest#testDeployAsSource 
java.lang.RuntimeException: Compilation failed
    at io.vertx.core.impl.verticle.CompilingClassLoader.<init>(CompilingClassLoader.java:115)
    at io.vertx.core.impl.JavaVerticleFactory.createVerticle(JavaVerticleFactory.java:33)
    at io.vertx.core.impl.DeploymentManager.createVerticles(DeploymentManager.java:229)
    at io.vertx.core.impl.DeploymentManager.lambda$doDeployVerticle$2(DeploymentManager.java:202)
    at io.vertx.core.impl.FutureImpl.setHandler(FutureImpl.java:79)
    at io.vertx.core.impl.DeploymentManager.doDeployVerticle(DeploymentManager.java:171)
    at io.vertx.core.impl.DeploymentManager.doDeployVerticle(DeploymentManager.java:143)
    at io.vertx.core.impl.DeploymentManager.deployVerticle(DeploymentManager.java:131)
    at io.vertx.core.impl.VertxImpl.deployVerticle(VertxImpl.java:654)
    at io.vertx.core.impl.VertxImpl.deployVerticle(VertxImpl.java:581)
    at io.vertx.test.core.DeploymentTest.testDeployAsSource(DeploymentTest.java:756)
    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.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
    at org.junit.rules.RunRules.evaluate(RunRules.java:20)
    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.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:283)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:173)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:128)
    at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:203)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:155)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)
Caused by: java.lang.RuntimeException: sun.misc.InvalidJarIndexException: Invalid index
    at com.sun.tools.javac.main.Main.compile(Main.java:559)
    at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:129)
    at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:138)
    at io.vertx.core.impl.verticle.CompilingClassLoader.<init>(CompilingClassLoader.java:99)
    ... 38 more
Caused by: sun.misc.InvalidJarIndexException: Invalid index
    at sun.misc.URLClassPath$JarLoader.getResource(URLClassPath.java:1140)
    at sun.misc.URLClassPath$JarLoader.getResource(URLClassPath.java:1050)
    at sun.misc.URLClassPath$JarLoader.findResource(URLClassPath.java:1020)
    at sun.misc.URLClassPath$1.next(URLClassPath.java:267)
    at sun.misc.URLClassPath$1.hasMoreElements(URLClassPath.java:277)
    at java.net.URLClassLoader$3$1.run(URLClassLoader.java:601)
    at java.net.URLClassLoader$3$1.run(URLClassLoader.java:599)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader$3.next(URLClassLoader.java:598)
    at java.net.URLClassLoader$3.hasMoreElements(URLClassLoader.java:623)
    at sun.misc.CompoundEnumeration.next(CompoundEnumeration.java:45)
    at sun.misc.CompoundEnumeration.hasMoreElements(CompoundEnumeration.java:54)
    at io.vertx.core.impl.verticle.PackageHelper.find(PackageHelper.java:46)
    at io.vertx.core.impl.verticle.MemoryFileManager.list(MemoryFileManager.java:81)
    at com.sun.tools.javac.api.ClientCodeWrapper$WrappedJavaFileManager.list(ClientCodeWrapper.java:231)
    at com.sun.tools.javac.jvm.ClassReader.fillIn(ClassReader.java:2803)
    at com.sun.tools.javac.jvm.ClassReader.complete(ClassReader.java:2446)
    at com.sun.tools.javac.jvm.ClassReader.access$000(ClassReader.java:76)
    at com.sun.tools.javac.jvm.ClassReader$1.complete(ClassReader.java:240)
    at com.sun.tools.javac.code.Symbol.complete(Symbol.java:574)
    at com.sun.tools.javac.comp.MemberEnter.visitTopLevel(MemberEnter.java:507)
    at com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:518)
    at com.sun.tools.javac.comp.MemberEnter.memberEnter(MemberEnter.java:437)
    at com.sun.tools.javac.comp.MemberEnter.complete(MemberEnter.java:1038)
    at com.sun.tools.javac.code.Symbol.complete(Symbol.java:574)
    at com.sun.tools.javac.code.Symbol$ClassSymbol.complete(Symbol.java:1037)
    at com.sun.tools.javac.comp.Enter.complete(Enter.java:493)
    at com.sun.tools.javac.comp.Enter.main(Enter.java:471)
    at com.sun.tools.javac.main.JavaCompiler.enterTrees(JavaCompiler.java:982)
    at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:857)
    at com.sun.tools.javac.main.Main.compile(Main.java:523)
    ... 41 more

see : https://vertx.ci.cloudbees.com/job/vert.x3-core/6524/testReport/io.vertx.test.core/DeploymentTest/testDeployAsSource/

pratikpparikh commented 6 years ago

@vietj what is next for this?

vietj commented 6 years ago

investigate the vertx core issue, this seems rather like a JDK bug.

vietj commented 6 years ago

it's a low priority issue for the moment for the core team, so you can investigate to know more about this blocker issue and then we can make a call

pratikpparikh commented 6 years ago

@vietj I can for sure pitch in? but can you provide minimal direction for how to set up and things that are needed for this completed?

vietj commented 3 years ago

closing for now, we'll reopen if that's really necessary at some point.