Determine the minimum gradle version based on the wrapper #32226
Conversation
This is restrictive and forces users of the plugin to move together with us, but without integration tests it's close to impossible to make sure that the claimed compatability is really there. If we do want to offer more flexibility, we should add those tests first.
|
Pinging @elastic/es-core-infra |
buildSrc/build.gradle
Outdated
| @@ -78,6 +83,7 @@ task writeVersionProperties { | |||
| processResources { | |||
| dependsOn writeVersionProperties | |||
| from tempPropertiesFile | |||
| from wrapperProperties | |||
There was a problem hiding this comment.
I think we should rename this file so it is more obvious what we are using it for. Or maybe not write the entire file, just the version.
There was a problem hiding this comment.
I initially added the file as is thinking about parsing it from the URL, but then realized that the wrapper know about the version it's going to use. In retrospect I'm not sure why I choose to add it to the properties files, I think the initial idea rooted, it makes much more sense to add a minimumGradleVersion resource that just has this version number.
|
I made changes so that we track minimum Gradle exactly like version like minimum Java version, it just so happens that right now, we auto-update this with the wrapper. My hope is that we'll be able to add some tests soon so that when we move to Gradle 4.10 we can add a test for 4.9 for confidence that we can still support it, then we might stop auto updating it with the wrapper. On the other hand, we might consider different minimum versions for build-tools and the elasticsearch build as a whole, right now both are checked against this. Checking the elasticsearch build is meaningful in case someone runs the build without the wrapper. |
I think we have so much on our plate right now that that isn't worth it. |
build.gradle
Outdated
| doLast { | ||
| final DistributionLocator locator = new DistributionLocator() | ||
| final GradleVersion version = GradleVersion.version(wrapper.gradleVersion) | ||
| final URI distributionUri = locator.getDistributionFor(version, wrapper.distributionType.name().toLowerCase(Locale.ENGLISH)) | ||
| final URI sha256Uri = new URI(distributionUri.toString() + ".sha256") | ||
| final String sha256Sum = new String(sha256Uri.toURL().bytes) | ||
| wrapper.getPropertiesFile() << "distributionSha256Sum=${sha256Sum}\n" | ||
| println "Added checksum to wrapper properties" | ||
| println "Added checksum and version to wrapper properties" |
There was a problem hiding this comment.
I think this println isn't accurate any more, right?
| @@ -4,3 +4,4 @@ distributionUrl=https\://services.gradle.org/distributions/gradle-4.9-all.zip | |||
| zipStoreBase=GRADLE_USER_HOME | |||
| zipStorePath=wrapper/dists | |||
| distributionSha256Sum=39e2d5803bbd5eaf6c8efe07067b0e5a00235e8c71318642b2ed262920b27721 | |||
| gradleVersion=4.9 | |||
There was a problem hiding this comment.
This won't be in the file if you regenerate it, right?
|
LGTM! |
* Determine the minimum gradle version based on the wrapper This is restrictive and forces users of the plugin to move together with us, but without integration tests it's close to impossible to make sure that the claimed compatability is really there. If we do want to offer more flexibility, we should add those tests first. * Track gradle version in individual file * PR review
* elastic/6.x: Tests: Fix XPack upgrade tests (#32352) Remove invalid entry from 6.3.2 release notes Number of utilities for writing gradle integration tests (#32282) Determine the minimum gradle version based on the wrapper (#32226) Enable FIPS JVM in CI (#32330) Build: Fix jarHell error I caused by last backport Build: Shadow x-pack:protocol into x-pack:plugin:core (#32240)
* ccr: Number of utilities for writing gradle integration tests (elastic#32282) Determine the minimum gradle version based on the wrapper (elastic#32226) Enable FIPS JVM in CI (elastic#32330) Build: Fix jarHell error I caused by last backport Use shadow plugin in ccr/qa Fix JarHell on X-Pack protocol
This is restrictive and forces users of the plugin to move together with
us, but without integration tests it's close to impossible to make sure
that the claimed compatability is really there.
If we do want to offer more flexibility, we should add those tests
first.
@nik9000 this is the promised follow-up to #32200