Upgrade to Gradle Wrapper 4.8 #31271

alpar-t · 2018-06-12T13:26:58Z

relates to #31230 : move to gradle 4.8 so we can start using JDK 11 in CI

Changes:

the update itself to use Gradle 4.8
work around Gradle 4.8 no longer considers versions equal when using dependencySubstitution and failOnVersionConflict gradle/gradle#5692 and Can no longer configure GenerateMavenPom tasks in 4.8, works in 4.7 gradle/gradle#5696
compare-gradle-builds plugin to be able to test the change
- this requires a git worktree to be set up ( typically to a common base, like a good CI build ) the plugin will assemble both and compare outputs in terms of generated zip and jar files. The plugin has a lot of missing feature but it's still useful to make sure there are no major changes w.r.t the artifacts that are produced. There's no support to deal with files that change by nature, like the manifest, thus the property that causes an empty manifest to be generated to make otherwise identical jars not show up in the report.

The current does not work with Gradle 4.8 RC1

elasticmachine · 2018-06-12T13:27:00Z

Pinging @elastic/es-core-infra

nik9000 · 2018-06-12T13:46:35Z

build.gradle

@@ -561,3 +561,27 @@ gradle.projectsEvaluated {
    }
  }
 }
+
+apply plugin: 'compare-gradle-builds'


I wonder if we should keep this long term. I just feel like we won't need it again for six months and when we want it again we'll have to edit all of this to make it work.

Is it worth applying the plugin at all if we don't have build.compare set?

nik9000 · 2018-06-12T13:48:05Z

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

