Repository navigation
[SPARK-20123][build]$SPARK_HOME variable might have spaces in it(e.g. $SPARK… - #17452
zuotingbing wants to merge 2 commits into
Conversation
…_HOME=/home/spark build/spark), then build spark failed.
zuotingbing
changed the title
|
|
||
| FWDIR="$(cd `dirname "${BASH_SOURCE[0]}"`; pwd)" | ||
| pushd $FWDIR > /dev/null | ||
| FWDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")"; pwd)" |
There was a problem hiding this comment.
This removed the backtick so it no longer works
There was a problem hiding this comment.
you can use like this:
FWDIR="$(cd "`dirname "${BASH_SOURCE[0]}"`"; pwd)"
But i used the "$( )", it also works. You can test this.
Thank you.
There was a problem hiding this comment.
Ah OK, it does seem to work. There are other instances below though that I suppose should be consistent
| # Build a zip file containing the source package with vignettes | ||
| SPARK_HOME="${SPARK_HOME}" "$R_SCRIPT_PATH/"R CMD build $FWDIR/pkg | ||
| SPARK_HOME="${SPARK_HOME}" "$R_SCRIPT_PATH/"R CMD build "$FWDIR"/pkg | ||
|
|
There was a problem hiding this comment.
You could quote whole arguments that include a variable for completeness
There was a problem hiding this comment.
Sure, but i do not think it is a good choice to quote whole arguments that include a variable.
e.g. cp "/home/spark build/spark/conf/*.template" "/home/test" will failed.
It seems to me that quote the variable is better than whole arguments, what is your suggestion?
Thanks!
|
Yes it wouldn't work with shell expansion. I don't think that is the case here but it is fine either way. Consistency is the most important thing. |
|
OK，will do. Thanks! |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
OK, looks like a good standardization. One small fix needed I think.
| mkdir "$DISTDIR"/conf | ||
| cp "$SPARK_HOME"/conf/*.template "$DISTDIR"/conf | ||
| mkdir "$DISTDIR/conf" | ||
| cp "$SPARK_HOME"/conf/*.template "$DISTDIR/conf" |
There was a problem hiding this comment.
I think the two args need quoting separately?
There was a problem hiding this comment.
oh NO, it might be wrong if we quote the arg $SPARK_HOME/conf/*.template as a whole.
It works well already and the debug info as follows :
+ mkdir '/home/spark build/spark/dist/conf'
+ cp '/home/spark build/spark/conf/docker.properties.template' '/home/spark build/spark/conf/fairscheduler.xml.template' '/home/spark build/spark/conf/log4j.properties.template' '/home/spark build/spark/conf/metrics.properties.template' '/home/spark build/spark/conf/slaves.template' '/home/spark build/spark/conf/spark-defaults.conf.template' '/home/spark build/spark/conf/spark-env.sh.template' '/home/spark build/spark/dist/conf'
There was a problem hiding this comment.
Sorry, yes that's also correct. But right now this is just one big argument to cp, not 2 or more. Doesn't it need to be more like cp "$SPARK_HOME"/conf/*.template "$DISTDIR/conf"?
There was a problem hiding this comment.
Of course i use cp "$SPARK_HOME"/conf/*.template "$DISTDIR/conf" to get the debug info. You can write a test.sh and $ sh -x test.sh for debug.
There was a problem hiding this comment.
Hm, I don't see that this works.
touch foo
cp "f* bar"
usage ... source_file target_file
What am I missing?
There was a problem hiding this comment.
If you quote f* bar , f* bar will be treated as one argument. The error is missing the destination file.
There was a problem hiding this comment.
OK the line above is correct. I must be crazy, I thought I read it earlier as being one big quoted argument. If it wasn't that way before then I must have misread it and you can ignore this comment.
| # Remove the python distribution from dist/ if we built it | ||
| if [ "$MAKE_PIP" == "true" ]; then | ||
| rm -f $DISTDIR/python/dist/pyspark-*.tar.gz | ||
| rm -f "$DISTDIR"/python/dist/pyspark-*.tar.gz |
There was a problem hiding this comment.
rm -f "$DISTDIR/python/dist/pyspark-*.tar.gz"?
There was a problem hiding this comment.
Doesn't that fail to work because * isn't expanded?
There was a problem hiding this comment.
it is wrong if we quote the arg $DISTDIR/python/dist/pyspark-*.tar.gz as a whole because * isn't expanded.
There was a problem hiding this comment.
ok I see that other thread...
|
Test build #3631 has finished for PR 17452 at commit
|
|
Merged to master |
4 participants
JIRA Issue: https://issues.apache.org/jira/browse/SPARK-20123
What changes were proposed in this pull request?
If $SPARK_HOME or $FWDIR variable contains spaces, then use "./dev/make-distribution.sh --name custom-spark --tgz -Psparkr -Phadoop-2.7 -Phive -Phive-thriftserver -Pmesos -Pyarn" build spark will failed.
How was this patch tested?
manual tests