fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo #73

famod · 2020-01-03T20:05:31Z

Short description of what this resolves:

Introduces an alternate arquillian-jacoco-with-asm jarfile that includes "private" ASM and JaCoCo packages that do not clash with (older) server-side ASM.

~~ℹ️ This is currently in WIP/draft state. I will add some code remarks myself. Feedback very much appreciated!~~

Changes proposed in this pull request:

turn project into multi-module project, retaining arquillian-jacoco as is to avoid regressions for users who don't want/need to use the new with-asm alternative.
introduce arquillian-jacoco-with-asm module that takes arquillian-jacoco and "relocates" all ASM (and thus also JaCoCo) packages to org.jboss.arquillian.extension.jacoco.org.[...] via maven-shade-plugin
small offtopic improvement: skip extraction of test JBoss when -Dmaven.test.skip[=true]

Fixes: #72 and probably #25

famod · 2020-01-03T20:22:18Z

Note: I am going to leave regular comments for topics that are not suitable to be added to certain code lines.

famod · 2020-01-03T20:22:32Z

Q: First, why the multi-module approach?
A: maven-shade-plugin (let's call it MSP as I will have to repeat that often here) does not really support provided scope dependencies (MSHADE-181) but jacoco-core has exactly that scope (which I didn't question).
So just calling MSP in the previous "single-module" project wouldn't have worked (without touching the scope).
Furthermore, even though you can produce a secondary attached shaded artifact with MSP, you can only have one POM. That POM would still declare all dependencies, although jacoco-core would already be "included" in the secondary artifact. I think that would confuse the users.

famod · 2020-01-03T20:27:59Z

Q: Why are all non-test dependencies defined in the parent POM?
A: Since provided dependencies are not inherited and since I wanted MSP to retain all the non-shaded dependencies in the "dependency reduced POM" I had to declare them in the parent in order to avoid repeating them in with-asm.

famod · 2020-01-03T21:11:00Z

One last thing for now: I would like to extend README.asciidoc describing this new with-asm flavor.
Is this ok or should such documentation go elsewhere?

famod · 2020-01-10T18:22:40Z

@bartoszmajsak Feedback is very much appreciated!

bartoszmajsak

Thanks a lot for this work! I think Q&A comments should be included in the README, as you suggested - that's a very good idea.

Can you list pending work that keeps this PR in a draft state?

bartoszmajsak · 2020-01-16T09:25:07Z

pom.xml

@@ -12,9 +12,10 @@
  <modelVersion>4.0.0</modelVersion>

  <groupId>org.jboss.arquillian.extension</groupId>
-  <artifactId>arquillian-jacoco</artifactId>
+  <artifactId>arquillian-jacoco-parent</artifactId>


Maybe we could also bump jboss-parent in that PR?

famod · 2020-01-16T22:21:51Z

Can you list pending work that keeps this PR in a draft state?

Sure. Main points:

maven-shade-plugin 3.2.2 has not yet been released. I will try to "ping" the maven-dev list again.
Unclear testing approach. Extracting a common test module which can be used for both flavours (different JaCoCo and ASM packages!) seems rather complex.
The simple solution would be to ignore this for now and add a full-fledged testing approach for with-asm later, whenever this will be.
Decide what to to with those MANIFEST.MF files. We might want to simply skip them all and just include our own (generated one?).

The rest (Readme & removal of module-info) I will add in the next few days, tomorrow hopefully.

famod · 2020-01-22T22:46:51Z

I resolved all of the remaining points except MSP 3.2.2 (still waiting for that) and the testing challenge (skip for now?).

famod · 2020-01-23T20:09:42Z

I am a little tired of waiting for maven-shade-plugin 3.2.2 to be relased so I switched to a specific snapshot version that includes MSHADE-311.
I am not sure whether this is acceptable for a limited amount of time. I'd create another PR as soon as 3.2.2 is released.

As for the testing challenge: I simply don't have the time right now to look into that. IMHO this is something that can be added later.

bartoszmajsak · 2020-01-24T05:24:58Z

Thanks a lot, @famod. Can we ping someone wrt to the release timeline for maven-shade-plugin. Personally I'm totally fine using SNAPSHOT myself, but I can imagine this can scare some people off :)

famod · 2020-01-24T12:51:29Z

Can we ping someone wrt to the release timeline for maven-shade-plugin

I tried twice: https://lists.apache.org/x/thread.html/2258bb598c3b173ef5228776aa1e3021c4dc323751f328e897b1aa50@%3Cdev.maven.apache.org%3E

