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

nik9000 · 2018-07-20T13:37:57Z

This bundles the x-pack:protocol project into the x-pack:plugin:core
project because we'd like folks to consider it an implementation detail
of our build rather than a separate artifact to be managed and depended
on. It is now bundled into both x-pack:plugin:core and
client:rest-high-level. To make this work I had to fix a few things.

Firstly, I had to make PluginBuildPlugin work with the shadow plugin.
In that case we have to bundle only the shadow dependencies and the
shadow jar.

Secondly, every reference to x-pack:plugin:core has to use the shadow
configuration. Without that the reference is missing all of the
un-shadowed dependencies. I tried to make it so that applying the shadow
plugin automatically redefines the default configuration to mirror the
shadow configuration which would allow us to use bare project references
to the x-pack:plugin:core project but I couldn't make it work. It'd look
like it works but then fail for transitive dependencies anyway. I think
it is still a good thing to do but I don't have the willpower to do it
now.

Finally, I had to fix an issue where Eclipse and IntelliJ didn't properly
reference shadowed transitive dependencies. Neither IDE supports shadowing
natively so they have to reference the shadowed projects. We fix this by
detecting shadow dependencies when in "Intellij mode" or "Eclipse mode"
and adding runtime dependencies to the same target. This convinces
IntelliJ and Eclipse to play nice.

elasticmachine · 2018-07-20T13:37:59Z

Pinging @elastic/es-core-infra

rjernst

This looks fine. A couple general comments:

Can you add docs somewhere in contributing or something like that on the ability to shade and using the shadow vs compile?
The name "shadow" for the configuration of what deps are not shaded. Is there a way to change this?

rjernst · 2018-07-23T20:58:25Z

build.gradle

@@ -516,6 +516,32 @@ allprojects {
  tasks.eclipse.dependsOn(cleanEclipse, copyEclipseSettings)
 }

