Upgrade to Gradle 5.0 #34263
Upgrade to Gradle 5.0 #34263
Conversation
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
I have one question, but I also don't think we should upgrade to a milestone release. This is like upgrading to a beta.
| } | ||
|
|
||
| static Task configureInstallPluginTask(String name, Project project, Task setup, NodeInfo node, String pluginName, String prefix) { | ||
| final FileCollection pluginZip; |
There was a problem hiding this comment.
I will not merge the PR until there is a GA, just wanted to start running in CI.
Gradle bumps the Groovy version, which now complains that the final variable is changed.
It doesn't see past branches the same way java does. One more reason to switch to java.
alpar-t
force-pushed
the
upgrade-gradle-5
branch
from
dabae45 to
2d5d468
Compare
|
@elasticmachine test this please |
|
@elasticmachine test this please |
Looks like Gradle changed the order in which things happen here. There were no guarantees on ordering before, the order just happened to be right.
| * @param includePackagedTests true if the packaged tests should be copied, false otherwise | ||
| */ | ||
| static Task createCopyRestSpecTask(Project project, Provider<Boolean> includePackagedTests) { | ||
| Task createCopyRestSpecTask() { |
There was a problem hiding this comment.
The static method was called with the value of includePackagedTests in what happened to be the right order so that changes to the property were picked up in time and the right value was passed into the project.afterEvaluate ( which had no use, since the value was already picked up anyhow ).
This can't be static to make changes to the property reflected as they should. Gradle 5 seems to have changed the order in which some operations are executed. I wish they would have an option to randomize ordering of otherwise undefined sequences.
|
@rjernst Gradle 5.0 is GA and this is ready for review and merge now. |
|
One thing to note is that in release manager Gradle will run with ml-cpp and found-elasticsearch-plugin in the elasticsearch-extra directory. Were there any particular things that needed changing as part of this PR that are also likely to need changing in the |
|
@droberts195 The best thing is to run with a build scan and look at deprecation messages. |
Needs to be merged before elastic/elasticsearch#34263 to prevent builds with ml-cpp in elasticsearch-extra from failing. Defining a custom wrapper task was incompatible with Gradle 5, so this is replaced with a modification to the built in wrapper task. The Gradle Wrapper is also upgraded to the latest version.
Needs to be merged before elastic/elasticsearch#34263 to prevent builds with ml-cpp in elasticsearch-extra from failing. Defining a custom wrapper task was incompatible with Gradle 5, so this is replaced with a modification to the built in wrapper task. The Gradle Wrapper is also upgraded to the latest version.
Needs to be merged before elastic/elasticsearch#34263 to prevent builds with ml-cpp in elasticsearch-extra from failing. Defining a custom wrapper task was incompatible with Gradle 5, so this is replaced with a modification to the built in wrapper task. The Gradle Wrapper is also upgraded to the latest version.
Needs to be merged before elastic/elasticsearch#34263 to prevent builds with ml-cpp in elasticsearch-extra from failing. Defining a custom wrapper task was incompatible with Gradle 5, so this is replaced with a modification to the built in wrapper task. The Gradle Wrapper is also upgraded to the latest version. Backport of #325
|
I can confirm that ml-cpp now works with Gradle 5 |
| project.afterEvaluate { | ||
| copyRestSpec.from({ project.zipTree(project.configurations.restSpec.singleFile) }) { | ||
| include 'rest-api-spec/api/**' | ||
| if (includePackagedTests.get()) { |
There was a problem hiding this comment.
This reversion of using Property works here, because we are in an afterEvaluate. However, the original intention was to configure the copy spec outside of afterEvalute, so the Property would be needed. The reason the afterEvaluate still exists is because, for some reason I could not explain, the zipTree seems to always be evaluated when copyRestSpec.from is called, even though we pass a closure. I bring this up because I see a similar copy.from added below with a similar lazily evaluated zip, which I assume works, so maybe this afterEvaluate could be removed (in a followup), but then this would need to be a Property again.
|
|
This pull request fixes the build.gradle file so that it works with the Gradle 5.0 update (#34263). It also makes use of the ignore: 404 header in REST tests so that previous snapshots are deleted as part of the test set up. This way each new test run with an external service should start with a fresh repository even if the previous test run failed and left snapshots in the repo. (This is a best effort as it does not fix a corrupted repository)
This pull request fixes the build.gradle file so that it works with the Gradle 5.0 update (#34263). It also makes use of the ignore: 404 header in REST tests so that previous snapshots are deleted as part of the test set up. This way each new test run with an external service should start with a fresh repository even if the previous test run failed and left snapshots in the repo. (This is a best effort as it does not fix a corrupted repository)
6 participants
It seems like the latest version brought along by Gradle changes how the
finalkeyword is interpreted,unfortunately it can't see past branches so it doesn't work like ti would in java.
This is a work in progress to get the tests running. Will only merge when Gradle 5.0 is GA.
Release Notes: https://github.com/gradle/gradle/releases/tag/v5.0.0