Both times someone answered the call and something was committed, but a release is yet to been seen.

Personally I'm totally fine using SNAPSHOT myself, but I can imagine this can scare some people off :)

It's only used internally, not "propagated" as a dependency, so...

bartoszmajsak · 2020-01-24T16:32:51Z

It's only used internally, not "propagated" as a dependency, so...

Sure, but it is also a moving target :) Potentially can break everything with one innocent change on their side. I will join the thread to see if we can get some traction.

famod · 2020-01-24T16:39:50Z

Potentially can break everything with one innocent change on their side.

Since it is a concrete snapshot version, only deletion of that version from the snapshot repo would be a problem.
But right, that can also happen without a warning.

famod · 2020-02-09T17:36:58Z

FYI, voting for releasing MSP 3.2.2 has begun: https://lists.apache.org/thread.html/r86a13a941eacf9580fbeef414efc1b5622c4af9d4cf715e5d8fcdd96%40%3Cdev.maven.apache.org%3E

…JaCoCo (WIP)

famod · 2020-02-12T21:13:22Z

@bartoszmajsak I replaced MSP 3.2.2-SNAPSHOT with 3.2.2 final that was released today.

This PR should now be ready to merge!

bartoszmajsak

Thank you for all this work @famod. Outstanding quality!

famod changed the title ~~fix(issue #72): introduce *-with-asm jar to shades ASM and JaCoCo (WIP)~~ fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo (WIP) Jan 3, 2020

famod mentioned this pull request Jan 3, 2020

build(deps): bump version.jacoco from 0.8.3 to 0.8.5 #70

Merged

bartoszmajsak reviewed Jan 16, 2020

View reviewed changes

famod force-pushed the issue-72-shade-asm branch from 68cb90c to 4261f3e Compare January 22, 2020 22:45

famod mentioned this pull request Jan 22, 2020

Please add more info to the README #24

Closed

famod force-pushed the issue-72-shade-asm branch from 4261f3e to 93828cc Compare January 22, 2020 23:07

famod marked this pull request as ready for review January 23, 2020 20:09

famod changed the title ~~fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo (WIP)~~ fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo Jan 23, 2020

fix(issue arquillian#72): introduce *-with-asm jar to shades ASM and …

16981d4

…JaCoCo (WIP)

famod force-pushed the issue-72-shade-asm branch from 9a6af44 to 16981d4 Compare February 12, 2020 21:06

bartoszmajsak approved these changes Feb 18, 2020

View reviewed changes

bartoszmajsak merged commit c9c1946 into arquillian:master Feb 18, 2020

famod deleted the issue-72-shade-asm branch February 18, 2020 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo #73

fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo #73

famod commented Jan 3, 2020 •

edited

Loading

famod commented Jan 3, 2020

famod commented Jan 3, 2020

famod commented Jan 3, 2020

famod commented Jan 3, 2020 •

edited

Loading

famod commented Jan 10, 2020

bartoszmajsak left a comment

bartoszmajsak Jan 16, 2020

famod commented Jan 16, 2020

famod commented Jan 22, 2020

famod commented Jan 23, 2020

bartoszmajsak commented Jan 24, 2020

famod commented Jan 24, 2020

bartoszmajsak commented Jan 24, 2020

famod commented Jan 24, 2020

famod commented Feb 9, 2020

famod commented Feb 12, 2020

bartoszmajsak left a comment

fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo #73

fix(issue #72): introduce *-with-asm jar which shades ASM and JaCoCo #73

Conversation

famod commented Jan 3, 2020 • edited Loading

Short description of what this resolves:

Changes proposed in this pull request:

famod commented Jan 3, 2020

famod commented Jan 3, 2020

famod commented Jan 3, 2020

famod commented Jan 3, 2020 • edited Loading

famod commented Jan 10, 2020

bartoszmajsak left a comment

Choose a reason for hiding this comment

bartoszmajsak Jan 16, 2020

Choose a reason for hiding this comment

famod commented Jan 16, 2020

famod commented Jan 22, 2020

famod commented Jan 23, 2020

bartoszmajsak commented Jan 24, 2020

famod commented Jan 24, 2020

bartoszmajsak commented Jan 24, 2020

famod commented Jan 24, 2020

famod commented Feb 9, 2020

famod commented Feb 12, 2020

bartoszmajsak left a comment

Choose a reason for hiding this comment

famod commented Jan 3, 2020 •

edited

Loading

famod commented Jan 3, 2020 •

edited

Loading