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

Bump gradle wrapper to version 5.6.4 #3422

Closed
wants to merge 2 commits into from

Conversation

sam-sla
Copy link

@sam-sla sam-sla commented Oct 17, 2019

Current version 4.10.2 is over one year old and newer versions of gradle seem to improve build times, from close to 3min to just over 2min on my machine.
Gradle 5 and above requires Java 8, and that requirement is met since the build instructions mention Java 10 and 11.

@ripcurlx
Copy link
Contributor

@sam-sla Based on the Wrapper JAR Checksum is it correct that you updated the gradle-wrapper.jar version from v4.9 to v4.10.2? Why not directly to v5.6.2? I'm no pro Gradle user so there might be a reason for it. Would it be possible to add also the checksum verification for the downloaded Gradle version to this PR?

@ripcurlx
Copy link
Contributor

I personally use Gradle v5.6.2 for some time already and never had any issues building Bisq so far.
@devinbileck Could you give it a run on Windows as well?
@wiz Could you give it a run on Linux as well?

@sam-sla
Copy link
Author

sam-sla commented Nov 12, 2019

I did this on Linux, forgot to mention :)

As as I remember I just ran the gradle wrapper task to 5.6.2 (and actually there's a 5.6.4 now that we should use). I don't remember going from 4.9 to 4.10.2, as it is 4.10.2 in the repo already.
I am no pro gradle user either :P Will look into the checksum verification, thanks for the feedback!

@ripcurlx
Copy link
Contributor

I haven't touched the gradle stuff so far, but found out when checking the hashes with the official gradle wrapper hashes that we have the wrapper file v4.9 on master and that it was updated to v4.10.2 in this PR. Maybe that is how it should be if it is upgraded step by step, but I'm just not sure if there is a real benefit having the wrapper file v4.10.2 if we download in the configuration 5.6.2.

@sam-sla sam-sla force-pushed the sam/bump-gradle-wrapper branch from f8c1713 to 74f832a Compare November 12, 2019 21:54
@sam-sla
Copy link
Author

sam-sla commented Nov 12, 2019

So I've re-done this to be sure. Fetched latest from the upstream repo and updated my fork; reset this branch and updated the wrapper directly to 5.6.4 (and executed it once which actually triggered more changes to the wrapper jar and the scripts!) then ran the checksum verification like you suggested and everything seems to match. I think it should be good now 👍

@sam-sla sam-sla changed the title Bump gradle wrapper to version 5.6.2 Bump gradle wrapper to version 5.6.4 Nov 12, 2019
@ripcurlx
Copy link
Contributor

@sam-sla Does gradle/wrapper/gradle-wrapper.jar.sha256 has any effect during the gradle download process? Or is this just the output of your manually checksum check of the wrapper jar? What I meant with verifying https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:verification, was the checksum of the downloaded distribution by the gradle-wrapper.jar as the gradle-wrapper.jar itself will be manually verified by us when we merge it into master anyways.

@sam-sla
Copy link
Author

sam-sla commented Nov 13, 2019

@sam-sla Does gradle/wrapper/gradle-wrapper.jar.sha256 has any effect during the gradle download process? Or is this just the output of your manually checksum check of the wrapper jar? What I meant with verifying https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:verification, was the checksum of the downloaded distribution by the gradle-wrapper.jar as the gradle-wrapper.jar itself will be manually verified by us when we merge it into master anyways.

Yes it was just the "output" of the verification, that sha was pulled from their page for the current version of the jar :) I thought you wanted it in so it could be verified at any point with sha256sum --check gradle-wrapper.jar.sha256, shall I remove it then?

@ripcurlx
Copy link
Contributor

I thought you wanted it in so it could be verified at any point with sha256sum --check gradle-wrapper.jar.sha256, shall I remove it then?

Yes please, I thought this is somehow loaded by the wrapper itself for verification.

@ripcurlx
Copy link
Contributor

Can you please add distributionSha256Sum=1f3067073041bc44554d0efe5d402a33bc3d3c93cc39ab684f308586d732a80d to the gradle-wrapper.properties file? This will cause a downloaded distribution to fail if it doesn't match the hash.

 bisq git:(sam/bump-gradle-wrapper) ✗ ./gradlew
Downloading https://services.gradle.org/distributions/gradle-5.6.4-bin.zip
.........................................................................................
Verification of Gradle distribution failed!

Your Gradle distribution may have been tampered with.
Confirm that the 'distributionSha256Sum' property in your gradle-wrapper.properties file is correct and you are downloading the wrapper from a trusted source.

 Distribution Url: https://services.gradle.org/distributions/gradle-5.6.4-bin.zip
Download Location: /Users/christoph/.gradle/wrapper/dists/gradle-5.6.4-bin/bxirm19lnfz6nurbatndyydux/gradle-5.6.4-bin.zip
Expected checksum: '1f3067073041bc44554d0efe5d402a33bc3d3c93cc39ab684f308586d732a80dxxx'
  Actual checksum: '1f3067073041bc44554d0efe5d402a33bc3d3c93cc39ab684f308586d732a80d'

@sam-sla
Copy link
Author

sam-sla commented Nov 14, 2019

Can you please add distributionSha256Sum=1f3067073041bc44554d0efe5d402a33bc3d3c93cc39ab684f308586d732a80d to the gradle-wrapper.properties file? This will cause a downloaded distribution to fail if it doesn't match the hash.

 bisq git:(sam/bump-gradle-wrapper) ✗ ./gradlew
Downloading https://services.gradle.org/distributions/gradle-5.6.4-bin.zip
.........................................................................................
Verification of Gradle distribution failed!

Your Gradle distribution may have been tampered with.
Confirm that the 'distributionSha256Sum' property in your gradle-wrapper.properties file is correct and you are downloading the wrapper from a trusted source.

 Distribution Url: https://services.gradle.org/distributions/gradle-5.6.4-bin.zip
Download Location: /Users/christoph/.gradle/wrapper/dists/gradle-5.6.4-bin/bxirm19lnfz6nurbatndyydux/gradle-5.6.4-bin.zip
Expected checksum: '1f3067073041bc44554d0efe5d402a33bc3d3c93cc39ab684f308586d732a80dxxx'
  Actual checksum: '1f3067073041bc44554d0efe5d402a33bc3d3c93cc39ab684f308586d732a80d'

That looks great, always learning :) Will do it later today 👍

Samuel Almeida added 2 commits November 14, 2019 18:45
Gradle will self-verify the sha of the downloaded wrapper
zip upon its first download.
@sam-sla sam-sla force-pushed the sam/bump-gradle-wrapper branch from 74f832a to 9a9ffa4 Compare November 14, 2019 17:47
@sam-sla
Copy link
Author

sam-sla commented Nov 14, 2019

Done now, and I noticed I had modified the distribution type from "all" to "bin" by mistake, so I fixed that one now too (the sha is different from the one you tested).
I also tested the sha verification when it downloads the wrapper zip 👍

Copy link
Member

@devinbileck devinbileck left a comment

Choose a reason for hiding this comment

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

ACK
Build was successful on Windows.
Checksums are consistent with https://gradle.org/release-checksums/.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

@ripcurlx
Copy link
Contributor

@sam-sla Travis detected an abuse on this branch and I'm not able to trigger a manual build for this. Could you close this PR and make a new one with same content referencing this PR? Thanks!

@sam-sla
Copy link
Author

sam-sla commented Nov 15, 2019

Closing in favor of #3615

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants