-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-22072][SPARK-22071][BUILD]Improve release build scripts #19312
Conversation
cc @JoshRosen |
Test build #82048 has finished for PR 19312 at commit
|
retest this please |
Test build #82056 has finished for PR 19312 at commit
|
dev/create-release/release-build.sh
Outdated
@@ -95,6 +95,28 @@ if [ -z "$SPARK_VERSION" ]; then | |||
| grep -v INFO | grep -v WARNING | grep -v Download) | |||
fi | |||
|
|||
# Verify we have the right java version set | |||
java_version=$("${JAVA_HOME}"/bin/javac -version 2>&1 | cut -d " " -f 2) |
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.
@holdenk, should we maybe catch the case when JAVA_HOME
is missing too?
If so, I think we could do something like ...
if [ -z "$JAVA_HOME" ]; then
echo "Please set JAVA_HOME."
exit 1
fi
...
Or maybe...
if [[ -x "$JAVA_HOME/bin/javac" ]]; then
javac_cmd="$JAVA_HOME/bin/javac"
else
javac_cmd=javac
fi
java_version=$("$javac_cmd" -version 2>&1 | cut -d " " -f 2)
...
I tested both in my local.
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. The logic we use in make-distribution to resolve JAVA_HOME when not set is a bit more complicated, but I think for the release script its reasonable (and probably a good practice) to explicitly require a JAVA_HOME to be set so I'll go with the first of the 2.
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.
LGTM, just one comment
## What changes were proposed in this pull request? Check JDK version (with javac) and use SPARK_VERSION for publish-release ## How was this patch tested? Manually tried local build with wrong JDK / JAVA_HOME & built a local release (LFTP disabled) Author: Holden Karau <holden@us.ibm.com> Closes #19312 from holdenk/improve-release-scripts-r2. (cherry picked from commit 8f130ad) Signed-off-by: Holden Karau <holden@us.ibm.com>
## What changes were proposed in this pull request? Check JDK version (with javac) and use SPARK_VERSION for publish-release ## How was this patch tested? Manually tried local build with wrong JDK / JAVA_HOME & built a local release (LFTP disabled) Author: Holden Karau <holden@us.ibm.com> Closes #19312 from holdenk/improve-release-scripts-r2. (cherry picked from commit 8f130ad) Signed-off-by: Holden Karau <holden@us.ibm.com>
Test build #82071 has finished for PR 19312 at commit
|
Merged to master, branch-2.2, and branch-2.1. |
## What changes were proposed in this pull request? Check JDK version (with javac) and use SPARK_VERSION for publish-release ## How was this patch tested? Manually tried local build with wrong JDK / JAVA_HOME & built a local release (LFTP disabled) Author: Holden Karau <holden@us.ibm.com> Closes apache#19312 from holdenk/improve-release-scripts-r2. (cherry picked from commit 8f130ad) Signed-off-by: Holden Karau <holden@us.ibm.com>
What changes were proposed in this pull request?
Check JDK version (with javac) and use SPARK_VERSION for publish-release
How was this patch tested?
Manually tried local build with wrong JDK / JAVA_HOME & built a local release (LFTP disabled)