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

Repackage asm #54

Closed
greenlaw110 opened this issue Mar 11, 2017 · 17 comments
Closed

Repackage asm #54

greenlaw110 opened this issue Mar 11, 2017 · 17 comments

Comments

@greenlaw110
Copy link

This library use asm 5.1 (at the moment) direclty. However as per http://asm.ow2.org/doc/faq.html#Q15, it suggest that

Tools and frameworks that are using ASM for bytecode processing (e.g. Hibernate, CGLIB, AspectWerkz) should repackage ASM code within their own name space. This can be automated with Jar Jar Links tool.

Here is a real issue caused by ASM library version conflict between sun's jersey and ReflectASM

@NathanSweet
Copy link
Member

I tried, jar jar links appears to be broken.

@greenlaw110
Copy link
Author

@NathanSweet

Well IDE is your friend, it's very simple to load all ASM code into your ide and run a code refactory to change the package name. You don't really need jarjar to work.

@NathanSweet
Copy link
Member

I'd rather modify the built artifact provided by ObjectWeb than repackage it from source each time it needs updating. As you can see in the jarjar issue, I got it to work. Their docs were outdated and their launcher code half wrong.

@greenlaw110
Copy link
Author

My approach is to create a separate project that package the asm, and maintain that project including release it as a separate jar in the maven repository.

But anyway it's your call

@magro
Copy link
Collaborator

magro commented Jul 16, 2017

@NathanSweet I guess pom.xml still has to be adjusted to make this work for other users and in the context of releasing to maven, right?!

@NathanSweet
Copy link
Member

@magro Yes.

@magro magro reopened this Jul 16, 2017
@magro
Copy link
Collaborator

magro commented Jul 16, 2017

We're already building/releasing a shaded jar, creating an additional artifact with a classifier attached. We could use the shading approach and just release the shaded jar instead of using jarjar (not sure how this would be integrated via maven).

@NathanSweet
Copy link
Member

Normally I would leave shading to whoever is cobbling together conflicting dependencies, but if the ASM people are saying we should shade their lib and distribute that, then I guess we don't need a non-shaded JAR.

Maven doesn't have to use jar jar, though it's possible. Can we shade it to "com.esotericsoftware.asm"?

Is it a problem for Maven that I use a shaded ASM JAR as a dependency? That means the ReflectASM source is using shaded imports.

@magro
Copy link
Collaborator

magro commented Jul 17, 2017

Can we shade it to "com.esotericsoftware.asm"?

Yes we can. In the PR (#57) I left it unchanged to keep changes as small as possible, but I don't think that changing to "com.esotericsoftware.asm" would break anything (assuming nobody uses the shaded classes of course). What's your motivation btw?

Is it a problem for Maven that I use a shaded ASM JAR as a dependency? That means the ReflectASM source is using shaded imports.

That's the problem, because maven doesn't use the patched asm jar, but the original one as declared as dependency. Therefore compilation fails.

@NathanSweet
Copy link
Member

The long package name was just very long. :)

I'm fine with shading it after building the ReflectASM JAR. Next time I want to do a release for GitHub I'll probably make scar/jarjar do it.

@magro
Copy link
Collaborator

magro commented Jul 17, 2017

Next time I want to do a release for GitHub I'll probably make scar/jarjar do it.

So you're not going to use mvn package for this? Shall we set up the maven/sonatype push-to-maven-central stuff for you? It's not that much...

@He-Pin
Copy link

He-Pin commented Sep 6, 2017

Caused by: java.lang.NoSuchMethodError: org.objectweb.asm.ClassWriter.<init>(I)V
  at com.esotericsoftware.reflectasm.MethodAccess.get(MethodAccess.java:120)
  at com.taobao.xgraphql.annotation.AnnotationToAST.getGraphQLObjectType(AnnotationToAST.java:451)

the packed version is not released,so ....

@He-Pin
Copy link

He-Pin commented Sep 11, 2017

I have done a shade in my local code:)

@NathanSweet
Copy link
Member

So you're not going to use mvn package for this?

@magro Since I'm not a Maven user, doing releases that way is asking for trouble. If it's OK with you, you can be the Maven master.

Does anything more need to be done for this isssue? Currently the ReflectASM source is using an ASM JAR repacked to com.esotericsoftware.asm. We need a new 1.11.6 Maven release (#60), so once that is done I think we can close this.

@NathanSweet
Copy link
Member

Somewhat OT for RefelctASM but, @magro since ReflectASM uses a repacked ASM, maybe we don't need to provided a shaded Kryo any more? If we do, the use case would not be about ASM, but that you had two libraries that use different versions of Kryo. In that case we would want to shade the entire Kryo lib and its dependencies.

@magro
Copy link
Collaborator

magro commented Jun 11, 2018

@NathanSweet Your commit ade0542 broke the mvn build, i.e. right now it's not possible anymore for me to publish the library to maven central. I don't have the time to check out other ways of building / publishing. If you want me to publish this thing to maven central so that kryo can use a new version I see nothing else than reverting the mentioned commit and merge #57 instead (of course there we can change the package name to com.esotericsoftware.asm).

@NathanSweet
Copy link
Member

Yep, reverting and doing the repack with Maven is fine.

magro added a commit that referenced this issue Jun 12, 2018
magro added a commit that referenced this issue Jun 12, 2018
Also remove the reference to the shaded jar, refs #54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants