Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump asm version to 6.2 for JDK 10/11 support #1169

Closed
wants to merge 2 commits into from
Closed

Bump asm version to 6.2 for JDK 10/11 support #1169

wants to merge 2 commits into from

Conversation

svrakitin
Copy link

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@ronshapiro
Copy link
Contributor

Try "I signed it!" - the bot may not be smart enough to detect "we"

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Apr 3, 2018
@svrakitin
Copy link
Author

Is there anything I can do to speed up the process of getting this to master? ;)

@mkurz
Copy link
Contributor

mkurz commented Apr 10, 2018

Getting this merged would be really really great! 👍

@netdpb netdpb requested a review from sameb April 10, 2018 14:14
@sameb
Copy link
Member

sameb commented Apr 10, 2018

this looks reasonable, but can someone please clarify what about JDK10 fails w/ guice? also, can someone update the travis config to run over jdk10 too?

@netdpb i think the preferred way to do PRs now is to submit them internally w/ MOE (which i think has a way to pull in PRs and retain the author metadata?) and then push it back out. @ronshapiro might know.

@svrakitin
Copy link
Author

svrakitin commented Apr 10, 2018

@sameb asm's ClassReader will simply fail to read .class files with bytecode of 54.0 version.

Travis doesn't have oraclejdk10 and openjdk10 targets yet. ;(

@svrakitin
Copy link
Author

Any update on this?

@mkurz
Copy link
Contributor

mkurz commented May 30, 2018

FYI: ASM 6.2 released with better JDK 10 support and even with JDK 11 support already!
http://asm.ow2.io/versions.html
https://gitlab.ow2.org/asm/asm/tags
https://gitlab.ow2.org/asm/asm/issues/317830

@svrakitin svrakitin changed the title Bumped asm version to 6.1.1 for JDK 10 support Bump asm version to 6.2 for JDK 10/11 support May 30, 2018
@eximius313
Copy link

+1 for ASM 6.2

@pettyjamesm
Copy link

Any update on the timeline for this to make it into a release? Would be great to be able to use guice on newer JVMs.

@ronshapiro
Copy link
Contributor

We have run into a bumps in the road in upgrading this, but we are working on resolving them

@ronshapiro
Copy link
Contributor

Would it make sense to recommend that users bring their own copy of ASM if they care about the Java version they want to support? Is this simply just adding a different version of the library on the classpath?

@mkurz
Copy link
Contributor

mkurz commented Jun 6, 2018

Is there a actually a reason why Guice bundles asm in the final package? Why not keep it as normal dependency so users can just override it?

@sameb
Copy link
Member

sameb commented Jun 6, 2018 via email

@mcculls
Copy link
Contributor

mcculls commented Jun 6, 2018

Note that the build already produces a version of Guice that doesn't bundle asm or cglib:

http://repo1.maven.org/maven2/com/google/inject/guice/4.2.0/guice-4.2.0-classes.jar

This is a jar of the Guice classes before any JarJar'ing.

Also if you don't need AOP then there's the "no_aop" jar which doesn't use asm/cglib at all:

http://repo1.maven.org/maven2/com/google/inject/guice/4.2.0/guice-4.2.0-no_aop.jar

@prodsey
Copy link

prodsey commented Jun 8, 2018

This is great.
We are planning to migrate an enterprise project (extensively uses guice ) from JDK 9 to JDK 10
Any ballpark ETA on this change to support JDK 10?
Thanks

@prodsey
Copy link

prodsey commented Jun 26, 2018

Any update on when can we expect jdk 10, 11 supported release?

@prodsey
Copy link

prodsey commented Jun 28, 2018

Below exception on startup with 4.2 release + JDK 10
I am thinking this is related?

Any update on the release?

Caused by: java.lang.IllegalArgumentException
at com.google.inject.internal.asm.$ClassReader.(ClassReader.java:160)
at com.google.inject.internal.asm.$ClassReader.(ClassReader.java:143)
at com.google.inject.internal.asm.$ClassReader.(ClassReader.java:418)
at com.google.inject.internal.cglib.proxy.$BridgeMethodResolver.resolveAll(BridgeMethodResolver.java:69)
at com.google.inject.internal.cglib.proxy.$Enhancer.emitMethods(Enhancer.java:1132)
at com.google.inject.internal.cglib.proxy.$Enhancer.generateClass(Enhancer.java:630)
at com.google.inject.internal.cglib.core.$DefaultGeneratorStrategy.generate(DefaultGeneratorStrategy.java:25)
at com.google.inject.internal.cglib.core.$AbstractClassGenerator.generate(AbstractClassGenerator.java:329)

@svrakitin
Copy link
Author

@prodsey this is original issue :)

@prodsey
Copy link

prodsey commented Jun 28, 2018

thanks @svrakitin

any ETA on when this will be released?

I tried with
[classifier]no_aop[/classifier] and additionally including ASM 6.2 and cglib 3.2.6 (did not work)

any other way I can start testing with JDK 10?

@himanshukandwal
Copy link

Facing the same issue after applying the suggested fixes.

Caused by: java.lang.IllegalArgumentException
at [email protected]/com.google.inject.internal.asm.$ClassReader.(ClassReader.java:160)
at [email protected]/com.google.inject.internal.asm.$ClassReader.(ClassReader.java:143)
at [email protected]/com.google.inject.internal.asm.$ClassReader.(ClassReader.java:418)
at [email protected]/com.google.inject.internal.util.LineNumbers.(LineNumbers.java:64)
at [email protected]/com.google.inject.internal.util.StackTraceElements$1.load(StackTraceElements.java:49)
at [email protected]/com.google.inject.internal.util.StackTraceElements$1.load(StackTraceElements.java:45)
at [email protected]/com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3443)
at [email protected]/com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2169)
at [email protected]/com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2128)
at [email protected]/com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2041)

@mcculls
Copy link
Contributor

mcculls commented Jun 29, 2018

@prodsey what was the error you had with the 'no_aop' jar? That doesn't use ASM or CGLIB at all, but at the same time you won't have access to any AOP features such as method interception. If you're still seeing ASM related errors with the 'no_aop' jar then check your classpath in case it contains another version of Guice.

If you want to try Guice with AOP and a different version of ASM/CGLIB then you should take the [classifier]classes[/classifier] jar and add ASM 6.2 and CGLIB 3.2.6 as separate dependencies. Again making sure the original Guice jar isn't being dragged onto the same project classpath.

( of course an official release with this version bump would also be appreciated! )

@prodsey
Copy link

prodsey commented Jun 29, 2018

@mcculls
With no_aop and asm 6.2 + latest cglib I got the same exception .... checked dep hierarchy and removed all older versions of guice. But still got the same exception. I need method interceptors. I have not tried [classifier]classes[/classifier] . Should that work?

I do not see [classifier]classes[/classifier] option in below link
https://github.com/google/guice/wiki/Guice42

I agree, an official release with JDK 10,11 support will definitely help.

@sameb
Can you please throw some light on when can we expect an official release?

@prodsey
Copy link

prodsey commented Jun 30, 2018

@mcculls
Thank you for the input. [classifier]classes[/classifier] Works fine when you include asm 6.2 and cglib JARs. I did some basic testing with OpenJDK 10.0.1.

I guess this is a good option till the official release right? Do you see any downside?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants