Consolidate dev-tools folder into elasticsearch-parent. #43

rmuir · 2015-05-21T04:14:50Z

This makes dev-tools a little module that other modules
pull with maven-remote-resources plugin. This way we can include
PMD configuration, whatever we need. Entirety of dev-tools soon...

I added the forbidden API configuration from es-core. Plugins don't
need to do the path based exceptions in POM files, these days just
use the @SuppressForbidden(reason = ...) mechanism via annotations
for exceptions.

I also added 'dev' profile, since it kinda goes along with forbidden-apis,
its a way to skip the check.

I tested this with kuromoji plugin, everything works easily with just these
additions to its POM:

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-remote-resources-plugin</artifactId>
</plugin>
<plugin>
  <groupId>de.thetaphi</groupId>
  <artifactId>forbiddenapis</artifactId>
</plugin>

Closes #42

This makes `dev-tools` a little module that other modules pull with maven-remote-resources plugin. This way we can include PMD configuration, whatever we need. Entirety of dev-tools soon... I added the forbidden API configuration from es-core. Plugins don't need to do the path based exceptions in POM files, these days just use the `@SuppressForbidden(reason = ...)` mechanism via annotations for exceptions. I also added 'dev' profile, since it kinda goes along with forbidden-apis, its a way to skip the check. I tested this with kuromoji plugin, everything works easily with just these additions to its POM: <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-remote-resources-plugin</artifactId> </plugin> <plugin> <groupId>de.thetaphi</groupId> <artifactId>forbiddenapis</artifactId> </plugin> Closes elastic#42

dadoonet · 2015-05-21T04:22:29Z

dev-tools/pom.xml

+  <build>
+    <plugins>
+      <plugin>
+        <artifactId>maven-remote-resources-plugin</artifactId>


I think this plugin should be defined in its parent.
Also don't forget the groupid. It will be mandatory in future versions of Maven

Ignore me. It's not a module, right?

It is a module, see last line of patch below, where the parent-pom has this defined this one as submodule. the remote reosurces plugin is already defined in the parent, so the version number is not needed. But the group id should be specified.

It's a module now because Robert added a new commit :)

Forget about that, you would need to add the parent-pom here, too, but that would produce a cycle or many additional executions.

But you should add the groupId:

<groupId>org.apache.maven.plugins</groupId> <artifactId>maven-remote-resources-plugin</artifactId>

I fixed the missing groupId.

Uwe is right, i should have explained this. I intentionally made this a standalone module, per the example on the maven website, to avoid cyclic dependencies.

rmuir · 2015-05-21T04:23:46Z

This needs some review from people that have maven knowledge. @dadoonet and @s1monw in case you guys have a free moment to take a look, I'd appreciate it.

I evaluated a couple alternatives:

forbidden-apis could instead use a "url" for the config, e.g. public http endpoint to source repo, but this is messy and wouldnt work well when you are actually testing changes to it. No atomicity of builds either. Also doesnt solve the general problem that we have all kinds of duplicated "resources" today I'd like to eliminate.
looks like we could use other plugins like maven-assembly-plugin but these looked like more XML. So i used the solution with less XML.

dadoonet · 2015-05-21T04:26:44Z

I like this solution a lot.
Just wondering if you could define it as a module so it will be published at the same time than its parent.

rmuir · 2015-05-21T04:31:01Z

@dadoonet I think that is correct. let me give it a try, if it works, I will update the PR!

rmuir · 2015-05-21T04:42:41Z

pom.xml

+                                 <!-- if the used Java version is too new, don't fail, just do nothing: -->
+                                 <failOnUnsupportedJava>false</failOnUnsupportedJava>
+                                 <!-- maven shade plugin is a nightmare -->
+                                 <failOnUnresolvableSignatures>false</failOnUnresolvableSignatures>


Guys try to ignore this for now :)

Just to explain the issue: forbidden-apis fails by default if any of your signatures cannot be resolved in the classpath. This is a good thing, and I want that behavior back. This is a temporarily solution to keep changes minimal and keep in sync with elasticsearch-core at the moment.

The issue is that elasticsearch-core signatures are defined as unshaded class names, but plugins depend on shaded versions.

One solution would be to fix es-core build to somehow run with shaded classpath. To me thats the real bug and the current inconsistency (that it tests with unshaded classpath but runs in production with a shaded one) scares me a lot. For example, we can't tighten our security policies at the package level because of this.

Another solution is to just pull out the 3rd party signatures into thirdparty.txt and have thirdparty_shaded.txt that goes with it.

