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

Fix shaded jar packaging #12589

Merged
merged 5 commits into from Aug 3, 2015
Merged

Fix shaded jar packaging #12589

merged 5 commits into from Aug 3, 2015

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Aug 1, 2015

There are a couple of problems:

  • shaded jar is completely untested <-- most important
  • has the wrong dependencies
  • has duplicated dependencies for what it shades (no reduced POM)
  • possibly doesn't include all the stuff it should

Basically this isn't going to work.

This moves shading to distributions/shaded, so it gets a proper POM. and the plan is to add tests for what you are supposed to be able to do with it (e.g. node client, whatever). At the least we can jar hell check it for sanity and other simple tests.

Additionally dependencies for packaging are quite crazy, to add a jar you have to do it in a zillion places... we should try to reduce these (also work in progress).

I hacked a way at this a bit to get towards something manageable. Dependencies of the shaded jar currently look like this (i didnt change the current logic yet):

[INFO] test:testme:jar:1.0-SNAPSHOT
[INFO] \- org.elasticsearch.distribution:elasticsearch-shaded:jar:2.0.0-beta1-SNAPSHOT:compile
[INFO]    +- org.apache.lucene:lucene-core:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-backward-codecs:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-analyzers-common:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-queries:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-memory:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-highlighter:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-queryparser:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-sandbox:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-suggest:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-misc:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-join:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-grouping:jar:5.2.1:compile
[INFO]    +- org.apache.lucene:lucene-spatial:jar:5.2.1:compile
[INFO]    +- com.spatial4j:spatial4j:jar:0.4.1:compile
[INFO]    +- org.yaml:snakeyaml:jar:1.12:compile
[INFO]    \- org.hdrhistogram:HdrHistogram:jar:2.1.6:compile

Seem some jars already got forgotten to be added to the shading configuration, and maybe we really don't need all these jars? So we need to figure out what tests to add and then we can adjust this as necessary.

@rmuir
Copy link
Contributor Author

rmuir commented Aug 1, 2015

I am also ok to name this distribution/client instead of shaded, since thats really what this thing is. Just seems like it would give us a path forwards to do this stuff in a cleaner way: shading is really sketchy.

@dadoonet
Copy link
Member

dadoonet commented Aug 1, 2015

I think that moving shaded to its module is the way to go IMO.

You added "elasticsearch-fully-loaded" module. I"m not sure yet what is the goal for this. Is this needed for this change (moving shaded to a distribution module)?

@rmuir
Copy link
Contributor Author

rmuir commented Aug 1, 2015

You added "elasticsearch-fully-loaded" module. I"m not sure yet what is the goal for this. Is this needed for this change (moving shaded to a distribution module)?

Most packaging distributions package up the optional dependencies (except slf4j). This needed to be pulled out of distribution/pom.xml so that the shaded distribution can work without having to explicitly disable each and every one of them. It also makes it easier to start removing these duplicate lists of our dependencies (see where i did that in the assembly.xml).

Its just a POM we use internally, I dont think it hurts anything.

@dadoonet
Copy link
Member

dadoonet commented Aug 1, 2015

Thank you for the explanation.

Its just a POM we use internally, I dont think it hurts anything.

Agreed.

On the other end, we should try to understand if there is really a need for a shaded jar and in which context.
So I'm +1 for creating this shaded module with your PR and then decide if we really want to keep it around.

@rjernst
Copy link
Member

rjernst commented Aug 1, 2015

I am also ok to name this distribution/client instead of shaded, since thats really what this thing is

+1

@rmuir
Copy link
Contributor Author

rmuir commented Aug 3, 2015

So I'm +1 for creating this shaded module with your PR and then decide if we really want to keep it around.

OK, ill keep it shaded for now. Ill try to add a test so we can get this in and iterate. Its all broken today and we can't stall this change on this stuff

@rmuir
Copy link
Contributor Author

rmuir commented Aug 3, 2015

I pushed a very simple test for the shaded jar, it just does the jar hell check. For now its something other than nothing...

@rmuir rmuir added review and removed WIP labels Aug 3, 2015
@dadoonet
Copy link
Member

dadoonet commented Aug 3, 2015

The change looks good to me. I did not test it though yet.

@rmuir
Copy link
Contributor Author

rmuir commented Aug 3, 2015

I tested, mvn verify passes: i will open some followup issues to improve the testing / do more cleanups

rmuir added a commit that referenced this pull request Aug 3, 2015
@rmuir rmuir merged commit c5b91b9 into elastic:master Aug 3, 2015
@clintongormley clintongormley added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts and removed review labels Aug 5, 2015
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants