KAFKA-1714: Fix gradle wrapper bootstrapping by granthenke · Pull Request #6031 · apache/kafka

granthenke · 2018-12-13T18:10:24Z

Given we need to follow the Apache rule of not checking
any binaries into the source code, Kafka has always had
a bit of a tricky Gradle bootstrap.
Using ./gradlew as users expect doesn’t work and a
local and compatible version of Gradle was required to
generate the wrapper first.

This patch changes the behavior of the wrapper task to
instead generate a gradlew script that can bootstrap the
jar itself. Additionally it adds a license, removes the bat
script, and handles retries.

The documentation in the readme was also updated.

Going forward patches that upgrade gradle should run
gradle wrapper before checking in the change.

With this change users using ./gradlew can be sure they
are always building with the correct version of Gradle.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

Given we need to follow the Apache rule of not checking any binaries into the source code, Kafka has always had a bit of a tricky Gradle bootstrap. Using ./gradlew as users expect doesn’t work and a local and compatible version of Gradle was required to generate the wrapper first. This patch changes the behavior of the wrapper task to instead generate a gradlew script that can bootstrap the jar itself. Additionally it adds a license, removes the bat script, and handles retries. The documentation in the readme was also updated. Going forward patches that upgrade gradle should run `gradle wrapper` before checking in the change. With this change users using ./gradlew can be sure they are always building with the correct version of Gradle.

granthenke · 2018-12-13T18:11:13Z

Note: This pattern has been used successfully on the Apache Kudu project:
https://github.com/apache/kudu/blob/master/java/gradle/wrapper.gradle

granthenke · 2018-12-14T18:17:44Z

Looking at previously closed Gradle patches I see a few folks that have done reviews:
@ijuma @cmccabe @lindong28

Would any of you be able to review this or suggest someone who could?

viktorsomogyi · 2018-12-18T14:32:48Z

+# limitations under the License."""
+
+// Custom task to inject support for downloading the gradle wrapper jar if it doesn't exist.
+// This allows us to avoid checking in the jar to our repository.


Did you remove the jar too?

The jar shouldn't be removed, just not checked in. The .gitignore entry handles this.

viktorsomogyi · 2018-12-19T14:08:54Z

+        def wrapperJarPath = wrapperBasePath + "/gradle-wrapper.jar"
+
+        // Add a trailing zero to the version if needed.
+        def fullVersion = project.gradleVersion.count(".") == 1 ? "${project.gradleVersion}.0" : versions.gradle


If I understand correctly here you would use the project's gradle version instead of the distributionUrl in the gradle-wrapper.properties, right? That would make sure to always use the correct version of gradle.
I think perhaps we could make a comment there that people should not expect the distributionUrl to work anymore, neither gradle wrapper --gradle-version x.y as they'd expect.

All of those things would still work as expected and as it did before.

What this patch does is edit the gradlew script to support downloading the wrapper jar if it doesn't exist. That jar is technically gradle version agnostic. It hasn't changed for a very long time.

Running the gradle wrapper task will still output a gradle/wrapper/gradle-wrapper.properties file and passing --gradle-version x.y.x, distribution-type=foo or --gradle-distribution-url=bar will all still override the task defaults.

viktorsomogyi

I reviewed and tried it out. LGTM.

ijuma

Thanks for the PR.

ijuma · 2019-01-10T06:35:34Z

+##
+##  Gradle start up script for UN*X
+##
+##############################################################################


Out of curiosity, is this script copied from somewhere?

This is the script that is generated by the gradle wrapper task.

ijuma · 2019-01-10T06:39:02Z

+task removeWindowsScript(type: Delete) {
+    delete "$rootDir/gradlew.bat"
+}
+wrapper.finalizedBy removeWindowsScript


Hmm, people do seem to build on Windows even if we don't support it. Should we keep this?

Users could still build with a local install of gradle, they just can't use the wrapper. Given we don't support it, I don't want to do more work to make it easier.

Does that mean that we need different instructions for Windows? Given the number of complaints we get from Windows users, I think we should not make things worse for them if we can avoid it. If you have concerns about this working for Windows users, we could simply state in the README that they have to do it the old way.

granthenke · 2019-01-10T14:26:06Z

@ijuma I still plan to create a separate patch to update the readme with more docs around using gradle and the wrapper. I just haven't had time.

chbatey · 2019-11-20T07:48:03Z

👍 as it stands bootstrapping from the latest version of gradle doesn't work so I needed to delete parts of the build.gradle file to get the latest gradle to generate a gradle wrapper

ijuma

LGTM

Given we need to follow the Apache rule of not checking any binaries into the source code, Kafka has always had a bit of a tricky Gradle bootstrap. Using ./gradlew as users expect doesn’t work and a local and compatible version of Gradle was required to generate the wrapper first. This patch changes the behavior of the wrapper task to instead generate a gradlew script that can bootstrap the jar itself. Additionally it adds a license, removes the bat script, and handles retries. The documentation in the readme was also updated. Going forward patches that upgrade gradle should run `gradle wrapper` before checking in the change. With this change users using ./gradlew can be sure they are always building with the correct version of Gradle. Reviewers: Viktor Somogyi <viktorsomogyi@gmail.com>, Ismael Juma <ismael@juma.me.uk

…invocation of `gradle` See apache/kafka#7677 for more details with regards to the former and apache/kafka#6031 with regards to the latter.

…invocation of `gradle` (#329) See apache/kafka#7677 for more details with regards to the former and apache/kafka#6031 with regards to the latter.

Given we need to follow the Apache rule of not checking any binaries into the source code, Kafka has always had a bit of a tricky Gradle bootstrap. Using ./gradlew as users expect doesn’t work and a local and compatible version of Gradle was required to generate the wrapper first. This patch changes the behavior of the wrapper task to instead generate a gradlew script that can bootstrap the jar itself. Additionally it adds a license, removes the bat script, and handles retries. The documentation in the readme was also updated. Going forward patches that upgrade gradle should run `gradle wrapper` before checking in the change. With this change users using ./gradlew can be sure they are always building with the correct version of Gradle. Reviewers: Viktor Somogyi <viktorsomogyi@gmail.com>, Ismael Juma <ismael@juma.me.uk

granthenke added 2 commits December 13, 2018 13:43

Include gradlew

829841a

Include the wrapper properties

0e9e7bc

Merge branch 'trunk' into gradle-wrapper

fe903f3

viktorsomogyi reviewed Dec 18, 2018

View reviewed changes

Fix removeWindowsScript

0207db1

viktorsomogyi reviewed Dec 19, 2018

View reviewed changes

viktorsomogyi approved these changes Dec 21, 2018

View reviewed changes

ijuma reviewed Jan 10, 2019

View reviewed changes

Merge branch 'trunk' into gradle-wrapper

b7e785a

ijuma approved these changes Nov 21, 2019

View reviewed changes

ijuma merged commit 09f700a into apache:trunk Nov 21, 2019

ijuma mentioned this pull request Apr 21, 2020

Replace ./gradlew installAll with ./gradlewAll install and remove invocation of gradle` confluentinc/kafka-streams-examples#329

Merged

Conversation

granthenke commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

granthenke commented Dec 13, 2018

Uh oh!

granthenke commented Dec 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viktorsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

granthenke commented Jan 10, 2019

Uh oh!

chbatey commented Nov 20, 2019

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

granthenke commented Dec 13, 2018 •

edited

Loading