-                if (assemble) {
-                    assemble.dependsOn(t)
+            project.getGradle().getTaskGraph().whenReady {
+                project.tasks.withType(GenerateMavenPom.class) { GenerateMavenPom t ->


Can you add a comment with the gradle issue number?

nik9000 · 2018-06-12T13:48:37Z

gradle/wrapper/gradle-wrapper.properties

@@ -1,6 +1,6 @@
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.7-all.zip
 distributionBase=GRADLE_USER_HOME


nik9000 · 2018-06-12T13:48:43Z

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

@@ -625,6 +630,10 @@ class BuildPlugin implements Plugin<Project> {
                        jarTask.manifest.attributes('Change': shortHash)
                    }
                }
+                // Force manifest entries that change by nature to a constant to be able to compare builds more effectively


I feel the same way about this one too. Until we need it again it'll sit there making folks wonder "when do we set this!?" and when we finally go to use it again we'll need to make other changes.

I think it is worth adding something like "when comparing builds, force manifest entries..." that way it is clear to the reader that we only do this when it is required to compare builds.

nik9000 · 2018-06-12T13:51:45Z

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

-            configuration.resolutionStrategy.failOnVersionConflict()
+            configuration.resolutionStrategy { 
+                failOnVersionConflict()
+                preferProjectModules()


Can you add a comment with the gradle issue number?

nik9000 · 2018-06-12T13:51:51Z

buildSrc/build.gradle

@@ -106,7 +106,6 @@ GradleVersion logVersion = GradleVersion.current() > GradleVersion.version('4.3'

 dependencies {
  compileOnly "org.gradle:gradle-logging:${logVersion.getVersion()}"
-  compile 'ru.vyarus:gradle-animalsniffer-plugin:1.2.0' // Gradle 2.14 requires a version > 1.0.1


alpar-t · 2018-06-12T14:44:09Z

I don't have a strong opinion on comparing the builds. I don't like the change to generate an empty manifest, but unfortunately the plugin doesn't give us any options to influence how it compares ( however it does seem from how it's implemented it's a future goal ). That being said, I think this will be useful way sooner than 6 months. In fact I think that it will be useful as a sanity check for most build related changes, just looking at what I plan to do next, comparing building with jdk11, and dealing with deprications in 4.8, like stable publishing, i'll be able to use this.

nik9000 · 2018-06-12T14:55:24Z

Do you think it'd make sense to only set up the compare plugin if you set -Dbuild.compare=path_to_reference?

alpar-t · 2018-06-12T15:04:15Z

@nik9000 generally, I like command line options with conventions that help one avoid typing, but in this case, it could replace -Dbuild.compare_friendly because we could just check that the property is set, making it less confusing. Could also have -Dbuild.compare=true and default to the current path, or even --build-compare=. As a side note, I think it would make sense to start moving from properties to options for things that we expect to change for different use-cases ( e.x. in CI ) and restrict the scope of properties to debugging. In other words, properties that act as arguments should really be arguments.

nik9000 · 2018-06-12T15:15:19Z

As a side note, I think it would make sense to start moving from properties to options for things that we expect to change for different use-cases ( e.x. in CI ) and restrict the scope of properties to debugging. In other words, properties that act as arguments should really be arguments.

We had some options but we had them integrated in way that caused trouble and we ended up switching to options.

I'd prefer we not develop a convention for "where to check out the old version of Elasticsearch to compare the builds" so I like the idea of a command line option for it. And as a side benefit we don't have to apply the plugin if you don't set the option.

alpar-t · 2018-06-13T08:17:10Z

ci, test this please

elastic#31324

alpar-t · 2018-06-14T13:04:56Z

@nik9000 I just added a property to make build compare conditional for now, we'll see where it goes from here. There's also a work around for #31324 to get the tests passing.

nik9000 · 2018-06-15T17:51:27Z

server/src/main/java/org/elasticsearch/bootstrap/Security.java

@@ -221,14 +221,16 @@ static Policy readPolicy(URL policyFile, Map<String, URL> codebases) {
                    if (aliasProperty.equals(property) == false) {
                        propertiesSet.add(aliasProperty);
                        String previous = System.setProperty(aliasProperty, url.toString());
-                        if (previous != null) {
+                        // allow to re-set to what was previously there. https://github.com/elastic/elasticsearch/issues/31324


When I pulled this locally and reverted the changes to this file I didn't have any trouble. We've traditionally been very weary of making changes to this file so I'd really like to make sure we need this before we do it, even if it is temporary.

Oh, wait, I just got it! Neat.

This looks like it is caused by gradle not passing any system properties to the test sometimes. We use the absence of a system property set by gradle to engage "IDE mode" when running tests which mucks with the security features a bit. If you run tests from gradle in IDE mode they won't work right. Security, at least, won't work right. If these were to happen for other tests then it'd explode in other ways.

I've dug some more. This is caused by us running the tests with the built in gradle test runner rather than the randomized runner. We configure the randomized runner to run with the system properties but we don't ever configure the standard runner.

And some more: this is not caused by the build compare plugin. Maybe by gradle 4.8 or maybe by one of our hacks to make 4.8 work.

Ok - I reverted the 4.8 change locally but kept the other changes and everything looks to work. I don't think we can upgrade to 4.8 until we get to the bottom of this.

Thanks for looking into this @nik9000. I'll create a feature branch.
I won't revert this change just yet, as it does allow us to uncover additional issues, like the CI run here just found, the build hangs, and it's easily reproducible with ./gradlew :example-plugins:painless-whitelist:check.
This upgrade of Gradle and JDK is turning out to be much harder than it should.

I was under the impression that we always replace the Gradle test runner with the randomized one. I wonder if for some reason that doesn't happen, or doesn't always happen for some reason. I did have a suspicion that we are not running with the randomized runner, but when I looked at it I didn't find evidence of this being the case.

dobbbb · 2018-06-18T09:58:52Z

help!!!😭
gradle idea

Where:
Build file '/Users/zhouwuhong/elasticsearch/benchmarks/build.gradle' line: 31
What went wrong:
A problem occurred evaluating project ':benchmarks'.

Failed to apply plugin [id 'elasticsearch.build']
JAVA_HOME must be set to build Elasticsearch

but!!!!!
echo $JAVA_HOME
/Library/Java/JavaVirtualMachines/jdk-10.jdk/Contents/Home

alpar-t · 2018-06-18T14:50:26Z

Closing this as the work was move to feature/31230_gradle48_jdk11 branch.

alpar-t · 2018-06-18T15:46:49Z

@doubleuhy I added that to #31399. Please find the proper channels to communicate. This has nothing to do with this PR, having it hare makes everything more difficult to track and organize PR.

alpar-t added 10 commits June 11, 2018 13:46

Move to Gradle 4.8 RC1

060e00a

Use latest version of plugin

26f0d17

The current does not work with Gradle 4.8 RC1

Switch to Gradle GA

1011582

Add and configure build compare plugin

5445055

add work-around for gradle/gradle#5692

c7338af

work around gradle/gradle#5696

ab67b25

Make use of Gradle build compare with reference project

2e8cb7e

Merge commit 'a178290' into upgrade/gradle_4.8

3a2e7bf

Make the manifest more compare friendly

eec1f43

Clear the manifest in compare friendly mode

d0f7e94

alpar-t added >non-issue :Delivery/Build Build or test infrastructure labels Jun 12, 2018

alpar-t mentioned this pull request Jun 12, 2018

Remove vestiges of animal sniffer #31178

Merged

jasontedor requested a review from nik9000 June 12, 2018 13:36

Remove animalsniffer from buildscript classpath

3f5840a

nik9000 reviewed Jun 12, 2018

View reviewed changes

alpar-t mentioned this pull request Jun 14, 2018

Understand Gradle 4.8 test failure around setup of security policies #31324

Closed

alpar-t added 3 commits June 14, 2018 15:18

Work around wird Security bootstrap issue

e7715c5

elastic#31324

reference Gradle issues in comments

89590fb

Conditionally configure build compare

c406996

alpar-t mentioned this pull request Jun 14, 2018

Changes to support JDK 11 #31333

Closed

alpar-t added 2 commits June 15, 2018 07:53

Merge commit 'fcf1e41' into upgrade/gradle_4.8

09c92de

Extend security fix to versioned jars

e1b579d

fix equals check

2f71509

nik9000 reviewed Jun 15, 2018

View reviewed changes

ywelsch mentioned this pull request Jun 18, 2018

gradle idea failed #31382

Closed

alpar-t closed this Jun 18, 2018

alpar-t deleted the upgrade/gradle_4.8 branch June 18, 2018 14:50

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

Upgrade to Gradle Wrapper 4.8 #31271

Upgrade to Gradle Wrapper 4.8 #31271

alpar-t commented Jun 12, 2018

elasticmachine commented Jun 12, 2018

nik9000 Jun 12, 2018

nik9000 Jun 15, 2018

nik9000 Jun 12, 2018

nik9000 Jun 12, 2018

nik9000 Jun 12, 2018

nik9000 Jun 12, 2018

nik9000 Jun 15, 2018

nik9000 Jun 12, 2018

nik9000 Jun 12, 2018

nik9000 Jun 12, 2018

alpar-t commented Jun 12, 2018

nik9000 commented Jun 12, 2018

alpar-t commented Jun 12, 2018

nik9000 commented Jun 12, 2018

alpar-t commented Jun 13, 2018

alpar-t commented Jun 14, 2018

nik9000 Jun 15, 2018

nik9000 Jun 15, 2018

nik9000 Jun 15, 2018

nik9000 Jun 15, 2018

nik9000 Jun 15, 2018

nik9000 Jun 16, 2018

alpar-t Jun 18, 2018

dobbbb commented Jun 18, 2018

alpar-t commented Jun 18, 2018

alpar-t commented Jun 18, 2018

		@@ -1,6 +1,6 @@
		distributionUrl=https\://services.gradle.org/distributions/gradle-4.7-all.zip
		distributionBase=GRADLE_USER_HOME

Upgrade to Gradle Wrapper 4.8 #31271

Upgrade to Gradle Wrapper 4.8 #31271

Conversation

alpar-t commented Jun 12, 2018

elasticmachine commented Jun 12, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Jun 12, 2018

nik9000 commented Jun 12, 2018

alpar-t commented Jun 12, 2018

nik9000 commented Jun 12, 2018

alpar-t commented Jun 13, 2018

alpar-t commented Jun 14, 2018

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

dobbbb commented Jun 18, 2018

alpar-t commented Jun 18, 2018

alpar-t commented Jun 18, 2018