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

Create one large uber jar containing ALL EEAs from subprojects #57

Merged
merged 4 commits into from
Nov 23, 2017

Conversation

triller-telekom
Copy link
Contributor

Addresses #51

Signed-off-by: Stefan Triller stefan.triller@telekom.de

@triller-telekom
Copy link
Contributor Author

With my little maven knowledge I gave it a try to create one large uber.jar.

The libraries folder is not a project itself as it creates the target folder containing eea-parent-1.0.0-SNAPSHOT.jar

Unfortunately I do not know if that is the correct way to do it, also regarding the version 1.0. I just know that the jar contains all EEAs :) Please feel free to comment/change/discard my efforts :)

Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOK. (BTW @ctron you're quite the Maven wizard AFAIK, perhaps you want to chime in here?)

<assembly xmlns="http://maven.apache.org/ASSEMBLY/2.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/ASSEMBLY/2.0.0 http://maven.apache.org/xsd/assembly-2.0.0.xsd">
<!-- <id></id>--> <!--This id would be appended to our jar name, let's set it to empty-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm.. but you've commented it out, so it won't set it to empty, no? 😺 Did you perhaps just want to remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed it.

<fileSet>
<excludes>
<exclude>target/**</exclude>
<exclude>pom.xml</exclude>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we exclude a few more things here, please:

  • eea-for-gav: This one is quite important - did you notice that, of course, the new JAR will keep overwriting the eea-for-gav from each module, and so contain an arbitrary one, proably based on something funky like inode iteration order from the OS? That will be confusing... the truth is that this new artifact is NOT for a particular single GAV anymore, so to avoid confusion to both humans and the eclipse-external-annotations-m2e-plugin, should it ever get to see such a JAR, IMHO it would be best if this JAR simply did not contain an eea-for-gav marker file at all. Maybe later the eclipse-external-annotations-m2e-plugin could be made smarter to be able to read several lines of GAVs, and this thing here could be merge the eea-for-gav files from each library... but you don't need that today, do you, because you are producing this with the intention of consuming it without the eclipse-external-annotations-m2e-plugin anyway - so let's just exclude it!

  • .project, .classpath, .settings/ from Eclipse currently are, but should never be, in the JAR produced by this, IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed today that the eea-for-gav is important. Still learning... But you are right, excluding it for now is probably the easiest way since we do not consume it anyway for now. I have done that.

@@ -81,6 +81,25 @@
</resources>
<plugins>
<plugin>
<!--Create one large uber jar containing ALL EEAs from sub projects-->
<artifactId>maven-assembly-plugin</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting... I have 2 issues with this, one very small and one I think is a bigger real problem:

  1. the resulting artifact from doing it like this will be called eea-parent-1.0.0-SNAPSHOT.jar. That's confusing, it shold be something like eea-all*.jar. That could be easy to change by changing the artifact ID of this POM, and aligning its version to be 0.0.1-SNAPSHOT instead of 1.0.0-SNAPSHOT, like everything else - but before you do both, read on, I think there is a bigger problem... 😄

  2. the JAR resulting from this which I find in my ~/.m2/repository after an mvn clean install with this change does not contain the usual /META-INF/maven/**/pom.xml & pom.properties... probably because maven-assembly-plugin really is more of a "create me ZIP" than "this is a new Maven artifact" kind of thing? If everything works without that META-INF/maven then I'm fine with this - but does it, have you actually tested a dependency to this new artifact? I've never seen a JAR consumed by Maven without that metadata. Did a (very) quick local test, and could NOT get a dependency to this artifact working at first (...not found...). Then figured you would need to <type>pom .. that finds it, but then it's not on the classpath (which is normal, because it's a POM not a JAR type).

Bottom line: I'm not convinced yet that as-is this actually works... sorry, but fair enough? Perhaps by introducing a separate new artifact libraries/all instead of choking this on eea-parent its easier? Just a thought.

Also, somehow I would feel much more comfortable with this if we could "self test" it, within this project. I was going to suggest using it as the dependency in the examples/maven/parent/pom.xml instead of jdk-eea and slf4j-api-eea, but that of course would break eclipse-external-annotations-m2e-plugin (unless you/someone is motivated to do the thing from above).

But how about you try to use this as the dependency in the examples/tycho from #54 (which we can get in first, before this one) instead of only the org.lastnpe.eea:jdk-eea? Because that example (probably?) doesn't work with eclipse-external-annotations-m2e-plugin (I've no idea how well M2E likes the tycho-compiler-plugin), so good fit there? (And not a coincidence, as this both comes from you.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the bigger problem you have with the current solution. As I have mentioned before I am certainly not a maven expert :) So maybe since you mentioned @ctron he could help us out here?

So I would like to wait if we get a hint from him before I try some of the possible options you pointed out. Thank you for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created an extra artifact eea-all now with the maven shade plugin suggested by @ctron. Thank you for pointing this out.

Also I learned that maven artifacts are not just plain zip files :)

Once #54 is merged I can add a dependency from it to this bundled artifact.

The only larger problem left is that we totally ignore versions, but I think for the uber jar it would be hard to integrate that anyway...

@ctron
Copy link
Member

ctron commented Nov 15, 2017

We might be able to re-use the maven-shade-plugin for that: https://maven.apache.org/plugins/maven-shade-plugin/

Copy link
Member

@sylvainlaurent sylvainlaurent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add a .gitignore to ignore dependency-reduced-pom.xml

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom
Copy link
Contributor Author

I have created the gitignore file to ignore the auto-generated dependency-reduced-pom.xml file and rebased the branch against the current master.

Also this PR is now using the maven-shade-plugin as suggested by @ctron.

@@ -0,0 +1 @@
/dependency-reduced-pom.xml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the root .gitignore - individual .gitignore in sub-folders can be very confusing.. that's a minor point though, and if otherwise this works I'll merge it and just fix that up afterwards.

@vorburger vorburger self-assigned this Nov 23, 2017
Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pulled this locally and it seems to work great - thanks to @ctron for pointing @triller-telekom to the maven-shade-plugin - lets do this! 🥇

@vorburger vorburger merged commit 17a08fb into lastnpe:master Nov 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants