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-5747: Fix wordsplitting bugs in make-distribution.sh #4540
Conversation
I'm guessing this is a continuation of #4126. |
@dyross Thank you for fixing this issue. Could you:
Totally fine if you just want to fix the call to Maven, but there are a few other problematic places in this script. For example: |
FWIW, this patch LGTM as-is. |
Rather than add yet more JIRAs, let's just make this part of the SPARK-5747 umbrella. We may have two commits, sure. Putting the JIRA in the title, and perhaps addressing a few more instances of this in the script, would be great. @pwendell quick check -- if we make a fix like this, how far back does it get back-ported? 1.2 or further? |
I've updated the commit wording and fixed a few more instances, as requested. The script seems to execute fine in our build system, but I am not 100% sure about all the edge cases. |
@@ -100,7 +100,7 @@ if [ -z "$JAVA_HOME" ]; then | |||
if [ $(command -v rpm) ]; then | |||
RPM_JAVA_HOME=$(rpm -E %java_home 2>/dev/null) |
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.
I think we need to quote the subshell, since its output may contain spaces.
RPM_JAVA_HOME="$(rpm -E %java_home 2>/dev/null)"
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.
Thanks for the updates @dyross! One minor comment and I think this is good to go. |
Add double quotes to several variables, including $MVN.
This patch LGTM. |
Thanks for the review, @nchammas. |
ok to test |
Test build #27381 has finished for PR 4540 at commit
|
Merging into master 1.3 thanks. There were significant merge conflicts for branch-1.2 so @dyross it would be good if you could open this PR against that branch too. |
The `$MVN` command variable may have spaces, so when referring to it, must wrap in quotes. Author: David Y. Ross <dyross@gmail.com> Closes #4540 from dyross/dyr-fix-make-distribution2 and squashes the following commits: 5a41596 [David Y. Ross] SPARK-5747: Fix wordsplitting bugs in make-distribution.sh (cherry picked from commit 26c816e) Signed-off-by: Andrew Or <andrew@databricks.com>
The
$MVN
command variable may have spaces, so when referring to it, must wrap in quotes.