Build: Fail if any libs depend on non-core libs #29336

nik9000 · 2018-04-02T19:11:06Z

Adds a precommit to every subprojects of :libs that fails the build
if there are any :libs dependencies.

I imagine it'd be possible to fail the build on startup if the
dependencies are unacceptable rather than use a precommit task but the
precommit task reports the error nicely and is easy to skip if for some
reason we want to temporarily skip it. So it felt like the right way to
go.

Since we now have three places where we resolve project substitutions
I've added dependencyToProject to project.ext in all projects. It
resolves both project style dependencies and "external" style (like
"org.elasticsearch:elasticsearch-core:${version}") dependencies to
Projects using the projectSubstitutions. I use this new function all
three places where resovle project substitutions.

Finally this pulls apply plugin: 'elasticsearch.build' out of
libs/*/build.gradle and into a subprojects clause in
libs/build.gradle. I do this entirely so that I can call
tasks.precommit.dependsOn checkDependencies without waiting for the
subprojects to be evaluated or worrying about whether or not they have
precommit set up in a normal way.

Adds a `precommit` to every subprojects of `:libs` that fails the build if there are any `:libs` dependencies. I imagine it'd be possible to fail the build on startup if the dependencies are unacceptable rather than use a precommit task but the precommit task reports the error nicely and is easy to skip if for some reason we want to temporarily skip it. So it felt like the right way to go. Since we now have three places where we resolve project substitutions I've added `dependencyToProject` to `project.ext` in all projects. It resolves both `project` style dependencies and "external" style (like "org.elasticsearch:elasticsearch-core:${version}") dependencies to `Project`s using the `projectSubstitutions`. I use this new function all three places where resovle project substitutions. Finally this pulls `apply plugin: 'elasticsearch.build'` out of `libs/*/build.gradle` and into a subprojects clause in `libs/build.gradle`. I do this entirely so that I can call `tasks.precommit.dependsOn checkDependencies` without waiting for the subprojects to be evaluated or worrying about whether or not they have `precommit` set up in a normal way.

elasticmachine · 2018-04-02T19:11:08Z

Pinging @elastic/es-core-infra

rjernst · 2018-04-02T20:34:14Z

I don't think we should do this as a task. This should fail during configuration. We already do similar traversal inside BuildPlugin.configureConfigurations, where we disable transitive dependencies (and always force version conflict failure). I also think this is going to be a little more complicated as we need "groups" of libs that can have dependencies on each other (eg painless is going to have painless-compiler and painless-whitelist jars).

jasontedor · 2018-04-02T22:36:32Z

I also think this is going to be a little more complicated as we need "groups" of libs that can have dependencies on each other (eg painless is going to have painless-compiler and painless-whitelist jars).

I think we can wait to cross that bridge when we get there.

rjernst · 2018-04-02T22:44:21Z

I think we can wait to cross that bridge when we get there.

I'm in the middle of splitting the painless jars out right now.

jasontedor · 2018-04-06T00:08:42Z

I'm in the middle of splitting the painless jars out right now.

That is, by my estimation, at least two if not three weeks away. Let us make the "group" change a follow-up to this PR, we can progress when the need is clearer.

rjernst · 2018-04-06T01:39:16Z

That's fine, but my concern about the way this is implemented still stands. Doing it via a task is not right; it needs to be checked during configuration.

jasontedor · 2018-04-06T01:55:27Z

That's fine, but my concern about the way this is implemented still stands. Doing it via a task is not right; it needs to be checked during configuration.

I have no objections to that, my comment was only about adding the "group" requirement on top of this specific PR.

nik9000 · 2018-04-10T15:32:05Z

I don't think we should do this as a task. This should fail during configuration.

If I think "there are two ways to do this and either are just fine" and I basically flip a coin @rjernst almost always has good arguments against the result of my coin flip. Well, more again the "either are just fine" part of my reasoning, but still. I'll change it to fail on startup.

nik9000 · 2018-04-12T21:21:28Z

I've switched this to run during configuration.

nik9000 · 2018-04-12T21:21:48Z

@rjernst, would you like to take another look? I've switched this to run during configuration.

rjernst

This looks much better @nik9000, thanks! I left a few comments.

rjernst · 2018-04-12T21:33:04Z

libs/build.gradle

+
+    /*
+     * Subprojects may depend on the "core" lib but may not depend on any
+     * other Elasticsearch code. This keeps are dependencies simpler to think


other Elasticsearch code -> other lib

Also, I would reword the second sentence to "This keeps the dependency graph simple"

rjernst · 2018-04-12T21:33:28Z

libs/build.gradle

+        configurations.all { Configuration conf ->
+            dependencies.all { Dependency dep ->
+                if (project.name.equals('elasticsearch-nio')) {
+                    System.err.println("ccc $conf.name $dep")


This looks like leftover debugging?

I'll shoot it.

rjernst · 2018-04-12T21:35:01Z

libs/build.gradle

+                    throw new InvalidUserDataException("projects in :libs "
+                            + "may not depend on other projects libs except "
+                            + ":libs:elasticsearch-core but "
+                            + "$project.path depends on $depProject.path")


I think you need brackets around these derefs? ${project.path} depends on ${depProject.path}

It isn't required but if you like it better I'm happy to do it that way.

I think it is clearer that way.

Fails the build if any subprojects of `:libs` have dependencies in `:libs` except for `:libs:elasticsearch-core`. Since we now have three places where we resolve project substitutions I've added `dependencyToProject` to `project.ext` in all projects. It resolves both `project` style dependencies and "external" style (like "org.elasticsearch:elasticsearch-core:${version}") dependencies to `Project`s using the `projectSubstitutions`. I use this new function all three places where resovle project substitutions. Finally this pulls `apply plugin: 'elasticsearch.build'` out of `libs/*/build.gradle` and into a subprojects clause in `libs/build.gradle`. I do this entirely so that I can call `tasks.precommit.dependsOn checkDependencies` without waiting for the subprojects to be evaluated or worrying about whether or not they have `precommit` set up in a normal way.

* master: Remove PipelineExecutionService#executeIndexRequest (elastic#29537) Fix overflow error in parsing of long geohashes (elastic#29418) Remove unused index.ttl.disable_purge setting (elastic#29527) FullClusterRestartIT.testRecovery should wait for all initializing shards Build: Fail if any libs depend on non-core libs (elastic#29336)

* master: (96 commits) TEST: Mute testEnsureWeReconnect Mute full cluster restart test recovery test REST high-level client: add support for Indices Update Settings API [take 2] (elastic#29327) Plugins: Fix native controller confirmation for non-meta plugin (elastic#29434) Remove PipelineExecutionService#executeIndexRequest (elastic#29537) Fix overflow error in parsing of long geohashes (elastic#29418) Remove unused index.ttl.disable_purge setting (elastic#29527) FullClusterRestartIT.testRecovery should wait for all initializing shards Build: Fail if any libs depend on non-core libs (elastic#29336) [TEST] REST client request without leading '/' (elastic#29471) Using ObjectParser in UpdateRequest (elastic#29293) Prevent accidental changes of default values (elastic#29528) [Docs] Add definitions to glossary (elastic#29127) Avoid self-deadlock in the translog (elastic#29520) Minor cleanup in NodeInfo.groovy Lazy configure build tasks that require older JDKs (elastic#29519) Simplify snapshot check in root build file Make NodeInfo#nodeVersion strongly-typed as Version (elastic#29515) Enable license header exclusions (elastic#29379) Use proper Java version for BWC builds (elastic#29493) ...

Currently this fails because the Eclipse configuration splits the main and test folders into separate projects to avoid circular dependencies. Relates elastic#29336

…29550) Currently this fails because the Eclipse configuration splits the main and test folders into separate projects to avoid circular dependencies. Relates #29336

nik9000 added review :Delivery/Build Build or test infrastructure v7.0.0 v6.3.0 labels Apr 2, 2018

nik9000 added 2 commits April 12, 2018 16:55

Merge branch 'master' into libs_deps_build

025c5e8

Switch to checking on startup

6d73b37

rjernst reviewed Apr 12, 2018

View reviewed changes

nik9000 added 2 commits April 16, 2018 11:15

Merge branch 'master' into libs_deps_build

00ffcdd

Fixup

50a4120

nik9000 merged commit 69aabb7 into elastic:master Apr 16, 2018

jpountz mentioned this pull request Apr 17, 2018

Fix dependency checks on libs when generating Eclipse configuration. #29550

Merged

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label 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: Fail if any libs depend on non-core libs #29336

Build: Fail if any libs depend on non-core libs #29336

nik9000 commented Apr 2, 2018

elasticmachine commented Apr 2, 2018

rjernst commented Apr 2, 2018

jasontedor commented Apr 2, 2018

rjernst commented Apr 2, 2018

jasontedor commented Apr 6, 2018

rjernst commented Apr 6, 2018

jasontedor commented Apr 6, 2018

nik9000 commented Apr 10, 2018

nik9000 commented Apr 12, 2018

nik9000 commented Apr 12, 2018

rjernst left a comment

rjernst Apr 12, 2018

nik9000 Apr 13, 2018

rjernst Apr 12, 2018

nik9000 Apr 13, 2018

rjernst Apr 12, 2018

nik9000 Apr 13, 2018

rjernst Apr 13, 2018

Build: Fail if any libs depend on non-core libs #29336

Build: Fail if any libs depend on non-core libs #29336

Conversation

nik9000 commented Apr 2, 2018

elasticmachine commented Apr 2, 2018

rjernst commented Apr 2, 2018

jasontedor commented Apr 2, 2018

rjernst commented Apr 2, 2018

jasontedor commented Apr 6, 2018

rjernst commented Apr 6, 2018

jasontedor commented Apr 6, 2018

nik9000 commented Apr 10, 2018

nik9000 commented Apr 12, 2018

nik9000 commented Apr 12, 2018

rjernst left a comment

Choose a reason for hiding this comment

rjernst Apr 12, 2018

Choose a reason for hiding this comment

nik9000 Apr 13, 2018

Choose a reason for hiding this comment

rjernst Apr 12, 2018

Choose a reason for hiding this comment

nik9000 Apr 13, 2018

Choose a reason for hiding this comment

rjernst Apr 12, 2018

Choose a reason for hiding this comment

nik9000 Apr 13, 2018

Choose a reason for hiding this comment

rjernst Apr 13, 2018

Choose a reason for hiding this comment