Agreed. With modules, we can first define a shaded one and then make es depend on it.
So forbidden file will use shaded names?

Yeah, that will work.

rmuir · 2015-05-21T05:21:24Z

@dadoonet I switched it to a module. works nice:

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] Elasticsearch Build Resources ..................... SUCCESS [  0.660 s]
[INFO] Elasticsearch Parent POM .......................... SUCCESS [  0.089 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS

dadoonet · 2015-05-21T05:30:25Z

\o/ LGTM

…sed.

rmuir · 2015-05-21T05:36:46Z

I defined property elasticsearch.tools.directory (${project.build.directory}/dev-tools) which was already referred 5 times in the config. This makes it easier to reuse for other purposes. Like maybe release scripts, PMD or whatever duplication we want to attack next.

uschindler · 2015-05-21T06:02:40Z

pom.xml

+                    <plugin>
+                        <groupId>de.thetaphi</groupId>
+                        <artifactId>forbiddenapis</artifactId>
+                        <version>1.5.1</version>


This is here the outdated version not in-line with above plugin versiondefinition. As the plugin version is already defined above, you should completely remove it here, profiles may optionally use version patterns, but if you don't specifiy the version it will be executed in any case.

Thanks @uschindler , good catch. This same bug also exists in elasticsearch, I will fix it there, too.

…i copied it from :)

uschindler · 2015-05-21T06:15:51Z

forbidden-apis could instead use a "url" for the config, e.g. public http endpoint to source repo, but this is messy and wouldnt work well when you are actually testing changes to it. No atomicity of builds either. Also doesnt solve the general problem that we have all kinds of duplicated "resources" today I'd like to eliminate.

