asm 4.0 dependency should not be present

[ghost]

From [email protected] on June 25, 2013 23:20:42

Hi,

Using Kryo in my project, I see that it still has a dependency on asm 4.0 even though it appropriately uses a dependency on a shaded version of reflectasm (that, I guess, was intended to remove that dependency on asm 4.0...)

---

[INFO] The following files have been resolved:
[INFO] com.esotericsoftware.kryo:kryo:jar:2.21:compile
[INFO] com.esotericsoftware.minlog:minlog:jar:1.2:compile
[INFO] com.esotericsoftware.reflectasm:reflectasm:jar:shaded:1.07:compile
[INFO] org.objenesis:objenesis:jar:1.2:compile

[INFO] org.ow2.asm:asm:jar:4.0:compile

It looks like the published pom of the shaded reflectasm is incorrect (as it still contains a dependency to asm 4.0, that has normaly been shaded and relocated).
It should be replaced by a "dependency-reduced" pom (without any dependency to asm) (http://maven.apache.org/plugins/maven-shade-plugin/shade-mojo.html#createDependencyReducedPom)

Kind regards,
Vincent

Original issue: http://code.google.com/p/kryo/issues/detail?id=116

[ghost]

From romixlev on July 23, 2013 02:31:58

Yes. This could be a problem.

@nate: Could you fix it in reflectasm project POM file?
I thin that just adding the following line in the configuration section of the maven-shade-plugin should fix the issue:
true

I'll do the same for the kryo pom.xml

[ghost]

From romixlev on July 23, 2013 06:25:49

In fact, it is not as easy as I thought. When I look into a content of shaded jars that we produce currently, I'm a bit puzzled. We relocate our dependencies, but for some reason all the dependency JARs are also included into our shaded jar. Why does it happen? It does not make too much sense for me. Also the pom.xml files do not seem to be affected by the createDependencyReducedPom option.

I'm not a big expert in maven-shade-plugin, but may be Martin can help out here?

-Leo

Cc: martin.grotzke

[ghost]

From [email protected] on July 23, 2013 09:09:18

Hi,

I did some research myself and indeed, it is not that simple. :-)
createDependencyReducedPom only works if shadedArtifactAttached is set to false (i.e the only pom deployed is the dependency reduced one).

According to me, a same groupId/artifactId has one and only one pom. You can't have two poms for same groupId/artifactId even if classifier differs.

The only solution I see is to have a dedicated artifact id (kryo-shaded ?) and therefore a dedicated pom, that uses the shade plugin with shadedArtifactAttached=false.

Kind regards,
Vincent

[ghost]

From martin.grotzke on July 23, 2013 14:49:21

Thanks @vincent for the investigation!
I'm currently on vacation and can have a look at this / fix it next week.

[ghost]

From romixlev on August 07, 2013 02:38:39

@martin: This is a small ping! :-) It would be nice if you could look into this issue.

[ghost]

From martin.grotzke on August 07, 2013 13:31:15

Thanx for the ping (got too deep in my stack, sorry), I'll try to look into this issue this week.

[ghost]

From martin.grotzke on August 11, 2013 15:44:06

I see 3 options:

1. Leave things as they are, but improve the documentation to show which excludes have to be used with the shaded jar
2. Create a dedicated artifact like "kryo-shaded" with a reduced pom (as suggested by Vincent)
3. Only provide the shaded jar/artifact (make the shaded jar the main artifact)

Option 3) is my preference, because it's the simplest solution for users, and because the shaded jar only adds ~10k to the normal jar (420k instead of 410k).

What do you think?

[ghost]

From martin.grotzke on September 30, 2013 14:42:18

We're now using option 3), as release with kryo-2.22.

Additionally I excluded asm from the shaded reflectasm dependency.

Status: Fixed
Labels: Milestone-2.22

[ghost]

From [email protected] on October 09, 2013 04:34:38

Option 3 is an excellent option.

Thanks a lot for the fix.

Kr,
Vincent

[ghost]

From martin.grotzke on October 09, 2013 07:09:08

You're welcome, and thank you for the feedback/confirmation! :-)

[romix]

@magro @NathanSweet Martin, actually, I'm not quite happy with the fix/decision you did for 2.22. And I guess many users who were using Objenesis with Kryo are also not quite happy, because it breaks binary compatibility. The package names for objenesis classes are now different due to shading and it means that everyone (!) who was using it (e.g. as an InstantiationStrategy) has to change the source code and use the new package names containing "shaded" in their names.

Do you see any possibility for a less intrusive workaround to avoid changes at the source code level?

And BTW as Nate mentioned already, it is not quite obvious why the original dependency JARs are still inside the shaded JAR? What is the purpose of having them there?

[magro]

@magro @NathanSweet Martin, actually, I'm not quite happy with the fix/decision you did for 2.22.

@romix First of all: I asked in the related issue and on the mailing list which of some options we should choose: https://groups.google.com/forum/#!searchin/kryo-users/shaded/kryo-users/cGR4-PZIHOg/MCoMZfdLh7QJ
There was only a single feedback, even after asking again. You and every one else could have thrown in a different opinion at that time!

I'll respond later to the actual question, need to come down and get back to work.

[NathanSweet]

