Skip to content

Make build Gradle 2.14 / 3.x compatible#22669

Merged
ywelsch merged 3 commits intoelastic:masterfrom
ywelsch:enhance/gradle-3x
Jan 19, 2017
Merged

Make build Gradle 2.14 / 3.x compatible#22669
ywelsch merged 3 commits intoelastic:masterfrom
ywelsch:enhance/gradle-3x

Conversation

@ywelsch
Copy link
Copy Markdown
Contributor

@ywelsch ywelsch commented Jan 18, 2017

This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.

Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I'm good with it, but I left one question. I think that @rjernst should have an opportunity to review before this is merged.

// Use logging dependency and add default imports for "org.gradle.internal.logging.progress"
compileGroovy.groovyOptions.configurationScript = file('src/compile/groovy/gradle-2.14-loggers.groovy')
dependencies {
compile "org.gradle:gradle-logging:3.3" // any version >= 2.14 is ok, take a recent one
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels weird, why not make it dependent on the gradle version?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For example:

diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle
index d8d4f3dc50..61a88de947 100644
--- a/buildSrc/build.gradle
+++ b/buildSrc/build.gradle
@@ -105,7 +105,7 @@ if (GradleVersion.current() == GradleVersion.version("2.13")) {
   // Use logging dependency and add default imports for "org.gradle.internal.logging.progress"
   compileGroovy.groovyOptions.configurationScript = file('src/compile/groovy/gradle-2.14-loggers.groovy')
   dependencies {
-    compile "org.gradle:gradle-logging:3.3" // any version >= 2.14 is ok, take a recent one
+    compile "org.gradle:gradle-logging:" + GradleVersion.current().version
     compile 'ru.vyarus:gradle-animalsniffer-plugin:1.2.0' // Gradle 2.14 requires a version > 1.0.1
   }
 }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason I fixed the version is because we export gradle-logging as dependency when publishing build-tools. This would mean that the dependencies of build-tools would change depending on whether someone uses Gradle 3.2 or 3.3 to build the artifact (the issue is still there between 2.13 and 2.14, but if we would move to 2.14+ in the near future, we could require 2.14 as minimal version and at least avoid this discrepancy from that point onward). Even though we use a fixed Vagrant image for building (with specific Gradle version installed), I felt that this would be too brittle. Another possibility to reuse the ProgressLogger code might be to shade the dependency, which could make it uniformly available in Gradle 2.13 and 2.14+. We would then have to publish that shaded artifact as well though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've taken a different route: I've made this dependency compileOnly as it is not needed by the runtime (that one bundles already the classes). It's ok thus to use GradleVersion.current().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much better, thank you.

@rjernst
Copy link
Copy Markdown
Member

rjernst commented Jan 18, 2017

Does this work at all on java 9? It is very very hacky, but I guess I'm ok with it for now, until gradle folks finish working on re-exposing progress logger...but it could break in any gradle release.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Jan 18, 2017

until gradle folks finish working on re-exposing progress logger

At this point I've kind of lost hope that they are going to do that. It looks like it has been on the list of things for the next release for a long time.

@jasontedor
Copy link
Copy Markdown
Member

jasontedor commented Jan 18, 2017

At this point I've kind of lost hope that they are going to do that. It looks like it has been on the list of things for the next release for a long time.

It was bumped out of the 3.x series into the 4.x series about four weeks ago in gradle/gradle#730. So, I agree with this sentiment.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Jan 18, 2017

I feel like we'd be better off forking ProgressLogger into our build tools but I haven't looked into it to know if that is possible. But forking it would fix the version dependency issue....

@rjernst
Copy link
Copy Markdown
Member

rjernst commented Jan 18, 2017

There is no way to fork it. We would have to take control of all gradle output.

@ywelsch
Copy link
Copy Markdown
Contributor Author

ywelsch commented Jan 18, 2017

@rjernst I had to adapt an existing hack in dc55c34 so that the hack also works with Gradle 3.x. With that, I was able to run the compilation/tests with Java 9 (still running Gradle with Java 8, however). Making Gradle run with Java 9 (non-Jigsaw) requires some more adjustments to the build-file and additional add-opens. I've gotten it to run most of the stuff already, but wonder if it's worth the trouble figuring this out before Gradle's Java 9 support is in a better state.

Copy link
Copy Markdown
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Let's make it work. I want to experiment with 3.x. :) Thanks for doing this @ywelsch.

@ywelsch ywelsch merged commit 652cb7d into elastic:master Jan 19, 2017
ywelsch added a commit that referenced this pull request Jan 19, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 19, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 19, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 19, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
@ywelsch
Copy link
Copy Markdown
Contributor Author

ywelsch commented Jan 19, 2017

Thanks @jasontedor @rjernst @nik9000

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 20, 2017
jasontedor added a commit that referenced this pull request Jan 20, 2017
jasontedor added a commit that referenced this pull request Jan 20, 2017
jasontedor added a commit that referenced this pull request Jan 20, 2017
jasontedor added a commit that referenced this pull request Jan 20, 2017
jasontedor added a commit that referenced this pull request Jan 20, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 23, 2017
* master: (33 commits)
  Docs fix - Added missing link to new Adjacency-matrix agg
  Pass `forceExecution` flag to transport interceptor (elastic#22739)
  Version: Add missing releases from 2.x in Version.java (elastic#22594)
  CONSOLE-ify filter aggregation docs
  CONSOLE-ify date_range aggregation docs
  Add single static instance of SpecialPermission (elastic#22726)
  Simplify InternalEngine#innerIndex (elastic#22721)
  Upgrade to Lucene 6.4.0 (elastic#22724)
  Fix broken TaskInfo.toString()
  Add CheckedSupplier and CheckedRunnable to core (elastic#22725)
  Revert "Make build Gradle 2.14 / 3.x compatible (elastic#22669)"
  Fixes retrieval of the latest snapshot index blob (elastic#22700)
  CONSOLE-ify date histogram docs
  CONSOLE-ify min and max aggregation docs
  CONSOLE-ify global-aggregation.asciidoc
  Fix script score function that combines _score and weight (elastic#22713)
  Corrected a plural verb to a singular one. (elastic#22681)
  Fix duplicates from search.query (elastic#22701)
  Readd unconverted snippets mark for doc
  Deguice rest handlers (elastic#22575)
  ...
ywelsch added a commit that referenced this pull request Jan 24, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 24, 2017
Instead of using Gradle-version specific compilation options, use distinct source sets. This also allows compilation of buildSrc/build-tools under IDEs that
don't understand the version-specific compilation options.

Relates to #22669
@ywelsch
Copy link
Copy Markdown
Contributor Author

ywelsch commented Jan 24, 2017

I've repushed with an additional commit 12b6ff5 that fixes the issue as well for compiling build-tools in IntelliJ (using custom source sets now instead of custom compilation options)

ywelsch added a commit that referenced this pull request Jan 24, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 24, 2017
Instead of using Gradle-version specific compilation options, use distinct source sets. This also allows compilation of buildSrc/build-tools under IDEs that
don't understand the version-specific compilation options.

Relates to #22669
ywelsch added a commit that referenced this pull request Jan 24, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 24, 2017
Instead of using Gradle-version specific compilation options, use distinct source sets. This also allows compilation of buildSrc/build-tools under IDEs that
don't understand the version-specific compilation options.

Relates to #22669
ywelsch added a commit that referenced this pull request Jan 24, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 24, 2017
Instead of using Gradle-version specific compilation options, use distinct source sets. This also allows compilation of buildSrc/build-tools under IDEs that
don't understand the version-specific compilation options.

Relates to #22669
ywelsch added a commit that referenced this pull request Jan 24, 2017
This changes build files so that building Elasticsearch works with both Gradle 2.13 as well as higher versions of Gradle (tested 2.14 and 3.3), enabling a smooth transition from Gradle 2.13 to 3.x.
ywelsch added a commit that referenced this pull request Jan 24, 2017
Instead of using Gradle-version specific compilation options, use distinct source sets. This also allows compilation of buildSrc/build-tools under IDEs that
don't understand the version-specific compilation options.

Relates to #22669
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants