Shade external dependencies in the rest client jar by hub-cap · Pull Request #25780 · elastic/elasticsearch

hub-cap · 2017-07-18T20:16:15Z

This commit removes all external dependencies from the rest client jar,
and shades them in an 'internal.' package within the jar, using
shadowJar gradle plugin. All projects that depended on the existing jar
have been converted to using the 'internal.' package prefixes to
interact with the rest client.

Closes #25208

hub-cap · 2017-07-18T20:19:42Z

client/rest/build.gradle

heh, will fix this comment

javanna

I left a couple of comments, I will leave the gradle part to @rjernst for review. Did I get it right that the plan is to have two artifacts, one with deps and the other one without?

javanna · 2017-07-19T20:14:04Z

client/test/build.gradle

why is httpcore replaced with the low-level rest client here?

I think I get it. either we use the ordinary httpcore classes, or the internal ones but they come from our own big jar.

well, we really cant use ordinary ones now. Since all of the dependencies in rest client are shaded to use internal.org.apache classes, if we leak anything like Header into an API, we need to use the internal.org.apache since those are in the method signatures. So, we have to use our internal ones always, which is why we removed the httpcore stuff, because client/test now uses internal.o.a.h.Header, for example.

thanks for the explanation!

javanna · 2017-07-19T20:15:58Z

core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy

don't we need to remove the existing jar entry without the suffix?

Hopefully this will be explained better in the commit msg update

I also do not understand why both of these are necessary. The rest client used by the test framework for REST integ tests should be using the jar with no classifier. The deps version above should replace the 2 http grants below? I don't see where the nodeps version would come in.

It doesn't, anymore :)

I lied, it does, in an IDE /sadface

So we need the grant above for gradle and the grant below for the IDEs which don't use the fully shaded dependency. They use the -nodeps jar. I think they should be using the shadedDeps jar rather than the original dependencies. That way the packages line up.

Id love for them to use the shaded jar itself, but we use project dependencies, and intellij is smarter than me when it comes to dealing w/ dependencies. here is what happens to the other projects, and they seem to (only in intellij) depend on the jar task w/ the funky classifier, even though I remove it from the runtime artifacts and remove the extendsFrom compile. So these stay, just for intellij purposes. And they are using the same packages anyway, since the -deps jar and the no classifier actual shaded jar have the same relocate bits.

hub-cap · 2017-07-19T22:57:22Z

@javanna Ill leave some better comments in the github commit on why the artifacts are split out like that. its somewhat confusing.

hub-cap · 2017-07-20T02:32:46Z

ok I decided to just add better docs to the build.gradle instead. Its saner there anyway.

rjernst

I left a couple comments. I also think we should think of a better package name prefix, since these are part of the api (sorry I did not realize that when suggesting the name in the first place!).

rjernst · 2017-07-20T06:23:36Z

client/rest/build.gradle

Shouldn't there be no need to use relocate here? If there is then unit tests ran with unshaded classes, which is exactly what we were trying to avoid.

This is the duplicated shadowing task that actually shades the classes in src as well as the internal. things.

That is my point. We shade the dependency itself. Nothing in src should be referring to unshaded packages, so there should be no need to do t his relocation here.

rjernst · 2017-07-20T06:32:00Z

core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy

I also do not understand why both of these are necessary. The rest client used by the test framework for REST integ tests should be using the jar with no classifier. The deps version above should replace the 2 http grants below? I don't see where the nodeps version would come in.

javanna · 2017-07-20T07:19:20Z

client/rest/build.gradle

thanks for adding this comment!

hub-cap · 2017-07-20T20:36:31Z

@javanna @rjernst I have cleaned up the build so that it uses the actual shaded jar outside of the rest project, instead of relying on the transient project dependencies (which was the -deps.jar). I have also removed the -nodeps jar from existence outside of the build, so the shaded jar is the only thing that other projects know about / build to.

rjernst · 2017-07-20T21:50:34Z

Just a note about this in general: I think as a followup (for rc/GA) we need to pull javadocs from the deps and add them to the generated client jar javadocs.

This commit removes all external dependencies from the rest client jar, and shades them in an 'internal.' package within the jar, using shadowJar gradle plugin. All projects that depended on the existing jar have been converted to using the 'internal.' package prefixes to interact with the rest client. Closes elastic#25208