@magro Sorry, I should have been following the thread more closely. :( If we are going to shade by default, it should only be classes that are only used internally. Since it is expected Kryo users use the Objenesis API, it shouldn't be shaded by default. Shading ASM by default is fine I guess, since it doesn't impact users. I tend to think no shading should be done by default, but I understand a lot of users are bothered by it, so doing whatever you need to keep those bastards from bothering us works for me. :)

Could do the ASM shading in the ReflectASM Maven JAR, whatever is easiest for you.

[romix]

@magro Martin, I'm really sorry for my tone! It was really not meant as a personal rant, even if you had this impression. I think everyone including me totally appreciates what you do for the project.

And you are totally right that all of us (not you personally) overlooked this binary compatibility breakage. On my side, at that time I quickly looked at the JAR and noticed that those original JARs are still there. But those original JARs are not the big deal, IMHO, even though it is not clear why shader includes them. What was really overlooked is that objenesis APIs became binary incompatible and require now changes in the client's source code. And this is really a bit of pain, because 2.22 is out now ...

I'm wondering how we could protect ourselves from similar binary compatibility changes in the future?

Chill guys proposed on the mailing list one project/tool that could used for automatic checking of binary compatibility. May be we could look into that one.

Another idea would be to have a separate satellite project which uses Kryo as a dependency and thus it models a typical client's project. In this project we could write a few unit tests that explicitly refer to classes from our dependencies like MinLog and objenesis, etc. If we later somehow manage to break the package names or API by shading, then those tests would not even compile or would fail at runtime. And this way we could understand that we broke the API.

[NathanSweet]

Could compile the tests in their own JAR and then run them against future versions (without compiling a new JAR of course).

[magro]

@romix No problem.

Re Shading (objenesis)

I wasn't really aware about the difference of asm and objenesis: that objenesis is somehow part of the kryo api and that it's not a good idea to move it to a different package. So I agree that we should depend on objenesis and not shade+relocate it.

What do you think about shading and releasing only the shaded jar in general? My preference is to continue to ship the shaded jar (includes minlog and reflectasm:shaded coming with relocated asm) as the default instead of providing the shaded jar as alternative. But I remember that there were different opinions on the mailing list, so maybe we should discuss it there again?

Why are jars included in the shaded jar?

That the jar additionally includes other jars (e.g. minlog) is caused by the configured osgi bundling (mvn-bundle-plugin), so it's expected I'd say (even if I don't know why these jars should be needed). It should be possible to get rid of this, not sure what this means for osgi.

Re Compatibility issues

I also thought about a separate project that checks compatibility issues between different versions and the current SNAPSHOT with a previous version. Slightly related: I'd also like to have a test for osgi compatibility, this would be the same just running within an osgi container. I also find the semver based project mentioned by Oscar interesting. I'd like to take a look at this, although I cannot promise when I'll find time for this. I just submitted an issue (#150) for this.

What do you think in general regarding compatibility issues? I see those options:

1. Keep 2.22 api as it is (apart from objenesis issue of course)
2. Get back to the api of 2.21 (or maybe even a version before?), e.g. s.th. that's compatible with 2.17 as used in some bigger project. If necessary we could then start a 3.x line that includes changes from 2.22
3. Provide a compatibility jar that allows to upgrade from e.g. 2.17

Anything else? Preferences? Should we start a thread on the mailing list for this?

[NathanSweet]

On Thu, Nov 14, 2013 at 2:39 PM, Martin Grotzke [email protected]:

@romix https://github.com/romix No problem.

Re Shading (objenesis)

I wasn't really aware about the difference of asm and objenesis: that
objenesis is somehow part of the kryo api and that it's not a good idea to
move it to a different package. So I agree that we should depend on
objenesis and not shade+relocate it.

What do you think about shading and releasing only the shaded jar in
general? My preference is to continue to ship the shaded jar (includes
minlog and reflectasm:shaded coming with relocated asm) as the default
instead of providing the shaded jar as alternative. But I remember that
there were different opinions on the mailing list, so maybe we should
discuss it there again?

Users might want to use the MinLog API, it isn't just used internally, so
it shouldn't be shaded.

I think shading is invasion in general and shouldn't be done by default. If
there are really so many users that have problems with ASM then I guess we
can shade ASM, but not other libs.

Why are jars included in the shaded jar?

That the jar additionally includes other jars (e.g. minlog) is caused by
the configured osgi bundling (mvn-bundle-plugin), so it's expected I'd say
(even if I don't know why these jars should be needed). It should be
possible to get rid of this, not sure what this means for osgi.

Weird. And lame. :)

Re Compatibility issues

I also thought about a separate project that checks compatibility issues
between different versions and the current SNAPSHOT with a previous
version. Slightly related: I'd also like to have a test for osgi
compatibility, this would be the same just running within an osgi
container. I also find the semver based project mentioned by Oscar
interesting. I'd like to take a look at this, although I cannot promise
when I'll find time for this. I just submitted an issue (#150#150)
for this.

What do you think in general regarding compatibility issues? I see those
options:

1. Keep 2.22 api as it is (apart from objenesis issue of course)
2. Get back to the api of 2.21 (or maybe even a version before?), e.g.
   s.th. that's compatible with 2.17 as used in some bigger project. If
   necessary we could then start a 3.x line that includes changes from 2.22
3. Provide a compatibility jar that allows to upgrade from e.g. 2.17

Sounds good. It would be great to have an upgrade path!