Skip to content

Upgrade to Gradle Wrapper 4.8 #31271

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.

Sign up for GitHub

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

Closed
wants to merge 17 commits into from
Closed

Upgrade to Gradle Wrapper 4.8 #31271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
060e00a
Move to Gradle 4.8 RC1
alpar-t May 18, 2018
26f0d17
Use latest version of plugin
alpar-t May 18, 2018
1011582
Switch to Gradle GA
alpar-t Jun 11, 2018
5445055
Add and configure build compare plugin
alpar-t Jun 11, 2018
c7338af
add work-around for https://github.com/gradle/gradle/issues/5692
alpar-t Jun 11, 2018
ab67b25
work around https://github.com/gradle/gradle/issues/5696
alpar-t Jun 12, 2018
2e8cb7e
Make use of Gradle build compare with reference project
alpar-t Jun 12, 2018
3a2e7bf
Merge commit 'a178290' into upgrade/gradle_4.8
alpar-t Jun 12, 2018
eec1f43
Make the manifest more compare friendly
alpar-t Jun 12, 2018
d0f7e94
Clear the manifest in compare friendly mode
alpar-t Jun 12, 2018
3f5840a
Remove animalsniffer from buildscript classpath
alpar-t Jun 12, 2018
e7715c5
Work around wird Security bootstrap issue
alpar-t Jun 14, 2018
89590fb
reference Gradle issues in comments
alpar-t Jun 14, 2018
c406996
Conditionally configure build compare
alpar-t Jun 14, 2018
09c92de
Merge commit 'fcf1e41' into upgrade/gradle_4.8
alpar-t Jun 15, 2018
e1b579d
Extend security fix to versioned jars
alpar-t Jun 15, 2018
2f71509
fix equals check
alpar-t Jun 15, 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
26 changes: 26 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,3 +575,29 @@ gradle.projectsEvaluated {
}
}
}

apply plugin: 'compare-gradle-builds'
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


if (System.properties.get("build.compare") != null) {
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
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

👍

}

/*****************************************************************************
Expand Down
28 changes: 20 additions & 8 deletions buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,11 @@ class BuildPlugin implements Plugin<Project> {
// just a self contained test-fixture configuration, likely transitive and hellacious
return
}
configuration.resolutionStrategy.failOnVersionConflict()
configuration.resolutionStrategy {
failOnVersionConflict()
// work around https://github.com/gradle/gradle/issues/5692
preferProjectModules()
}
})

// force all dependencies added directly to compile/testCompile to be non-transitive, except for ES itself
Expand Down Expand Up @@ -475,13 +479,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 +633,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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (System.properties.getProperty("build.compare_friendly", "false") == "true") {
jarTask.manifest.getAttributes().clear()
}
}
// add license/notice files
project.afterEvaluate {
Expand Down
Binary file modified gradle/wrapper/gradle-wrapper.jar
View file
Open in desktop
Binary file not shown.
6 changes: 3 additions & 3 deletions gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

👍

distributionPath=wrapper/dists
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.8-all.zip
zipStoreBase=GRADLE_USER_HOME
distributionSha256Sum=203f4537da8b8075e38c036a6d14cb71b1149de5bf0a8f6db32ac2833a1d1294
zipStorePath=wrapper/dists
distributionSha256Sum=da9600da2a28a43f5f77364deecbb9b01c1ddb7d3ecafe1d5c93bcd8a8059ab1
6 changes: 4 additions & 2 deletions server/src/main/java/org/elasticsearch/bootstrap/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait, I just got it! Neat.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (previous != null && url.toString().equals(previous) == false) {
throw new IllegalStateException("codebase property already set: " + aliasProperty + " -> " + previous +
", cannot set to " + url.toString());
}
}
propertiesSet.add(property);
String previous = System.setProperty(property, url.toString());
if (previous != null) {
// allow to re-set to what was previously there. https://github.com/elastic/elasticsearch/issues/31324
if (previous != null && url.toString().equals(previous) == false) {
throw new IllegalStateException("codebase property already set: " + property + " -> " + previous +
", cannot set to " + url.toString());
}
Expand Down