These URL references are bad for off-line builds. This would make it impossible to run the maven build without internet connection in an airplane :-) So the solution that I would prefer is to publish signatures as maven artifacts (there is an issue open: policeman-tools/forbidden-apis#13), which will be downloaded to the local Maven repository.

The solution presented here is more universally usable and I like it more (because you can share the whole dev-tools folders with scripts, PMD/CheckStyle stuff,...)

dadoonet · 2015-05-21T06:23:44Z

Yeah we came to your issue yesterday with @rmuir. Once there we could use it!

uschindler · 2015-05-21T06:56:34Z

I tested this with kuromoji plugin, everything works easily with just these additions to its POM: [...]

This is a current limitation of the structure of the parent POM. Basically there are 2 ways to define plugins in the parent POM:

inside build/pluginManagement/plugins: This just defines setting used by plugins, but don't enable them. This is nice to define default versions of plugins or settings, but each submodule has to enable the plugin. This is the reason, why it has to be enabled in kuromoji
if you define the plugins in build/plugins directly (outside pluginManagement), the configuration effect is the same, but it automatically enables the plugin, also for all submodules. This also executes the plugin on the parent POM (yes!), but this is generally handled correctly by the plugins. E.g. compiler plugin or forbidden-apis does not execute in <packaging>pom</packaging> (you see a message about skipped execution in build log).

See for example Apache TIKA's parent POM: https://github.com/apache/tika/blob/fd8484792260fe9412b2756623c3f1afd58465f5/tika-parent/pom.xml#L307

It enables several plugins for all submodules. If one of the submodules wants to disable a plugin inherited from the parent pom, you can set its execution to "none" in the submodule, e.g., https://github.com/apache/tika/blob/fd8484792260fe9412b2756623c3f1afd58465f5/tika-bundle/pom.xml#L357 (which disables forbiddenapis in the bundle module of TIKA),

rmuir · 2015-05-21T07:04:07Z

@uschindler thanks very much for explaining that. I prefer this approach. I will update the PR, and can disable forbidden temporarily or fix any violations in our plugins before we pushing here.

We really have to minimize the elements we have in plugin POMs and that will help a lot... Kuromoji POM (https://github.com/elastic/elasticsearch-analysis-kuromoji/blob/master/pom.xml) is already far too large for my tastes. I don't understand things like the test-dependencies having to be declared in each child and so on, we need to get to the bottom of this stuff :)

uschindler · 2015-05-21T07:12:32Z

Just keep in mind that not all plugins should be moved out of pluginManagement. For example, the "lifecycle-mapping" Eclipse plugin that tells m2e to not execute some of the plugins, needs to stay in pluginManagement (it is not able to execute at all, its just a "placeholder" to configure Eclipse's m2e).

in general +1! I would also prefer management like this: Split between configuration only definitions in POMs and inherited execution.

uschindler · 2015-05-21T07:17:33Z

We really have to minimize the elements we have in plugin POMs and that will help a lot... Kuromoji POM (https://github.com/elastic/elasticsearch-analysis-kuromoji/blob/master/pom.xml) is already far too large for my tastes. I don't understand things like the test-dependencies having to be declared in each child and so on, we need to get to the bottom of this stuff :)

I would define a parent for all "ES plugins" with all the plugins for the ES-plugins correctly configured (like assembly plugin, maybe test-resources. Some structure like this:

elasticsearch-parent -> elasticsearch
elasticsearch-parent -> elasticsearch-plugin-parent -> kuromoji, foobar

This would also make it easier for 3rd party plugins. They could use the "official" plugin parent POM and it will build correctly out of box.

This would also allow to handle the "shaded" resources shit :-) The plugin-parent would use the shaded resources.

In addition the plugin-parent module would also already configure dependencies needed: it could add the elasticsearch dependency already (as Robert mentioned) - because there can be no plugin that does not depend on elasticsearch?

dadoonet · 2015-05-21T07:21:40Z

Totally agreed. It will help to share sames settings for maven plugins across Elasticsearch plugins.
That said, we can introduce that later on if we don't want to go too far for the first iteration

uschindler · 2015-05-21T07:30:30Z

Yeah, 2nd iteration, because this involves more cleanup work and is not really related to the stuff here.

rmuir · 2015-05-21T12:53:36Z

OK, thanks again @uschindler . Now plugins are getting much simpler, with less duplication in each child POM. I will fix forbidden API violations in all plugins now, so that we can move forward.

dadoonet reviewed May 21, 2015
View reviewed changes

rmuir reviewed May 21, 2015
View reviewed changes

rmuir added 2 commits May 21, 2015 01:14

declare dev-tools as module

d88737e

Give a meaningful name for this module.

66c00d3

define elasticsearch.tools.directory property, so it can be cleanly u…

fa7d3da

…sed.

uschindler reviewed May 21, 2015
View reviewed changes

Remove unnecessary outdated version. TODO: fix this in es-core where …

9534ba8

…i copied it from :)

rmuir mentioned this pull request May 21, 2015

Remove useless outdated forbidden-api version in -Pdev config. elastic/elasticsearch#11273

Merged

Add missing groupId here.

f32f586

rmuir added 3 commits May 21, 2015 08:36

Move some plugins up so we don't have to redefine them in every project.

3e51b90

remove more duplication

f04b7e6

Remove more duplication. Es-core can easily "override" here explicitly

8125827

rmuir added 2 commits May 21, 2015 09:21

clean up this duplicate/mess

2920302

tweak this to the default packages seem to want

99821c3

rmuir closed this in 15b04b1 May 21, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate dev-tools folder into elasticsearch-parent. #43

Consolidate dev-tools folder into elasticsearch-parent. #43

rmuir commented May 21, 2015

dadoonet May 21, 2015

dadoonet May 21, 2015

uschindler May 21, 2015

dadoonet May 21, 2015

uschindler May 21, 2015

rmuir May 21, 2015

rmuir May 21, 2015

rmuir commented May 21, 2015

dadoonet commented May 21, 2015

rmuir commented May 21, 2015

rmuir May 21, 2015

dadoonet May 21, 2015

rmuir May 21, 2015

rmuir commented May 21, 2015

dadoonet commented May 21, 2015

rmuir commented May 21, 2015

uschindler May 21, 2015

rmuir May 21, 2015

uschindler commented May 21, 2015

dadoonet commented May 21, 2015

uschindler commented May 21, 2015

rmuir commented May 21, 2015

uschindler commented May 21, 2015

uschindler commented May 21, 2015

dadoonet commented May 21, 2015

uschindler commented May 21, 2015

rmuir commented May 21, 2015

Consolidate dev-tools folder into elasticsearch-parent. #43

Consolidate dev-tools folder into elasticsearch-parent. #43

Conversation

rmuir commented May 21, 2015

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

rmuir commented May 21, 2015

dadoonet commented May 21, 2015

rmuir commented May 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmuir commented May 21, 2015

dadoonet commented May 21, 2015

rmuir commented May 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented May 21, 2015

dadoonet commented May 21, 2015

uschindler commented May 21, 2015

rmuir commented May 21, 2015

uschindler commented May 21, 2015

uschindler commented May 21, 2015

dadoonet commented May 21, 2015

uschindler commented May 21, 2015

rmuir commented May 21, 2015