nik9000 · 2017-07-24T12:19:47Z

also think we should think of a better package name prefix, since these are part of the api

We could, in the grand tradition of Java, go super verbose with something like org.elasticsearch.client.shaded.httpcomponents.

tlrx

This looks promising. I have compilation errors in my IDE but maybe it's due to Intellij IDEA 2017.1.

tlrx · 2017-07-24T10:59:53Z

client/rest/build.gradle


+/**
+ * The rest client is a shaded jar. It contains the source of the rest client, as well as all the dependencies,
+ * shaded to the `internal.` package. 3 artifacts come out of this build process.


nit: doc needs to be updated once the package name is defined

tlrx · 2017-07-24T11:04:29Z

client/rest/build.gradle

  signature "org.codehaus.mojo.signature:java17:1.0@signature"
 }

+dependencyLicenses.dependencies = project.configurations.shade


I think we'll have to clean Apache license and notice files in the final Jar using a Shadow plugin Transformers or build our own one.

Ill chat w/ ryan about this, good call for bringing it up

nik9000 · 2017-07-24T12:22:25Z

client/rest/build.gradle


+/**
+ * The rest client is a shaded jar. It contains the source of the rest client, as well as all the dependencies,
+ * shaded to the `internal.` package. 3 artifacts come out of this build process.


internal. is no longer accurate.

nik9000 · 2017-07-24T12:24:20Z

client/rest/build.gradle

+ * 3) The *actual* jar that will be used by clients. This has no classifier, contains the rest client src and `internal.`.
+ *    This jar is the only actual output artifact of this job.
+ *
+ *  The IDE does not like removing artifacts and changing configurations on the fly, so the bits that make the build


s/The IDE/IDEs/ ? I expect you mean IntelliJ and Eclipse.

nik9000 · 2017-07-24T12:26:37Z

client/rest/build.gradle

+ *    The reason this jar was not overwritten was because gradle knows what its inputs and outputs are for a given task
+ *    and we should not alter that by overwriting the original jar. What we do is add a `nodeps` classifier to it instead.
+ *    This jar is removed from the output artifacts in cli, and retained in IDE builds.
+ * 2) A jar that contains *only* the `internal.` dependencies. This is a jar that is built before the src is compiled.


I like this comment but I'd rephrase it, maybe moving the number 2 jar to the number 1 jar and explaining that it is also there for IDEs. I think it'd be good also to explain that we use the shaded package names in the source so we still jar up the bits that come out of javac without any post processing.

nik9000 · 2017-07-24T12:32:22Z

build.gradle

  allprojects {
-    if (project.path == ':test:framework') {
+    if (project.path == ':test:framework' || project.path == ':client:test') {
      // :test:framework:test cannot run before and after :core:test


Can you add a comment explaining why :core:test also cannot run before :core:test?

Ill leave that for ryan to answer heh

I don't understand your statement @nik9000. The task graph cannot have cycles, and a dependency on yourself is still a cycle.

Sorry, :client:test cannot run before :core:test. Basically, if we are going to leave a comment explaining why :test:framework needs this exemption we should probably explain the :client:test too.

Ok I see the confusion. The comment should be updated. It means that if the code below this were to run, a circular dependency would be added between :test:framework:test and :core:test. Likewise, the new check is added because :client:test would have a circular task dep with :client:rest:test.

nik9000 · 2017-07-24T12:34:26Z

client/rest/build.gradle

+shadowJar {
+  configurations = [project.configurations.shade]
+  classifier = null
+  relocate 'org.apache', 'org.elasticsearch.client'


I think it'd be clearer to do this with the regular jar task because you are just combining the two jar files into one, right? At least, I thought that is what we'd talked about doing.

You are right, at this point I could combine the deps jar with the src/ in the project. I can follow up with this, as its a better idea than running another "very similar" shadow task. I can follow up with this

nik9000 · 2017-07-24T12:35:14Z

client/rest/build.gradle

+}
+
+jar {
+  // rename the default jar because we are removing the classifier from the shadowJar, which is a null classifier


I'd say something like "move the default jar to another classifier so the jar with the dependencies can have the empty classifier".

nik9000 · 2017-07-24T12:44:46Z

core/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy

So we need the grant above for gradle and the grant below for the IDEs which don't use the fully shaded dependency. They use the -nodeps jar. I think they should be using the shadedDeps jar rather than the original dependencies. That way the packages line up.

rjernst · 2017-07-24T14:01:14Z

We could, in the grand tradition of Java, go super verbose with something like org.elasticsearch.client.shaded.httpcomponents

Please let's not do that. The package names have real costs (eg Windows file path limits).

nik9000 · 2017-07-24T14:24:09Z

Please let's not do that. The package names have real costs (eg Windows file path limits).

Fine by me. I'd certainly prefer it if all the shaded things were grouped together in some obvious way but I'm not going to block anything over it.

rjernst

This works great for me from the CLI. I at first had issues testing it with IntelliJ, in 2016.2. I upgraded to 2017.2, but still had issues. I then tried using the "module per source set" option and it "sort of" worked, but I needed to downgrade to 2017.1. So, I think we should add a note in CONTRIBUTING that using the module per source set option is now required, and probably also a note that 2017.1 is recommended (2017.2 was just recently released so I expect the issue should be fixed in a patch release).

hub-cap · 2017-07-24T17:12:32Z

👍 ill create a few issues off this initial one based on the feedback here

javanna · 2017-07-24T18:55:17Z

thanks @hub-cap ! Was this supposed to go to 5.x too? I see it has the 5.6 label.

hub-cap · 2017-07-24T19:25:15Z

yes i have backported it, ty for keepin an eye out @javanna <3

hub-cap added :Java High Level REST Client :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch labels Jul 18, 2017

hub-cap commented Jul 18, 2017

View reviewed changes

client/rest/build.gradle Outdated

Copy link
Copy Markdown

Contributor Author

hub-cap Jul 18, 2017

Choose a reason for hiding this comment

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

heh, will fix this comment

clintongormley added >enhancement v5.6.0 v6.0.0 labels Jul 19, 2017

clintongormley requested review from javanna and rjernst July 19, 2017 08:17

javanna removed the :Java High Level REST Client label Jul 19, 2017

javanna reviewed Jul 19, 2017

View reviewed changes

hub-cap force-pushed the issues/25208 branch from 5dc2822 to fb0a3dd Compare July 20, 2017 02:34

rjernst reviewed Jul 20, 2017

View reviewed changes

javanna reviewed Jul 20, 2017

View reviewed changes

client/rest/build.gradle Outdated

Copy link
Copy Markdown

Contributor

javanna Jul 20, 2017

Choose a reason for hiding this comment

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

thanks for adding this comment!

hub-cap force-pushed the issues/25208 branch from fb0a3dd to 5c69faa Compare July 20, 2017 20:32

hub-cap force-pushed the issues/25208 branch 3 times, most recently from 54c9d76 to 8e5060b Compare July 21, 2017 20:15

hub-cap added 6 commits July 23, 2017 22:43

Adding better docs

5a5aaca

Fix the output of the build to actually be the shaded jar

3294fdf

Fixing missed paths with rebase

b3db97c

change location to org.elasticsearch.client

c3e122c

Cleaning up comments just a bit more

6bd53c4

hub-cap force-pushed the issues/25208 branch from 8e5060b to 6bd53c4 Compare July 24, 2017 03:59

tlrx reviewed Jul 24, 2017

View reviewed changes

nik9000 reviewed Jul 24, 2017

View reviewed changes

Comment cleanup

4b4667c

rjernst approved these changes Jul 24, 2017

View reviewed changes

Fixing the license issue once these projects were removed

a8a8905

hub-cap merged commit e816ef8 into elastic:master Jul 24, 2017

hub-cap deleted the issues/25208 branch July 25, 2017 05:43

clintongormley added v6.0.0-beta1 :Java High Level REST Client and removed v6.0.0 :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch labels Jul 25, 2017

javanna added :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch and removed :Java High Level REST Client labels Aug 1, 2017

Conversation

hub-cap commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Jul 19, 2017

Uh oh!

hub-cap commented Jul 20, 2017

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Jul 20, 2017

Uh oh!

rjernst commented Jul 20, 2017

Uh oh!

nik9000 commented Jul 24, 2017

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!