+allprojects {
+  /*
+   * IntelliJ and, ironically, Eclipse don't know about the shadow plugin so


nit: It's unclear why this is "ironic". I would omit this.

Will do. It is ironic because and eclipse is caused by a shadow.

rjernst · 2018-07-23T21:00:21Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

+            /*
+             * We bundle the plugin's jar file and its dependencies. We have
+             * to wait until the project is evaluated before we know if the
+             * plug *has* the shadow plugin. If it does we need the shadow


plug -> project?

nik9000 · 2018-07-23T21:10:34Z

The name "shadow" for the configuration of what deps are not shaded. Is there a way to change this?

We'd have to make a new configuration. I was weary to do that at first but I think I understand how all things hangs together well enough to do that. What do you think of the name unshaded? What do you think of doing it in a follow up PR?

Can you add docs somewhere in contributing or something like that on the ability to shade and using the shadow vs compile?

I'll add that before merging.

nik9000 · 2018-07-23T21:11:17Z

And thanks for reviewing @rjernst!

Includes a start of explaining gradle and gradle projects which feels sort of required before talking about configurations. Not at all finished but a start.

alpar-t

What is the advantage of using the shadow plugin as opposed to configuring the x-pack:core, high level rest and possibly other builds to simply include the class and resource files from the protocol project as in jar { from project(':x-pack:protocol').classes ; from project(':x-pack:protocol').processResources } ( or something like that ) with a compile only dependency on project(':x-pack:protocol') to keep the IDEs happy about it could do this without leaking shadow plugin semantics across the build. It feels to me that we are throwing out too much of what the plugin does to warrant using it.
We could even generalize that by creating a configuration of our own bundled or baked that can have only project level dependencies that behave like mentioned above: added to the jar and as compileOnly deps. Would probably keep things simpler in the gradle side.

alpar-t · 2018-07-24T05:50:56Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -873,11 +876,20 @@ class BuildPlugin implements Plugin<Project> {
        project.dependencyLicenses.dependencies = project.configurations.runtime.fileCollection {
            it.group.startsWith('org.elasticsearch') == false
        } - project.configurations.compileOnly
+        project.plugins.withType(ShadowPlugin).whenPluginAdded {
+            project.dependencyLicenses.dependencies += project.configurations.shadow.fileCollection {


Not sure if this is new, but I think this will resolve the configuration at configuration time which is undesirable.

It mimics what we do above for the runtime configuration. I'm not arguing that it is right, just that it is in good company.

alpar-t · 2018-07-24T05:52:47Z

CONTRIBUTING.md

+
+<dl>
+<dt>`compile`</dt><dd>Code that is on the classpath at both compile and
+runtime. If the `shadow` plugin is applied to the project then this code is


Might want to link to the shadow to be 100% clear on what plugin is meant.
There are often multiple Gradle plugins doing the same thing and its easy to get confused.

alpar-t · 2018-07-24T05:56:25Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

+             * we're fine with the jar an all of the non-provided runtime
+             * dependencies.
+             */
+            project.afterEvaluate {


from accepts closures that run at execution time for this type of usage.
It does mean that the conditional has to move into the closure

from { project.plugins.hasPlugin(ShadowPlugin) ? project.shadowJar : project.jar } from { project.plugins.hasPlugin(ShadowPlugin) ? project.configurations.shadow : project.configurations.compileOnly }

That works! Awesome!

nik9000 · 2018-07-24T12:59:02Z

What is the advantage of using the shadow plugin as opposed to configuring the x-pack:core, high level rest and possibly other builds to simply include the class and resource files from the protocol project as in jar { from project(':x-pack:protocol').classes ; from project(':x-pack:protocol').processResources } ( or something like that ) with a compile only dependency on project(':x-pack:protocol') to keep the IDEs happy about it could do this without leaking shadow plugin semantics across the build. It feels to me that we are throwing out too much of what the plugin does to warrant using it.
We could even generalize that by creating a configuration of our own bundled or baked that can have only project level dependencies that behave like mentioned above: added to the jar and as compileOnly deps. Would probably keep things simpler in the gradle side.

I don't think it is worth re-implementing a bunch of the semantics of the shadow plugin though. We actually activate the META-INF merging across the board which is quite useful. And the jdbc driver uses relocation. So I think the shadow plugin is the right choice for us.

I don't think we can get away without leaking shadow semantics out of the build because we'd end up including the bundled classes twice in dependencies which will cause jar hell to fail. I'd love to alter the default configuration of a project when it has the shadow plugin so that we can just depend on the project and get the appropriate target. I just couldn't get it to work and got tired of fighting it.

I'll certainly look at your other points!

alpar-t · 2018-07-24T13:55:10Z

LGTM Would be great if we could change the default configuration to shadow but we can do it in another PR as discussed.

nik9000 · 2018-07-24T16:02:10Z

Thanks for reviewing @atorok and @rjernst! I have two follow up things to do after I finish backporting this:

Figure out if I can make the default context work extend from the shadow context so you don't need to say context: 'shadow' all over the build.
Figure out if I can rename the shadow context into something more obvious like unbundled.

rjernst · 2018-07-24T19:05:13Z

I like the idea of having a bundled configuration, but all of these ideas are things that we can iterate on. If this works, let's go with it for now.

* master: Security: revert to old way of merging automata (#32254) Networking: Fix test leaking buffer (#32296) Undo a debugging change that snuck in during the field aliases merge. Painless: Update More Methods to New Naming Scheme (#32305) [TEST] Fix assumeFalse -> assumeTrue in SSLReloadIntegTests Ingest: Support integer and long hex values in convert (#32213) Introduce fips_mode setting and associated checks (#32326) Add V_6_3_3 version constant [DOCS] Removed extraneous callout number. Rest HL client: Add put license action (#32214) Add ERR to ranking evaluation documentation (#32314) Introduce Application Privileges with support for Kibana RBAC (#32309) Build: Shadow x-pack:protocol into x-pack:plugin:core (#32240) [Kerberos] Add Kerberos authentication support (#32263) [ML] Extract persistent task methods from MlMetadata (#32319) Add Restore Snapshot High Level REST API Register ERR metric with NamedXContentRegistry (#32320) fixes broken build for third-party-tests (#32315) Allow Integ Tests to run in a FIPS-140 JVM (#31989) [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280) awaitsfix testRandomClusterStateUpdates [TEST] add version skip to weighted_avg tests Consistent encoder names (#29492) Add WeightedAvg metric aggregation (#31037) Switch monitoring to new style Requests (#32255) Rename ranking evaluation `quality_level` to `metric_score` (#32168) Fix a test bug around nested aggregations and field aliases. (#32287) Add new permission for JDK11 to load JAAS libraries (#32132) Silence SSL reload test that fails on JDK 11 [test] package pre-install java check (#32259) specify subdirs of lib, bin, modules in package (#32253) Switch x-pack:core to new style Requests (#32252) awaitsfix SSLConfigurationReloaderTests Painless: Clean up add methods in PainlessLookup (#32258) Fail shard if IndexShard#storeStats runs into an IOException (#32241) AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated Remove unnecessary warning supressions (#32250) CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185) Add new fields to monitoring template for Beats state (#32085)

Relates #32240

This bundles the x-pack:protocol project into the x-pack:plugin:core project because we'd like folks to consider it an implementation detail of our build rather than a separate artifact to be managed and depended on. It is now bundled into both x-pack:plugin:core and client:rest-high-level. To make this work I had to fix a few things. Firstly, I had to make PluginBuildPlugin work with the shadow plugin. In that case we have to bundle only the `shadow` dependencies and the shadow jar. Secondly, every reference to x-pack:plugin:core has to use the `shadow` configuration. Without that the reference is missing all of the un-shadowed dependencies. I tried to make it so that applying the shadow plugin automatically redefines the `default` configuration to mirror the `shadow` configuration which would allow us to use bare project references to the x-pack:plugin:core project but I couldn't make it work. It'd *look* like it works but then fail for transitive dependencies anyway. I think it is still a good thing to do but I don't have the willpower to do it now. Finally, I had to fix an issue where Eclipse and IntelliJ didn't properly reference shadowed transitive dependencies. Neither IDE supports shadowing natively so they have to reference the shadowed projects. We fix this by detecting `shadow` dependencies when in "Intellij mode" or "Eclipse mode" and adding `runtime` dependencies to the same target. This convinces IntelliJ and Eclipse to play nice.

* elastic/6.x: Tests: Fix XPack upgrade tests (#32352) Remove invalid entry from 6.3.2 release notes Number of utilities for writing gradle integration tests (#32282) Determine the minimum gradle version based on the wrapper (#32226) Enable FIPS JVM in CI (#32330) Build: Fix jarHell error I caused by last backport Build: Shadow x-pack:protocol into x-pack:plugin:core (#32240)

nik9000 · 2018-07-26T16:08:37Z

I opened #32409 to rework the configuration of the shadow plugin. I think it makes it much more obvious what is going on.

nik9000 added 13 commits July 19, 2018 08:38

WIP

519c3f0

Wip

8e6bf29

Works

9783529

Wip

6f4c706

Closer

1eefd66

woops

f2feb0b

More

82e5f8c

Moar shadow

90b02b2

Revert bad

d53a38a

Explain

8420ea6

Fix eclipse

bad3c1b

Fix intellij

17322bd

Fix featureAware

16cf779

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

nik9000 requested a review from rjernst July 20, 2018 13:40

nik9000 changed the title ~~WIP Build: Shadow x-pack:protocol into x-pack:plugin:core~~ Build: Shadow x-pack:protocol into x-pack:plugin:core Jul 20, 2018

nik9000 added review blocker labels Jul 20, 2018

rjernst approved these changes Jul 23, 2018

View reviewed changes

nik9000 added 2 commits July 23, 2018 17:41

Merge branch 'master' into x_pack_transport_protocol

4413a6b

Explanation of configurations

640b4f7

Includes a start of explaining gradle and gradle projects which feels sort of required before talking about configurations. Not at all finished but a start.

alpar-t reviewed Jul 24, 2018

View reviewed changes

nik9000 added 3 commits July 24, 2018 09:08

Wait until the last second!

99d91d7

Link

f3d7a5e

Fix last jarHell

d23123d

alpar-t approved these changes Jul 24, 2018

View reviewed changes

nik9000 merged commit e6b9f59 into elastic:master Jul 24, 2018

nik9000 added the backport pending label Jul 24, 2018

dnhatn added a commit that referenced this pull request Jul 25, 2018

CCR: use shadow plugin

a5d8f0b

Relates #32240

nik9000 removed the backport pending label Jul 25, 2018

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: Shadow x-pack:protocol into x-pack:plugin:core #32240

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

nik9000 commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

rjernst left a comment

rjernst Jul 23, 2018

nik9000 Jul 23, 2018

rjernst Jul 23, 2018

nik9000 Jul 23, 2018

nik9000 commented Jul 23, 2018

nik9000 commented Jul 23, 2018

alpar-t left a comment

alpar-t Jul 24, 2018

nik9000 Jul 24, 2018

alpar-t Jul 24, 2018

nik9000 Jul 24, 2018

alpar-t Jul 24, 2018

nik9000 Jul 24, 2018

nik9000 commented Jul 24, 2018

alpar-t commented Jul 24, 2018

nik9000 commented Jul 24, 2018

rjernst commented Jul 24, 2018

nik9000 commented Jul 26, 2018

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

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

Conversation

nik9000 commented Jul 20, 2018

elasticmachine commented Jul 20, 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

nik9000 commented Jul 23, 2018

nik9000 commented Jul 23, 2018

alpar-t 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

Choose a reason for hiding this comment

nik9000 commented Jul 24, 2018

alpar-t commented Jul 24, 2018

nik9000 commented Jul 24, 2018

rjernst commented Jul 24, 2018

nik9000 commented Jul 26, 2018