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 8.8 #11778

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Bump Gradle wrapper to 8.8 #11778

merged 3 commits into from
Jul 4, 2024

Conversation

wzieba
Copy link
Contributor

@wzieba wzieba commented Jun 24, 2024

Description

This PR bumps Gradle wrapper to 8.8.

Testing information

CI checks should be just fine.

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@wzieba wzieba added the category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. label Jun 24, 2024
@wzieba wzieba added this to the 19.3 milestone Jun 24, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 24, 2024

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commite66afb9
Direct Downloadwoocommerce-wear-prototype-build-pr11778-e66afb9.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 24, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commite66afb9
Direct Downloadwoocommerce-prototype-build-pr11778-e66afb9.apk

@wzieba wzieba marked this pull request as ready for review June 24, 2024 12:10
@wzieba wzieba requested a review from ParaskP7 June 24, 2024 12:10
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @wzieba !

I have reviewed this PR, everything works as expected, good job! 🌟

I also tested this with included builds (local-builds.gradle) and everything seems to be working as expected. ✅

Btw, why are we doing this update now, I am just trying to understand whether there was a need for that, for security purposes maybe, or whether this was done purely to keep our tooling up-to-date, and maybe, to also have Gradle prepared for a future AGP update? 🤔


I have left a question (❓) for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.


EXTRAS

  1. I issued another such update command and it gave me an updated gradlew and gradlew-wrapper.jar file. Can you try it again on your side and see if you also are getting these file to be re-updated, just like it happens on my side? 🙏

FYI: I used the following command:
./gradlew wrapper --gradle-version=8.8 --distribution-type=bin --gradle-distribution-sha256-sum=a4b4158601f8636cdeeab09bd76afb640030bb5b144aafe261a5e8af027dc612

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 19.4. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wzieba
Copy link
Contributor Author

wzieba commented Jul 3, 2024

Btw, why are we doing this update now, I am just trying to understand whether there was a need for that, for security purposes maybe, or whether this was done purely to keep our tooling up-to-date, and maybe, to also have Gradle prepared for a future AGP update? 🤔

It's a preparation for AGP update, but also a casual dependency update. I think that we were usually combining AGP and Gradle updates, but there's no need to not update Gradle separately and more often.

I issued another such update command and it gave me an updated gradlew and gradlew-wrapper.jar file. Can you try it again on your side and see if you also are getting these file to be re-updated, just like it happens on my side? 🙏

I've doubled check and there's no difference for my gradlew and gradlew-wrapper.jar files. Also, I think "Validate Gradle Wrapper" should throw an error if these files were not precisely as they intend to be 🤔 . Could you please double check if there's still a diff when you run wrapper task?

@ParaskP7
Copy link
Contributor

ParaskP7 commented Jul 4, 2024

It's a preparation for AGP update, but also a casual dependency update. I think that we were usually combining AGP and Gradle updates, but there's no need to not update Gradle separately and more often.

I agree, maybe in the past, with some major updates, we had to do both, but unless we do have to do both, let's always do Gradle in isolation, and only then follow-up on AGP updates. 💯

I've doubled check and there's no difference for my gradlew and gradlew-wrapper.jar files. Also, I think "Validate Gradle Wrapper" should throw an error if these files were not precisely as they intend to be 🤔 . Could you please double check if there's still a diff when you run wrapper task?

Yea, I did double and triple check that, me running ./gradlew wrapper --gradle-version=8.8 ... will always produce a new diff. On the gradlew side it is just a comment change (can be ignored I guess), but on the gradlew-wrapper.jar side I am also seeing some code changes (can't be too much ignored). 🤔

I wonder why I get a diff and you don't, this is so strange and unpredictable, and imagine, we both use a Mac... 🤷


FYI: I don't want to block the merge so feel free to merge this anyway. 😊

PS: Same for WPAndroid.

@wzieba
Copy link
Contributor Author

wzieba commented Jul 4, 2024

Yea, I did double and triple check that, me running ./gradlew wrapper --gradle-version=8.8 ... will always produce a new diff.

This is concerning 😕 - could you please prepare a PR (or only branch) with the diff? I'd like to examine the difference.

@ParaskP7
Copy link
Contributor

ParaskP7 commented Jul 4, 2024

Sure thing @wzieba I just did that and pushed this commit here on this bump_gradle_wrapper_petros branch. 👍

Let me know if that helps. 🙏

@wzieba
Copy link
Contributor Author

wzieba commented Jul 4, 2024

So both versions are safe:

From this PR:
image

From your commit
image

But comparing this to https://gradle.org/release-checksums/, it looks like my update updated jar to version 8.6 🤨 The difference is that I used local gradle installed and I have it in version 8.6. But it shouldn't matter, as the task parameters point to 8.8.

I'll go ahead and cherry-pick your commit and merge this PR.

Command: ./gradlew wrapper --gradle-version=8.8 --distribution-type=all
--gradle-distribution-sha256-sum=
f8b4f4772d302c8ff580bc40d0f56e715de69b163546944f787c87abf209c961
@wzieba wzieba enabled auto-merge July 4, 2024 13:31
@ParaskP7
Copy link
Contributor

ParaskP7 commented Jul 4, 2024

...it looks like my update updated jar to version 8.6 🤨

Oh, interesting, and I was wondering what was happening there, thanks for sharing! 😅

I'll go ahead and cherry-pick your commit and merge this PR.

🚀

@wzieba wzieba merged commit 2c0faf4 into trunk Jul 4, 2024
15 checks passed
@wzieba wzieba deleted the bump_gradle_wrapper branch July 4, 2024 13:49
wzieba referenced this pull request in wordpress-mobile/WordPress-Android Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants