Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade gradle wrapper to 4.8 #31525

Merged
merged 29 commits into from
Jun 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
70effe4
Move to Gradle 4.8 RC1
alpar-t May 18, 2018
ac5b3b1
Use latest version of plugin
alpar-t May 18, 2018
541ae72
Switch to Gradle GA
alpar-t Jun 11, 2018
fc0667d
Add and configure build compare plugin
alpar-t Jun 11, 2018
b0c9ff4
add work-around for https://github.com/gradle/gradle/issues/5692
alpar-t Jun 11, 2018
b18dc30
work around https://github.com/gradle/gradle/issues/5696
alpar-t Jun 12, 2018
d6479de
Make use of Gradle build compare with reference project
alpar-t Jun 12, 2018
bbb2f9a
Make the manifest more compare friendly
alpar-t Jun 12, 2018
302f171
Clear the manifest in compare friendly mode
alpar-t Jun 12, 2018
50b7c3c
Remove animalsniffer from buildscript classpath
alpar-t Jun 12, 2018
db8ebd3
reference Gradle issues in comments
alpar-t Jun 14, 2018
869a84b
Conditionally configure build compare
alpar-t Jun 14, 2018
a54139c
Fix javadoc errors
alpar-t Jun 12, 2018
db92ced
Fix doc issues
alpar-t Jun 13, 2018
93213ab
Fix some more doclint issues
alpar-t Jun 18, 2018
46fefdc
fix typo in build script
alpar-t Jun 19, 2018
5e53dee
Add sanity check to make sure the test task was replaced
alpar-t Jun 19, 2018
9eaa07e
Include number of non conforming tasks in the exception.
alpar-t Jun 19, 2018
6ccfec8
No longer replace test task, create implicit instead
alpar-t Jun 20, 2018
e141e4b
Revert "No longer replace test task, create implicit instead"
alpar-t Jun 21, 2018
6ae2a7b
Fix replacement of the test task
alpar-t Jun 21, 2018
05b966c
Only apply build comapare plugin if needed
alpar-t Jun 21, 2018
ba43cc1
Make sure test runs before integTest
alpar-t Jun 21, 2018
505ab68
Fix doclint aftter merge
alpar-t Jun 21, 2018
f2edc18
PR review comments
alpar-t Jun 25, 2018
e648dce
Switch to Gradle 4.8.1 and remove workaround
alpar-t Jun 22, 2018
21c72f9
PR review comments
alpar-t Jun 26, 2018
ca88789
Consolidate task ordering
alpar-t Jun 27, 2018
ca48ace
Merge commit '4fc833b1decd71b5d3ded54d3a5ee2411bf34e00' into 31230_gr…
alpar-t Jun 27, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmarks/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ buildscript {

apply plugin: 'elasticsearch.build'

// order of this seciont matters, see: https://github.com/johnrengelman/shadow/issues/336
// order of this section matters, see: https://github.com/johnrengelman/shadow/issues/336
apply plugin: 'application' // have the shadow plugin provide the runShadow task
mainClassName = 'org.openjdk.jmh.Main'
apply plugin: 'com.github.johnrengelman.shadow' // build an uberjar with all benchmarks
Expand Down
28 changes: 28 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,9 @@ gradle.projectsEvaluated {
// :test:framework:test cannot run before and after :server:test
return
}
if (tasks.findByPath('test') != null && tasks.findByPath('integTest') != null) {
integTest.mustRunAfter test
}
configurations.all { Configuration configuration ->
/*
* The featureAwarePlugin configuration has a dependency on x-pack:plugin:core and x-pack:plugin:core has a dependency on the
Expand Down Expand Up @@ -575,3 +578,28 @@ gradle.projectsEvaluated {
}
}
}

if (System.properties.get("build.compare") != null) {
apply plugin: 'compare-gradle-builds'
compareGradleBuilds {
ext.referenceProject = System.properties.get("build.compare")
doFirst {
if (file(referenceProject).exists() == false) {
throw new GradleException(
"Use git worktree to check out a version to compare against to ../elasticsearch_build_reference"
)
}
}
sourceBuild {
gradleVersion = "4.7" // does not default to gradle weapper of project dir, but current version
projectDir = referenceProject
tasks = ["clean", "assemble"]
arguments = ["-Dbuild.compare_friendly=true"]
}
targetBuild {
tasks = ["clean", "assemble"]
// use -Dorg.gradle.java.home= to alter jdk versions
arguments = ["-Dbuild.compare_friendly=true"]
}
}
}
1 change: 0 additions & 1 deletion buildSrc/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

/*****************************************************************************
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,44 @@
package com.carrotsearch.gradle.junit4

import com.carrotsearch.ant.tasks.junit4.JUnit4
import org.gradle.api.AntBuilder
import org.gradle.api.GradleException
import org.gradle.api.Plugin
import org.gradle.api.Project
import org.gradle.api.Task
import org.gradle.api.UnknownTaskException
import org.gradle.api.plugins.JavaBasePlugin
import org.gradle.api.tasks.TaskContainer
import org.gradle.api.tasks.TaskProvider
import org.gradle.api.tasks.testing.Test

import java.util.concurrent.atomic.AtomicBoolean

class RandomizedTestingPlugin implements Plugin<Project> {

static private AtomicBoolean sanityCheckConfigured = new AtomicBoolean(false)

void apply(Project project) {
setupSeed(project)
replaceTestTask(project.tasks)
configureAnt(project.ant)
configureSanityCheck(project)
}

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

project.rootProject.getGradle().getTaskGraph().whenReady {
List<Task> nonConforming = project.getGradle().getTaskGraph().allTasks
.findAll { it.name == "test" }
.findAll { (it instanceof RandomizedTestingTask) == false}
.collect { "${it.path} -> ${it.class}" }
if (nonConforming.isEmpty() == false) {
throw new GradleException("Found the ${nonConforming.size()} `test` tasks:" +
"\n ${nonConforming.join("\n ")}")
}
}
}
}

/**
Expand Down Expand Up @@ -45,29 +69,32 @@ class RandomizedTestingPlugin implements Plugin<Project> {
}

static void replaceTestTask(TaskContainer tasks) {
Test oldTestTask = tasks.findByPath('test')
if (oldTestTask == null) {
// Gradle 4.8 introduced lazy tasks, thus we deal both with the `test` task as well as it's provider
// https://github.com/gradle/gradle/issues/5730#issuecomment-398822153
// since we can't be sure if the task was ever realized, we remove both the provider and the task
TaskProvider<Test> oldTestProvider
try {
oldTestProvider = tasks.getByNameLater(Test, 'test')
} catch (UnknownTaskException unused) {
// no test task, ok, user will use testing task on their own
return
}
tasks.remove(oldTestTask)
Test oldTestTask = oldTestProvider.get()

Map properties = [
name: 'test',
type: RandomizedTestingTask,
dependsOn: oldTestTask.dependsOn,
group: JavaBasePlugin.VERIFICATION_GROUP,
description: 'Runs unit tests with the randomized testing framework'
]
RandomizedTestingTask newTestTask = tasks.create(properties)
newTestTask.classpath = oldTestTask.classpath
newTestTask.testClassesDir = oldTestTask.project.sourceSets.test.output.classesDir
// since gradle 4.5, tasks immutable dependencies are "hidden" (do not show up in dependsOn)
// so we must explicitly add a dependency on generating the test classpath
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)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need remove at all then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

newTestTask.configure{
group = JavaBasePlugin.VERIFICATION_GROUP
description = 'Runs unit tests with the randomized testing framework'
dependsOn oldTestTask.dependsOn, 'testClasses'
classpath = oldTestTask.classpath
testClassesDir = oldTestTask.project.sourceSets.test.output.classesDir
}

// hack so check task depends on custom test
Task checkTask = tasks.findByPath('check')
Task checkTask = tasks.getByName('check')
checkTask.dependsOn.remove(oldTestProvider)
checkTask.dependsOn.remove(oldTestTask)
checkTask.dependsOn.add(newTestTask)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
})

// force all dependencies added directly to compile/testCompile to be non-transitive, except for ES itself
Expand Down Expand Up @@ -475,13 +477,17 @@ class BuildPlugin implements Plugin<Project> {
}
}

project.tasks.withType(GenerateMavenPom.class) { GenerateMavenPom t ->
// place the pom next to the jar it is for
t.destination = new File(project.buildDir, "distributions/${project.archivesBaseName}-${project.version}.pom")
// build poms with assemble (if the assemble task exists)
Task assemble = project.tasks.findByName('assemble')
if (assemble) {
assemble.dependsOn(t)
// Work around Gradle 4.8 issue until we `enableFeaturePreview('STABLE_PUBLISHING')`
// https://github.com/gradle/gradle/issues/5696#issuecomment-396965185
project.getGradle().getTaskGraph().whenReady {
project.tasks.withType(GenerateMavenPom.class) { GenerateMavenPom t ->
// place the pom next to the jar it is for
t.destination = new File(project.buildDir, "distributions/${project.archivesBaseName}-${project.version}.pom")
// build poms with assemble (if the assemble task exists)
Task assemble = project.tasks.findByName('assemble')
if (assemble) {
assemble.dependsOn(t)
}
}
}
}
Expand Down Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@alpar-t alpar-t Jun 27, 2018

Choose a reason for hiding this comment

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

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.

}
}
// add license/notice files
project.afterEvaluate {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1010,8 +1010,8 @@ public int compareTo(DeadNode rhs) {
}

/**
* Adapts an <code>Iterator<DeadNodeAndRevival></code> into an
* <code>Iterator<Node></code>.
* Adapts an <code>Iterator&lt;DeadNodeAndRevival&gt;</code> into an
* <code>Iterator&lt;Node&gt;</code>.
*/
private static class DeadNodeIteratorAdapter implements Iterator<Node> {
private final Iterator<DeadNode> itr;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ public void testBody() throws IOException {
}

/**
* @deprecated will remove method in 7.0 but needs tests until then. Replaced by {@link RequestTests#testAddHeaders()}.
* @deprecated will remove method in 7.0 but needs tests until then. Replaced by {@link RequestTests}.
*/
@Deprecated
public void tesPerformRequestOldStyleNullHeaders() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public void onFailure(Exception exception) {
}

/**
* @deprecated will remove method in 7.0 but needs tests until then. Replaced by {@link RequestTests#testAddHeader()}.
* @deprecated will remove method in 7.0 but needs tests until then. Replaced by {@link RequestTests}.
*/
@Deprecated
public void testPerformOldStyleAsyncWithNullHeaders() throws Exception {
Expand Down
Binary file modified gradle/wrapper/gradle-wrapper.jar
Binary file not shown.
6 changes: 3 additions & 3 deletions gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
distributionUrl=https\://services.gradle.org/distributions/gradle-4.7-all.zip
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.8.1-all.zip
zipStoreBase=GRADLE_USER_HOME
distributionSha256Sum=203f4537da8b8075e38c036a6d14cb71b1149de5bf0a8f6db32ac2833a1d1294
zipStorePath=wrapper/dists
distributionSha256Sum=ce1645ff129d11aad62dab70d63426fdce6cfd646fa309dc5dc5255dd03c7c11
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/search/SearchHit.java
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,8 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t
* This parser outputs a temporary map of the objects needed to create the
* SearchHit instead of directly creating the SearchHit. The reason for this
* is that this way we can reuse the parser when parsing xContent from
* {@link CompletionSuggestion.Entry.Option} which unfortunately inlines the
* output of
* {@link org.elasticsearch.search.suggest.completion.CompletionSuggestion.Entry.Option} which unfortunately inlines
* the output of
* {@link #toInnerXContent(XContentBuilder, org.elasticsearch.common.xcontent.ToXContent.Params)}
* of the included search hit. The output of the map is used to create the
* actual SearchHit instance via {@link #createFromMap(Map)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public void collect(int doc, long bucket) throws IOException {

/**
* Replay the documents that might contain a top bucket and pass top buckets to
* the {@link this#deferredCollectors}.
* the {@link #deferredCollectors}.
*/
private void runDeferredCollections() throws IOException {
final boolean needsScores = needsScores();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ final class CompositeValuesCollectorQueue implements Releasable {
*
* @param sources The list of {@link CompositeValuesSourceConfig} to build the composite buckets.
* @param size The number of composite buckets to keep.
* @param afterKey
* @param afterKey composite key
*/
CompositeValuesCollectorQueue(BigArrays bigArrays, SingleDimensionValuesSource<?>[] sources, int size, CompositeKey afterKey) {
this.bigArrays = bigArrays;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ abstract class SingleDimensionValuesSource<T extends Comparable<T>> implements R
* The current value is filled by a {@link LeafBucketCollector} that visits all the
* values of each document. This method saves this current value in a slot and should only be used
* in the context of a collection.
* See {@link this#getLeafCollector}.
* See {@link #getLeafCollector}.
*/
abstract void copyCurrent(int slot);

Expand All @@ -87,15 +87,15 @@ abstract class SingleDimensionValuesSource<T extends Comparable<T>> implements R
* The current value is filled by a {@link LeafBucketCollector} that visits all the
* values of each document. This method compares this current value with the value present in
* the provided slot and should only be used in the context of a collection.
* See {@link this#getLeafCollector}.
* See {@link #getLeafCollector}.
*/
abstract int compareCurrent(int slot);

/**
* The current value is filled by a {@link LeafBucketCollector} that visits all the
* values of each document. This method compares this current value with the after value
* set on this source and should only be used in the context of a collection.
* See {@link this#getLeafCollector}.
* See {@link #getLeafCollector}.
*/
abstract int compareCurrentWithAfter();

Expand All @@ -120,7 +120,7 @@ T getAfter() {
* Creates a {@link LeafBucketCollector} that extracts all values from a document and invokes
* {@link LeafBucketCollector#collect} on the provided <code>next</code> collector for each of them.
* The current value of this source is set on each call and can be accessed by <code>next</code> via
* the {@link this#copyCurrent(int)} and {@link this#compareCurrent(int)} methods. Note that these methods
* the {@link #copyCurrent(int)} and {@link #compareCurrent(int)} methods. Note that these methods
* are only valid when invoked from the {@link LeafBucketCollector} created in this source.
*/
abstract LeafBucketCollector getLeafCollector(LeafReaderContext context, LeafBucketCollector next) throws IOException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ public void testEnsureNoSelfReferences() throws IOException {

/**
* Test that the same map written multiple times do not trigger the self-reference check in
* {@link CollectionUtils#ensureNoSelfReferences(Object)}
* {@link CollectionUtils#ensureNoSelfReferences(Object, String)} (Object)}
*/
public void testRepeatedMapsAndNoSelfReferences() throws Exception {
Map<String, Object> mapB = singletonMap("b", "B");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@

/**
* Mappings. Mappings define the way that documents should be translated to
* Lucene indices, for instance how the
* {@link org.elasticsearch.index.mapper.UidFieldMapper document identifier}
* should be indexed, whether a string field should be indexed as a
* Lucene indices, for instance whether a string field should be indexed as a
* {@link org.elasticsearch.index.mapper.TextFieldMapper text} or
* {@link org.elasticsearch.index.mapper.KeywordFieldMapper keyword} field,
* etc. This parsing is done by the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1345,8 +1345,6 @@ public void testExceptionOnNegativeInterval() {
* https://github.com/elastic/elasticsearch/issues/31392 demonstrates an edge case where a date field mapping with
* "format" = "epoch_millis" can lead for the date histogram aggregation to throw an error if a non-UTC time zone
* with daylight savings time is used. This test was added to check this is working now
* @throws ExecutionException
* @throws InterruptedException
*/
public void testRewriteTimeZone_EpochMillisFormat() throws InterruptedException, ExecutionException {
String index = "test31392";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ protected IndexShard newShard(ShardRouting routing, IndexMetaData indexMetaData,
* @param indexMetaData indexMetaData for the shard, including any mapping
* @param indexSearcherWrapper an optional wrapper to be used during searchers
* @param globalCheckpointSyncer callback for syncing global checkpoints
* @param indexEventListener
* @param indexEventListener index even listener
* @param listeners an optional set of listeners to add to the shard
*/
protected IndexShard newShard(ShardRouting routing, ShardPath shardPath, IndexMetaData indexMetaData,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ protected String[] shuffleProtectedFields() {
* To find the right position in the root query, we add a marker as `queryName` which
* all query builders support. The added bogus field after that should trigger the exception.
* Queries that allow arbitrary field names at this level need to override this test.
* @throws IOException
*/
public void testUnknownField() throws IOException {
String marker = "#marker#";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ final TimeValue delay(long expirationDate, long now) {
}

/**
* {@link SchedulerEngine.Schedule#nextScheduledTimeAfter(long, long)} with respect to
* license expiry date
* {@link org.elasticsearch.xpack.core.scheduler.SchedulerEngine.Schedule#nextScheduledTimeAfter(long, long)}
* with respect to license expiry date
*/
public final long nextScheduledTimeForExpiry(long expiryDate, long startTime, long time) {
TimeValue delay = delay(expiryDate, time);
Expand Down Expand Up @@ -169,4 +169,4 @@ public final String toString() {
orientation.name(), TimeValue.timeValueMillis(min), TimeValue.timeValueMillis(max),
TimeValue.timeValueMillis(frequency));
}
}
}
Loading