Upgrade gradle wrapper to 4.8 #31525

alpar-t · 2018-06-22T09:32:43Z

Switch to gradle 4.8 as discussed in #31230.

Includes:

workaround Gradle issues
fix the test task replacement that broke in 4.8 and add a sanity check to detect if it happens again
add the possibility to compare build artifacts from builds. Useful to check that the changes to the build script did not cause untended changes for the artifacts. This is enabled with a property.

Not strictly Gradle 4.8 but included here as some of the work was done in conjunction with JDK 11:

javadoc corrections for problems detected by the newer version of doclint
rule in Gradle to always run test before integTest if both are available.

Does not include:

remove usage of features deprecated in 4.8

The current does not work with Gradle 4.8 RC1

Relates to elastic#31324. It seems like Gradle has an inconsistent behavior and the taks is not always replaced.

Closes elastic#31324. The issue has full context in comments. With this change the `test` task becomes nothing more than an alias for `utest`. Some of the stand alone tests that had a `test` task now have `integTest`, and a few of them that used to have `integTest` to run multiple tests now only have `check`. This will also help separarate unit/micro tests from integration tests.

This reverts commit f1ebaf7.

Based on information from gradle/gradle#5730 replace the task taking into account the task providres. Closes elastic#31324.

elasticmachine · 2018-06-22T09:32:45Z

Pinging @elastic/es-core-infra

alpar-t · 2018-06-22T12:12:29Z

@nik9000 @rjernst looking forward to your reviews

nik9000 · 2018-06-22T15:57:56Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy

+        // https://github.com/elastic/elasticsearch/issues/31324
+        if (sanityCheckConfigured.getAndSet(true) == false) {
+            project.rootProject.getGradle().getTaskGraph().whenReady {
+                def nonConforming = project.getGradle().getTaskGraph().allTasks


List instead of def? We try to be explicit in types to help those of us used to Java.

nik9000 · 2018-06-22T15:59:59Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy

-        newTestTask.dependsOn('testClasses')
+        // we still have to use replace here despite the remove above because the task container knows about the provider
+        // by the same name
+        RandomizedTestingTask newTestTask = tasks.replace('test', RandomizedTestingTask)


Do we need remove at all then?

Indeed, we don't need to remove with replace.

nik9000

I left two minor things. LGTM otherwise. No need for another round of review I think.

nik9000 · 2018-06-26T14:09:39Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy

-import org.gradle.api.Plugin
-import org.gradle.api.Project
-import org.gradle.api.Task
+import org.gradle.api.*


We don't allow * imports in Elasticsearch's code proper because they can make it more difficult to track down where things come from. We probably shouldn't have them here either.

nik9000 · 2018-06-26T14:11:14Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy

+        try {
+            oldTestProvider = tasks.getByNameLater(Test, 'test')
+        }
+        catch (UnknownTaskException why) {


Could you rename the exception to something more descriptive? why makes me think we don't know why the exception is thrown. Also it'd be lovely if you'd move the catch up to the } line.

rjernst

This looks good. I left a couple minor nits and questions.

rjernst · 2018-06-26T14:35:53Z

build.gradle

+
+// make sure integration tests run after unit tests
+allprojects {
+  if (tasks.findByPath('test') != null && tasks.findByPath('integTest') != null) {


Can you put this together in the block around line 319, which also deals with test ordering? What causes this to be a requirement for upgrading to 4.8? Also, separately, I think we should look to make most of these mustRunAfter into shouldRunAfter to allow parallelization.

++ on souldRunAfter. It's not a necessity for Gradle 4.8 but I did find it to be useful when debugging the many problems we had with Gradle 4.8.

rjernst · 2018-06-26T14:37:05Z

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy

+    private static void configureSanityCheck(Project project) {
+        // Check the task graph to confirm tasks were indeed replaced
+        // https://github.com/elastic/elasticsearch/issues/31324
+        if (sanityCheckConfigured.getAndSet(true) == false) {


Why is this atomic needed? When is the project ever applied more than once? I think a lot of our code would be broken if it were possible for multiple invocations.

The plugins is applied once per project, but it's done so on multiple projects.
Since there's a single task graph for all projects, and we must run the check on the task graph rather than the task collection ( as we ween the lather can look right and the former still has the wrong tasks ) we must make sure it's only done once.

rjernst · 2018-06-26T14:40:29Z

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

@@ -348,7 +348,9 @@ class BuildPlugin implements Plugin<Project> {
                // just a self contained test-fixture configuration, likely transitive and hellacious
                return
            }
-            configuration.resolutionStrategy.failOnVersionConflict()
+            configuration.resolutionStrategy { 
+                failOnVersionConflict()


Did this change out of necessity? It was this way before to match what would be done in java (no closures in java).

It's more for historic reasons, there was a work-around here with 4.8 that's no longer required in 4.8.1. With Java 8, as there's a functional interface Configuration resolutionStrategy(Action<? super ResolutionStrategy> action); so the closure can be replaced with a lambda. The Gradle API has both closures and functional interfaces in many places.

I'm not entirely convinced about how useful keeping Java like syntax is Groovy will be in aiding conversion after having converted a few files in a different PR. Dynamic Groovy properties are by far harder to deal with than syntax. I would rater adopt a strategy going forward to convert files before doing any edits rather than caring for this syntax, it requires too much attention without it resulting in the end goal.

rjernst · 2018-06-26T14:43:18Z

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

@@ -625,6 +631,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
+                if (System.properties.getProperty("build.compare_friendly", "false") == "true") {
+                    jarTask.manifest.getAttributes().clear()


Can you elaborate more on this comment? I am concerned debug ability of a build that lacks any of the manifest info we inject.

This property should never be passed directly. It's being used by the build compare plugin.
Unfortunately this useful plugin is still missing a lot of features, like understanding that fields in manifest files will change between builds, so with this property we prevent this to reduce noise in build compare. I'm not happy about what this property does either.
It will backfire, but it does require one to pull the trigger.

…adle48

* Move to Gradle 4.8 RC1 * Use latest version of plugin The current does not work with Gradle 4.8 RC1 * Switch to Gradle GA * Add and configure build compare plugin * add work-around for gradle/gradle#5692 * work around gradle/gradle#5696 * Make use of Gradle build compare with reference project * Make the manifest more compare friendly * Clear the manifest in compare friendly mode * Remove animalsniffer from buildscript classpath * Fix javadoc errors * Fix doc issues * reference Gradle issues in comments * Conditionally configure build compare * Fix some more doclint issues * fix typo in build script * Add sanity check to make sure the test task was replaced Relates to #31324. It seems like Gradle has an inconsistent behavior and the taks is not always replaced. * Include number of non conforming tasks in the exception. * No longer replace test task, create implicit instead Closes #31324. The issue has full context in comments. With this change the `test` task becomes nothing more than an alias for `utest`. Some of the stand alone tests that had a `test` task now have `integTest`, and a few of them that used to have `integTest` to run multiple tests now only have `check`. This will also help separarate unit/micro tests from integration tests. * Revert "No longer replace test task, create implicit instead" This reverts commit f1ebaf7. * Fix replacement of the test task Based on information from gradle/gradle#5730 replace the task taking into account the task providres. Closes #31324. * Only apply build comapare plugin if needed * Make sure test runs before integTest * Fix doclint aftter merge * PR review comments * Switch to Gradle 4.8.1 and remove workaround * PR review comments * Consolidate task ordering

* master: Docs: Remove duplicate test setup Print output when the name checker IT fails (#31660) Fix syntax errors in get-snapshots docs (#31656) Docs: Fix description of percentile ranks example example (#31652) Add MultiSearchTemplate support to High Level Rest client (#30836) Add test for low-level client round-robin behaviour (#31616) SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Correct integTest enable logic (#31646) Fix missing get-snapshots docs reference #31645 Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) Add Get Snapshots High Level REST API (#31537) QA: Merge query-builder-bwc to restart test (#30979) Update reindex.asciidoc (#31626) Docs: Skip xpack snippet tests if no xpack (#31619) mute CreateSnapshotRequestTests HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (#31215) Remove legacy MetaDataStateFormat (#31603) Add explain API to high-level REST client (#31387) Preserve thread context when connecting to remote cluster (#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (#31494) [TEST] call yaml client close method from test suite (#31591)

* 6.x: Docs: Remove duplicate test setup Docs: Fix description of percentile ranks example example (#31652) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Remove item listed in 6.3 notes that's not in 6.3 (#31623) remove unused import Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) QA: Merge query-builder-bwc to restart test (#30979) Docs: Skip xpack snippet tests if no xpack (#31619) [TEST] Fix RemoteClusterConnectionTests Remove legacy MetaDataStateFormat (#31603) [TEST] call yaml client close method from test suite (#31591) [TEST] Close additional clients created while running yaml tests (#31575) Node selector per client rather than per request (#31471) Preserve thread context when connecting to remote cluster (#31574) Remove redundant 'minimum_should_match' Unify headers for full text queries JDBC driver prepared statement set* methods (#31494) add logging breaking changes from 5.3 to 6.0 (#31583) Fix a formatting issue in the docvalue_fields documentation. (#31563) Improve robustness of geo shape parser for malformed shapes QA: Create xpack yaml features (#31403)

alpar-t added 24 commits June 22, 2018 09:39

Move to Gradle 4.8 RC1

70effe4

Use latest version of plugin

ac5b3b1

The current does not work with Gradle 4.8 RC1

Switch to Gradle GA

541ae72

Add and configure build compare plugin

fc0667d

add work-around for gradle/gradle#5692

b0c9ff4

work around gradle/gradle#5696

b18dc30

Make use of Gradle build compare with reference project

d6479de

Make the manifest more compare friendly

bbb2f9a

Clear the manifest in compare friendly mode

302f171

Remove animalsniffer from buildscript classpath

50b7c3c

reference Gradle issues in comments

db8ebd3

Conditionally configure build compare

869a84b

Fix javadoc errors

a54139c

Fix doc issues

db92ced

Fix some more doclint issues

93213ab

fix typo in build script

46fefdc

Add sanity check to make sure the test task was replaced

5e53dee

Relates to elastic#31324. It seems like Gradle has an inconsistent behavior and the taks is not always replaced.

Include number of non conforming tasks in the exception.

9eaa07e

Revert "No longer replace test task, create implicit instead"

e141e4b

This reverts commit f1ebaf7.

Fix replacement of the test task

6ae2a7b

Based on information from gradle/gradle#5730 replace the task taking into account the task providres. Closes elastic#31324.

Only apply build comapare plugin if needed

05b966c

Make sure test runs before integTest

ba43cc1

Fix doclint aftter merge

505ab68

alpar-t added the :Delivery/Build Build or test infrastructure label Jun 22, 2018

nik9000 reviewed Jun 22, 2018

View reviewed changes

alpar-t added 2 commits June 25, 2018 15:45

PR review comments

f2edc18

Switch to Gradle 4.8.1 and remove workaround

e648dce

alpar-t changed the title ~~31230 gradle48~~ Upgrade gradle wrapper to 4.8 Jun 26, 2018

nik9000 approved these changes Jun 26, 2018

View reviewed changes

nik9000 added v7.0.0 v6.4.0 labels Jun 26, 2018

PR review comments

21c72f9

rjernst approved these changes Jun 26, 2018

View reviewed changes

alpar-t added 2 commits June 27, 2018 11:13

Consolidate task ordering

ca88789

Merge commit '4fc833b1decd71b5d3ded54d3a5ee2411bf34e00' into 31230_gr…

ca48ace

…adle48

alpar-t merged commit 8557bba into elastic:master Jun 28, 2018

alpar-t deleted the 31230_gradle48 branch June 28, 2018 05:13

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

Upgrade gradle wrapper to 4.8 #31525

Upgrade gradle wrapper to 4.8 #31525

alpar-t commented Jun 22, 2018

elasticmachine commented Jun 22, 2018

alpar-t commented Jun 22, 2018

nik9000 Jun 22, 2018

nik9000 Jun 22, 2018

alpar-t Jun 25, 2018

nik9000 left a comment

nik9000 Jun 26, 2018

nik9000 Jun 26, 2018

rjernst left a comment

rjernst Jun 26, 2018

alpar-t Jun 27, 2018

rjernst Jun 26, 2018

alpar-t Jun 27, 2018

rjernst Jun 26, 2018

alpar-t Jun 27, 2018

rjernst Jun 26, 2018

alpar-t Jun 27, 2018 •

edited

Loading

Upgrade gradle wrapper to 4.8 #31525

Upgrade gradle wrapper to 4.8 #31525

Conversation

alpar-t commented Jun 22, 2018

elasticmachine commented Jun 22, 2018

alpar-t commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

alpar-t Jun 27, 2018 •

edited

Loading