Build: Rework shadow plugin configuration #32409

nik9000 · 2018-07-26T16:07:13Z

This reworks how we configure the shadow plugin in the build. The major
change is that we no longer bundle dependencies in the compile configuration,
instead we bundle dependencies in the new bundle configuration. This feels
more right because it is a little more "opt in" rather than "opt out" and the
name of the bundle configuration is a little more obvious.

As an neat side effect of this, the runtimeElements configuration used when
one project depends on another now contains exactly the dependencies needed
to run the project so you no longer need to reference projects that use the
shadow plugin like this:

testCompile project(path: ':client:rest-high-level', configuration: 'shadow')

You can instead use the much more normal:

testCompile "org.elasticsearch.client:elasticsearch-rest-high-level-client:${version}"

elasticmachine · 2018-07-26T16:07:15Z

Pinging @elastic/es-core-infra

rjernst

Thank you Nik! This looks much cleaner. I have a couple questions.

rjernst · 2018-07-26T18:51:37Z

benchmarks/build.gradle

@@ -33,13 +33,13 @@ archivesBaseName = 'elasticsearch-benchmarks'
 test.enabled = false

 dependencies {
-    compile("org.elasticsearch:elasticsearch:${version}") {
+    bundle("org.elasticsearch:elasticsearch:${version}") {


Why would we need to bundle anything in benchmarks?

I have no clue. It has been shadowing all of its dependencies for ages and I was keeping it going. I could dig into it though.

Ok, I didn't realize that. Quite odd, maybe @danielmitterdorfer can elaborate on why we do this?

This is an area where we can improve a lot: commenting the non-obvious choices in our code.

I think it's for convenience, so that you can run the benchmarks as simply as java -jar benchmarks.jar <class under benchmark>.

I've done a little digging and I also think it is for convenience. Every example I see of using JMH shades the dependencies but this says that it is not required.

rjernst · 2018-07-26T18:52:41Z

build.gradle

@@ -296,7 +297,7 @@ subprojects {
    // org.elasticsearch:elasticsearch must be the last one or all the links for the
    // other packages (e.g org.elasticsearch.client) will point to server rather than
    // their own artifacts.
-    if (project.plugins.hasPlugin(BuildPlugin)) {
+    if (project.plugins.hasPlugin(BuildPlugin) || project.plugins.hasPlugin(PluginBuildPlugin)) {


PluginBuildPlugin extends BuildPlugin, maybe it should apply it instead? Then we only need the one check.

I think that'd be cleaner but I didn't want to do it while messing around with shadow stuff.

rjernst · 2018-07-26T18:56:33Z

client/benchmark/build.gradle

-  compile project(path: ':modules:reindex', configuration: 'runtime')
-  compile project(path: ':modules:lang-mustache', configuration: 'runtime')
-  compile project(path: ':modules:percolator', configuration: 'runtime')
+  bundle("org.elasticsearch:elasticsearch:${version}")


Again, why do benchmark jars for testing need bundled deps?

nik9000 · 2018-08-01T00:40:47Z

OK! I've removed the shadow stuff from benchmarks in another PR and rebased this on top. What do you think, @rjernst ?

alpar-t

This looks really good!

rjernst

This is much better, thanks! Just a couple final questions.

rjernst · 2018-08-09T02:06:43Z

build.gradle

-	  }
-	}
+    project.plugins.withType(ShadowPlugin).whenPluginAdded {
+      project.afterEvaluate {


Can this be done when the bundle configuration is added, in BuildPlugin, instead of in an afterEvaluate?

BuildPlugin doesn't have isEclipse or isIdea but I can do this after the BuildPlugin is added.

BuildPlugin doesn't have isEclipse or isIdea

Maybe it should? It seems odd to have to do this extra work, it should really be part of the base setup.

I'll use it if it is defined.

rjernst · 2018-08-09T02:08:59Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/JarHellTask.groovy

@@ -39,6 +40,9 @@ public class JarHellTask extends LoggedExec {
    public JarHellTask() {
        project.afterEvaluate {
            FileCollection classpath = project.sourceSets.test.runtimeClasspath
+            if (project.plugins.hasPlugin(ShadowPlugin)) {
+                classpath += project.configurations.bundle


Since compile extends from bundle, shouldn't this be in the test runtime classpath already?

compile only extends from bundle when we're configuring an IDE.

This reverts commit e6a4ea4.

nik9000 · 2018-08-09T19:54:05Z

There are timing issues with moving the isIdea stuff to BuildPlugin. I think if we want to move isIdea and isEclipse to BuildPlugin we should move creating it to the plugin and we should do it in a separate change.

nik9000 · 2018-08-21T18:27:23Z

I ran into some issues with IntelliJ and had to do some minor fixups here. None of the build logic changed but a little of the configuration did. I'm retesting with @hub-cap to make sure this is good. It looks fine on my side but I don't know much about IntelliJ other than "open the project and click the hammer button". I'm also waiting on the PR testing robot to catch up with it.

hub-cap · 2018-08-21T22:17:19Z

@nik9000 this looks good now from my end. Thank you for chasing down all the annoying bits here. <3

This reworks how we configure the `shadow` plugin in the build. The major change is that we no longer bundle dependencies in the `compile` configuration, instead we bundle dependencies in the new `bundle` configuration. This feels more right because it is a little more "opt in" rather than "opt out" and the name of the `bundle` configuration is a little more obvious. As an neat side effect of this, the `runtimeElements` configuration used when one project depends on another now contains exactly the dependencies needed to run the project so you no longer need to reference projects that use the shadow plugin like this: ``` testCompile project(path: ':client:rest-high-level', configuration: 'shadow') ``` You can instead use the much more normal: ``` testCompile "org.elasticsearch.client:elasticsearch-rest-high-level-client:${version}" ```

This change was made to master, this commit brings it over to index-lifecycle. See #32409

nik9000 added 9 commits July 25, 2018 10:30

WIP

cae6b5c

Build: Rework shadowing

7f89181

Javadocs

831d6db

Drop unneeded

9d30c49

Explain

c2c64c5

Merge branch 'master' into shadow_default

293cc2a

Almost

59e263b

Merge branch 'master' into shadow_default

4ea85ac

Better reference

1202dbe

nik9000 added >enhancement review :Delivery/Build Build or test infrastructure v7.0.0 v6.5.0 labels Jul 26, 2018

nik9000 requested review from rjernst and alpar-t July 26, 2018 16:07

nik9000 mentioned this pull request Jul 26, 2018

Build: Shadow x-pack:protocol into x-pack:plugin:core #32240

Merged

rjernst reviewed Jul 26, 2018

View reviewed changes

nik9000 added 2 commits July 31, 2018 18:32

Merge branch 'master' into shadow_default

10195a0

Remove benchmark changes

b8ad995

alpar-t approved these changes Aug 6, 2018

View reviewed changes

rjernst approved these changes Aug 9, 2018

View reviewed changes

nik9000 added 4 commits August 9, 2018 13:26

Merge branch 'master' into shadow_default

d5be929

More precise

e30b8a7

Move idea checking

e6a4ea4

Revert "Move idea checking"

03f2f92

This reverts commit e6a4ea4.

Merge branch 'master' into shadow_default

744e77f

nik9000 added 8 commits August 20, 2018 11:43

Merge branch 'master' into shadow_default

dd1c1cc

Fixup build

fb0ba83

Unbreak

ea6f915

Fixup

1d2d77d

Hate hate hate hat

8581519

I dind't mean to

d7479de

fix indentation

5dab259

Merge branch 'master' into shadow_default

c9e6230

nik9000 added 3 commits August 21, 2018 15:08

One more?

f924f2c

Merge branch 'master' into shadow_default

1b6614d

It never ends

b3a0cb7

nik9000 merged commit 2c81d7f into elastic:master Aug 22, 2018

nik9000 added the backport pending label Aug 22, 2018

nik9000 removed backport pending review labels Aug 22, 2018

gwbrown added a commit that referenced this pull request Aug 23, 2018

Fix Gradle configuration

191bd7c

This change was made to master, this commit brings it over to index-lifecycle. See #32409

gwbrown added a commit that referenced this pull request Aug 29, 2018

Fix Gradle configuration

7d679f7

This change was made to master, this commit brings it over to index-lifecycle. See #32409

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Rework shadow plugin configuration #32409

Build: Rework shadow plugin configuration #32409

nik9000 commented Jul 26, 2018

elasticmachine commented Jul 26, 2018

rjernst left a comment

rjernst Jul 26, 2018

nik9000 Jul 26, 2018

rjernst Jul 26, 2018

jasontedor Jul 26, 2018

jasontedor Jul 26, 2018 •

edited

Loading

nik9000 Jul 30, 2018

rjernst Jul 26, 2018

nik9000 Jul 26, 2018

rjernst Jul 26, 2018

nik9000 commented Aug 1, 2018

alpar-t left a comment

rjernst left a comment

rjernst Aug 9, 2018

nik9000 Aug 9, 2018

rjernst Aug 9, 2018

nik9000 Aug 9, 2018

rjernst Aug 9, 2018

nik9000 Aug 9, 2018 •

edited

Loading

nik9000 commented Aug 9, 2018

nik9000 commented Aug 21, 2018

hub-cap commented Aug 21, 2018

Build: Rework shadow plugin configuration #32409

Build: Rework shadow plugin configuration #32409

Conversation

nik9000 commented Jul 26, 2018

elasticmachine commented Jul 26, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Aug 1, 2018

alpar-t left a comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

nik9000 commented Aug 9, 2018

nik9000 commented Aug 21, 2018

hub-cap commented Aug 21, 2018

jasontedor Jul 26, 2018 •

edited

Loading

nik9000 Aug 9, 2018 •

edited

Loading