Convert cglib to be a maven project

[ghost]

Initial basis to convert cglib to be a maven project, I suspect there would be further work needed to ensure this work correctly but hopefully helps.

• Two tests were disabled for now since they relied on a signed jar file.
• Maven needs to be updated to generate sources/javadoc jar files

[sameb]

This looks like it's using 'shade' instead of 'jarjar' to embed asm in the nodeps build. Any idea how they differ? It'd be nice if we could keep using jarjar. The Guice poms have an example of using jarjar (core pom: https://github.com/google/guice/blob/master/core/pom.xml, parent pom: https://github.com/google/guice/blob/master/pom.xml)

Also, out of curiosity, what did you do to manage to make it work on jdk8/jdk7? (The current builds fail with VerifyErrors relating to stackmaps.)

[guw]

Can you work on a solution that does not disable the jar signing? You might want to use the Maven Ant task.

The signing test is critical. It was an issue I fixed recently.

[ghost]

@sameb
-XX:-UseSplitVerifier is the command line option to resolve the issue with VerifyError stackmaps. There is a bug report about it here: https://bugs.openjdk.java.net/browse/JDK-8051012. More information here: http://www.infoq.com/news/2014/08/Java8-U11-Broke-Tools

I don't really have any experience with the shade plugin, I just reused the pull request submitted a few weeks ago. I suspect we should be able to reuse the jarjar plugin instead, let me get back to you on this.

@guw
Sure, I'll have a look and see what I can do.

[ghost]

Okay, I've converted to use jarjar and introduced a project to run integration tests such as the code signing issue. Currently it runs with failsafe but I think it could use the surefire plugin, shouldn't matter too much either way.

I've now only one test case that has been disabled in TestEnhancer (https://github.com/JBodkin/cglib/commits/20476c6f7fba45fbcaa35f50fc39b4f6afca2a8c/cglib/src/test/java/net/sf/cglib/proxy/TestEnhancer.java)

[sameb]

Exciting, thanks @JBodkin! A few more questions:

• What's wrong with the TestEnhancer tests that they need to be disabled?
• If we're using -XX:-UseSplitVerifier to make jdk7/jdk8 tests pass, does that mean if we build/deploy from jdk7/jdk8 that end-users will also need to use -XX:-UseSplitVerifier to prevent cglib from failing? If so, I'm not too keen on that and would prefer if there was some other route.

[ghost]

There's just one small test, I believe it can be moved to an integration test.

public void testSamples() throws Throwable{
    samples.Trace.main(new String[]{});
    samples.Beans.main(new String[]{});
}

I'll have to investigate to see where the VerifyErrors are happening, this may require a code change though. There are a fair few open source projects that are affected by this though.

[ghost]

Okay, I've moved the disabled test out into an integration test so it is now passing.

[sameb]

Perfect. Maybe for the time being we leave jdk7/jdk8 failing (w/o using -XX:-UseSplitVerifier, so we don't get a potentially false sense of security), and just do a change to maven structure? I'll look over the PR in some more detail and leave scattered comments where I have more questions.

[ghost]

Interesting, it seems the build works without that option.

[sameb]

Very interesting! @guw, any idea what's up here?

[guw]

I haven't checked if the tests are executed. Any chance it's a newer asm version?

[sameb]

Not a newer asm version AFAICT, lib has 5.0.3, and the logs (https://s3.amazonaws.com/archive.travis-ci.org/jobs/37534265/log.txt) show 5.0.3 being downloaded in the mvn tests. It looks like the logs also show the tests are being run.

[ghost]

Perhaps the ant build process passes in a optimisation flag that doesn't happen here in the maven version?

[sameb]

I'd be willing to believe that the maven build is just smarter ... maybe setting the --source & --target in a better way? Might as well investigate a little first though.

[guw]

So the Java version is different. It’s now “1.7.0_65”.

Also, I believe Maven executes the class files directly, i.e. everything loaded and running in the same reactor build. The Ant test runs tests based on the jars. This could cause differences in class domain at runtime.

[sameb]

ant builds are still failing with the latest java version too (
https://s3.amazonaws.com/archive.travis-ci.org/jobs/37504078/log.txt). So
java version isn't the difference here.

According to
http://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html,
maven will fork once by default for the JUnit tests and reuse that fork for
subsequent tests. Looks like ant had the same setting.

One potential difference is that javac ant ran with source=1.2, target=1.2 (
https://github.com/cglib/cglib/blob/master/build.xml#L31) whereas with mvn
we're setting a "java.version.source" and "java.target.source" property to
1.5 & using that in the compiler setting. I wonder if using 1.2 would make
it break again? (In which case, I'm happy to up it to 1.5.)

On Thu, Oct 9, 2014 at 3:54 PM, Gunnar Wagenknecht <notifications@github.com

wrote:

So the Java version is different. It’s now “1.7.0_65”.

Also, I believe Maven executes the class files directly, i.e. everything
loaded and running in the same reactor build. The Ant test runs tests based
on the jars. This could cause differences in class domain at runtime.

-Gunnar

Am 09.10.2014 um 12:48 schrieb Sam Berlin notifications@github.com:

I'd be willing to believe that the maven build is just smarter ... maybe
setting the --source & --target in a better way? Might as well investigate
a little first though.

—
Reply to this email directly or view it on GitHub.

Gunnar Wagenknecht
gunnar@wagenknecht.org

—
Reply to this email directly or view it on GitHub
#23 (comment).

[ghost]

I believe the surefire plugin that executes the unit tests only runs with java 1.5 minimum, so I won't be able to decrease the version in this further. http://jira.codehaus.org/browse/SUREFIRE/fixforversion/19174

[sameb]

The difference is that maven the target/source properties for the compiler apply to src & test, whereas in ant the properties are just set on the src. If you add target="${compile.target} to the 'build-test' ant target (and change it from 1.2 to 1.5, or 1.6 works too), ant will began to pass.

This is good news & bad news. The bad news is that it seems like the target of the code using cglib influences how the JVM will interpret the generated bytecode, expecting stack maps or not. The good news is it is how it works today, so anyone using cglib already has this problem. So I don't think there's any new false sense of security here, though it's something that merits investigation if anyone has time to look into it later.

So, I think the concept of the PR is sound and just needs a review of the nitty gritty inside it.

[ghost]

Great, that's good to hear.

If we decide to switch over to this method, we could utilize TravisCI more in order to be able to perform releases. A contributor to the repository could use the maven-release-plugin to push a tag to github, when TravisCI runs, it picks up this tag and creates a new release in GitHub release, at the same time it can be pushed out to the central repository. Hopefully this would help to streamline the release process.
(This is separate to this pull request though). It would help with #20

[sameb]

if this has a dependency on cglib, does that mean that asm will still be downloaded?

(specifically: does it mean that people who use cglib-nodeps will be downloading asm. we'd want to avoid that.)

[ghost]

Changed cglib dependency to be optional

[sameb]

should this be 'provided' scope (or something else?) so that people using cglib don't depend on ant transitively?

[ghost]

Changed ant to be an optional dependency

[sameb]

left a few comments. please excuse my overall ignorance of maven -- my comments are largely from cargo-culting guice's POM and a limited understanding of it all.

[sameb]

thinking through this, i'm not sure this is the best way to expose the nodep jar to maven (as a separate artifact). both the normal & nodep artifacts produce the same classnames, except one delegates to real ASM & the other delegates to an embedded ASM. i think a better way to do this would be to use a classifier on within the cglib pom. for example, Guice does this with the no_aop build:https://github.com/google/guice/blob/master/core/pom.xml#L189 (though in this case we'd probably do a mixture of the jarjar / no_aop step.. having an execution phase do jarjar + packaging with a different classifier). we probably want to override the deps also and mark them as compile-only too in the nodep classifier.

then folks'd use it with <classifier>nodep</classifier>.

[ghost]

The jarjar plugin does not support classifiers, therefore it fails to build the artifact when you attempt to use one. Ticket: sonatype/jarjar-maven-plugin/issues/6

Guice automatically embeds cglib and asm by default, you can verify this here: https://repo1.maven.org/maven2/com/google/inject/guice/3.0/

[sameb]

Fair enough, thanks for checking it out!

[sameb]

LGTM except for the question about where/how to do the nodep variant. LMK what you think.

[ghost]

All done

[sameb]

Thanks so much for doing this!