-
Notifications
You must be signed in to change notification settings - Fork 14.7k
MINOR: new checksum verification in gradlew #20658
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
Conversation
Th PR description is not correct. We don't want to revert whole changes. Als, please reference to the conversation |
Done |
@joshua2519 I have another idea that we could check the |
@chia7712 |
wrapper.gradle
Outdated
|
||
def bootstrapString = """ | ||
# Loop in case we encounter an error. | ||
REQUIRED_WRAPPER_JAR_CHECKSUM=\$(curl -sSL "$wrapperJarChecksumUrl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please hardcode the sha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that. Considering that Gradle version updates are infrequent, hardcoding the SHA is a much more efficient method than retrieving the checksum every time. I've included a code comment to ensure the checksum is also updated whenever we perform a Gradle version upgrade.
wrapper.gradle
Outdated
continue | ||
fi | ||
else | ||
# Verify checksum of existing wrapper JAR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments explaining the reason for adding this mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
sleep 5 | ||
continue | ||
fi | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch causes the unnecessary loops, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Could it be better to add a condition to break the loop?
if [ "\$LOCAL_WRAPPER_JAR_CHECKSUM" != "\$REQUIRED_WRAPPER_JAR_CHECKSUM" ] ; then | ||
rm -f "$wrapperJarPath" | ||
else | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chia7712
The loop will break once the JAR file exists and the checksum matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if users' local machine can't use either sha256sum
or shasum
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chia7712
Thank you for pointing that out.
If no checksum tool is found, exit with an error.
gradlew
Outdated
LOCAL_WRAPPER_JAR_CHECKSUM=$(shasum -a 256 "$APP_HOME/gradle/wrapper/gradle-wrapper.jar" | awk '{print $1}') | ||
else | ||
# If no checksum tool is found, exit with an error. | ||
die "ERROR: Cannot find sha256sum or shasum to verify wrapper JAR. Please install one of these tools." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just skip the sha check if there is no available tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
wrapper.gradle
Outdated
elif command -v shasum >/dev/null 2>&1; then | ||
LOCAL_WRAPPER_JAR_CHECKSUM=\$(shasum -a 256 "$wrapperJarPath" | awk '{print \$1}') | ||
else | ||
# If no checksum tool is found, exit with an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no checksum tool is found, exit with an error.
this comment is out-of-date, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. I have fixed it.
I have completed the following tests locally:
|
def wrapperBaseUrl = "https://raw.githubusercontent.com/gradle/gradle/v$versions.gradle/gradle/wrapper" | ||
def wrapperJarUrl = wrapperBaseUrl + "/gradle-wrapper.jar" | ||
// IMPORTANT: This checksum **must** be updated whenever the Gradle version changes. | ||
String wrapperChecksum = "76805e32c009c0cf0dd5d206bddc9fb22ea42e84db904b764f3047de095493f3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please merge wrapperBaseUrl
with wrapperJarUrl
and then move wrapperChecksum
up to line#41? Doing so would remind future developers to update both variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
According to the [discussion](apache#19513 (comment)) in apache#19513 , add a new checksum verification process to determine whether a new wrapper JAR needs to be downloaded. This prevents developers from running into incompatibility issues when using an outdated wrapper JAR after a Gradle upgrade. Reviewers: Chia-Ping Tsai <[email protected]>
According to the
discussion
in #19513 , add a new checksum verification process to determine whether
a new wrapper JAR needs to be downloaded.
This prevents developers from running into incompatibility issues when
using an outdated wrapper JAR after a Gradle upgrade.
Reviewers: Chia-Ping Tsai [email protected]