You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/03/08 08:04:00 UTC
[GitHub] [spark] stczwd opened a new pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
stczwd opened a new pull request #35763:
URL: https://github.com/apache/spark/pull/35763
### Why are the changes needed?
Some shell codes are written incorrectly and run abnormally in special cases. Besides, they cannot also pass the check of the shellcheck plugin, especially in IDEA or shellcheck actions. This PR try to fix the shell code style problems, that are reported by [shellcheck](https://github.com/koalaman/shellcheck). There will be another PR to add `shellcheck` to GA.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Origin UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830513931
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! "${tags[*]}" =~ "r" ]]; then
Review comment:
Yeah I'm even wondering about the original logic - why match a single-character regular expression? It's probably OK just the one thing I'm not too sure about. If the behavior _should_ be unchanged - concatenates the string - then probably OK. I wonder if the person meant to write "if one of the arguments is `"r"`" which is not what this does
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r821684004
##########
File path: sbin/spark-daemon.sh
##########
@@ -149,7 +149,7 @@ execute_command() {
sleep 2
# Check if the process has died; in that case we'll tail the log so the user can see
if [[ ! $(ps -p "$newpid" -o comm=) =~ "java" ]]; then
- echo "failed to launch: $@"
+ echo "failed to launch: $*"
Review comment:
SC2145 (error): Argument mixes string and array. Use * or separate argument.
##########
File path: bin/docker-image-tool.sh
##########
@@ -284,7 +284,7 @@ Examples:
EOF
}
-if [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then
+if [[ "$*" = *--help ]] || [[ "$*" = *-h ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: R/install-source-package.sh
##########
@@ -40,7 +40,7 @@ fi
if [ ! -f "$FWDIR/SparkR_$VERSION.tar.gz" ]; then
echo -e "R source package file '$FWDIR/SparkR_$VERSION.tar.gz' is not found."
echo -e "Please build R source package with check-cran.sh"
- exit -1;
+ exit 255;
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: R/run-tests.sh
##########
@@ -66,7 +66,7 @@ else
echo -en "\033[31m" # Red
echo "Had CRAN check errors; see logs."
echo -en "\033[0m" # No color
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/check-license
##########
@@ -35,7 +35,7 @@ acquire_rat_jar () {
wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
else
printf "You do not have curl or wget installed, please install rat manually.\n"
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/run-tests-jenkins
##########
@@ -31,7 +31,7 @@ export LANG="en_US.UTF-8"
PYTHON_VERSION_CHECK=$(python3 -c 'import sys; print(sys.version_info < (3, 6, 0))')
if [[ "$PYTHON_VERSION_CHECK" == "True" ]]; then
echo "Python versions prior to 3.6 are not supported."
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/create-release/release-tag.sh
##########
@@ -42,7 +42,7 @@ EOF
set -e
set -o pipefail
-if [[ $@ == *"help"* ]]; then
+if [[ "$*" == *"help"* ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: sbin/start-master.sh
##########
@@ -27,7 +27,7 @@ fi
# Any changes need to be reflected there.
CLASS="org.apache.spark.deploy.master.Master"
-if [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then
+if [[ "$*" = *--help ]] || [[ "$*" = *-h ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! "${tags[*]}" =~ "r" ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: sbin/start-thriftserver.sh
##########
@@ -50,7 +50,7 @@ function usage {
"${SPARK_HOME}"/bin/spark-class $CLASS --help 2>&1 | grep -v "$pattern" 1>&2
}
-if [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then
+if [[ "$*" = *--help ]] || [[ "$*" = *-h ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
"${BUILD_COMMAND[@]}"
# Make directories
rm -rf "$DISTDIR"
mkdir -p "$DISTDIR/jars"
echo "Spark $VERSION$GITREVSTRING built for Hadoop $SPARK_HADOOP_VERSION" > "$DISTDIR/RELEASE"
-echo "Build flags: $@" >> "$DISTDIR/RELEASE"
+echo "Build flags: $*" >> "$DISTDIR/RELEASE"
Review comment:
SC2145 (error): Argument mixes string and array. Use * or separate argument.
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
fi
-VERSION=$("$MVN" help:evaluate -Dexpression=project.version $@ \
+VERSION=$("$MVN" help:evaluate -Dexpression=project.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version $@ \
+SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version "$@" \
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
fi
-VERSION=$("$MVN" help:evaluate -Dexpression=project.version $@ \
+VERSION=$("$MVN" help:evaluate -Dexpression=project.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version $@ \
+SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SPARK_HADOOP_VERSION=$("$MVN" help:evaluate -Dexpression=hadoop.version $@ \
+SPARK_HADOOP_VERSION=$("$MVN" help:evaluate -Dexpression=hadoop.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SPARK_HIVE=$("$MVN" help:evaluate -Dexpression=project.activeProfiles -pl sql/hive $@ \
+SPARK_HIVE=$("$MVN" help:evaluate -Dexpression=project.activeProfiles -pl sql/hive "$@" \
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
##########
File path: build/sbt-launch-lib.bash
##########
@@ -62,13 +62,13 @@ acquire_sbt_jar () {
mv "${JAR_DL}" "${JAR}"
else
printf "You do not have curl or wget installed, please install sbt manually from https://www.scala-sbt.org/\n"
- exit -1
+ exit 255
fi
fi
if [ ! -f "${JAR}" ]; then
# We failed to download
printf "Our attempt to download sbt locally to ${JAR} failed. Please install sbt manually from https://www.scala-sbt.org/\n"
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
fi
-VERSION=$("$MVN" help:evaluate -Dexpression=project.version $@ \
+VERSION=$("$MVN" help:evaluate -Dexpression=project.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version $@ \
+SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SPARK_HADOOP_VERSION=$("$MVN" help:evaluate -Dexpression=hadoop.version $@ \
+SPARK_HADOOP_VERSION=$("$MVN" help:evaluate -Dexpression=hadoop.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SPARK_HIVE=$("$MVN" help:evaluate -Dexpression=project.activeProfiles -pl sql/hive $@ \
+SPARK_HIVE=$("$MVN" help:evaluate -Dexpression=project.activeProfiles -pl sql/hive "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| fgrep --count "<id>hive</id>";\
- # Reset exit status to 0, otherwise the script stops here if the last grep finds nothing\
+ # Reset exit status to 0, otherwise the script stops here if the last grep finds nothing
Review comment:
SC1143 (error): This backslash is part of a comment and does not continue the line.
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
fi
-VERSION=$("$MVN" help:evaluate -Dexpression=project.version $@ \
+VERSION=$("$MVN" help:evaluate -Dexpression=project.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version $@ \
+SCALA_VERSION=$("$MVN" help:evaluate -Dexpression=scala.binary.version "$@" \
| grep -v "INFO"\
| grep -v "WARNING"\
| tail -n 1)
-SPARK_HADOOP_VERSION=$("$MVN" help:evaluate -Dexpression=hadoop.version $@ \
+SPARK_HADOOP_VERSION=$("$MVN" help:evaluate -Dexpression=hadoop.version "$@" \
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
##########
File path: sql/core/src/test/scripts/gen-thrift.sh
##########
@@ -22,6 +24,6 @@ cd -
rm -rf $BASEDIR/gen-java
mkdir -p $BASEDIR/gen-java
-for input in `ls $BASEDIR/thrift/*.thrift`; do
+for input in $BASEDIR/thrift/*.thrift; do
Review comment:
SC2045 (error): Iterating over ls output is fragile. Use globs.
##########
File path: R/run-tests.sh
##########
@@ -56,7 +56,7 @@ if [[ $FAILED != 0 || $NUM_TEST_WARNING != 0 ]]; then
echo -en "\033[31m" # Red
echo "Had test warnings or failures; see logs."
echo -en "\033[0m" # No color
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/make-distribution.sh
##########
@@ -108,7 +108,7 @@ fi
if [ -z "$JAVA_HOME" ]; then
echo "Error: JAVA_HOME is not set, cannot proceed."
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: build/sbt-launch-lib.bash
##########
@@ -62,13 +62,13 @@ acquire_sbt_jar () {
mv "${JAR_DL}" "${JAR}"
else
printf "You do not have curl or wget installed, please install sbt manually from https://www.scala-sbt.org/\n"
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/check-license
##########
@@ -44,7 +44,7 @@ acquire_rat_jar () {
# We failed to download
rm "$JAR"
printf "Our attempt to download rat locally to ${JAR} failed. Please install rat manually.\n"
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/create-release/release-build.sh
##########
@@ -53,7 +53,7 @@ if [ $# -eq 0 ]; then
exit_with_usage
fi
-if [[ $@ == *"help"* ]]; then
+if [[ "$*" == *"help"* ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: python/run-tests
##########
@@ -24,7 +24,7 @@ cd "$FWDIR"
PYTHON_VERSION_CHECK=$(python3 -c 'import sys; print(sys.version_info < (3, 6, 0))')
if [[ "$PYTHON_VERSION_CHECK" == "True" ]]; then
echo "Python versions prior to 3.6 are not supported."
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/create-release/release-build.sh
##########
@@ -181,7 +181,7 @@ BASE_PROFILES="-Pmesos -Pyarn -Pkubernetes"
PUBLISH_SCALA_2_13=1
SCALA_2_13_PROFILES="-Pscala-2.13"
-if [[ $SPARK_VERSION < "3.2" ]]; then
+if [ "$(echo $SPARK_VERSION | awk -F "." '{if ($1 <= 3 && $2 < 2) print $1}')" ]; then
Review comment:
SC2072 (error): Decimals are not supported. Either use integers only, or use bc or awk to compare.
##########
File path: dev/create-release/release-util.sh
##########
@@ -53,7 +53,7 @@ function run_silent {
echo "========================"
echo "= $BANNER"
- echo "Command: $@"
+ echo "Command: $*"
Review comment:
SC2145 (error): Argument mixes string and array. Use * or separate argument.
##########
File path: dev/run-tests
##########
@@ -23,7 +23,7 @@ cd "$FWDIR"
PYTHON_VERSION_CHECK=$(python3 -c 'import sys; print(sys.version_info < (3, 6, 0))')
if [[ "$PYTHON_VERSION_CHECK" == "True" ]]; then
echo "Python versions prior to 3.6 are not supported."
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: dev/run-pip-tests
##########
@@ -105,7 +105,7 @@ for python in "${PYTHON_EXECS[@]}"; do
sdists=(*.tar.gz)
if [ ${#sdists[@]} -ne 1 ]; then
echo "Unexpected number of targets found in dist directory - please cleanup existing sdists first."
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: sbin/spark-daemon.sh
##########
@@ -82,7 +82,7 @@ spark_rotate_log ()
num=${SPARK_LOG_MAX_FILES}
else
echo "Error: SPARK_LOG_MAX_FILES must be a positive number, but got ${SPARK_LOG_MAX_FILES}"
- exit -1
+ exit 255
Review comment:
SC2242 (error): Can only exit with status 0-255. Other data should be written to stdout/stderr.
##########
File path: sbin/spark-config.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
+
Review comment:
SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
##########
File path: sbin/start-history-server.sh
##########
@@ -32,7 +32,7 @@ fi
# Any changes need to be reflected there.
CLASS="org.apache.spark.deploy.history.HistoryServer"
-if [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then
+if [[ "$*" = *--help ]] || [[ "$*" = *-h ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: dev/test-dependencies.sh
##########
@@ -119,7 +119,7 @@ for HADOOP_HIVE_PROFILE in "${HADOOP_HIVE_PROFILES[@]}"; do
}' | sort | grep -v spark > dev/pr-deps/spark-deps-$HADOOP_HIVE_PROFILE
done
-if [[ $@ == **replace-manifest** ]]; then
+if [[ "$*" == **replace-manifest** ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: sbin/start-worker.sh
##########
@@ -39,7 +39,7 @@ fi
# Any changes need to be reflected there.
CLASS="org.apache.spark.deploy.worker.Worker"
-if [[ $# -lt 1 ]] || [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then
+if [[ $# -lt 1 ]] || [[ "$*" = *--help ]] || [[ "$*" = *-h ]]; then
Review comment:
SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
##########
File path: dev/create-release/release-build.sh
##########
@@ -341,8 +341,8 @@ if [[ "$1" == "package" ]]; then
fi
if [[ $PUBLISH_SCALA_2_12 = 1 ]]; then
- echo "Packages to build: ${!BINARY_PKGS_ARGS[@]}"
- for key in ${!BINARY_PKGS_ARGS[@]}; do
+ echo "Packages to build: ${!BINARY_PKGS_ARGS[*]}"
Review comment:
SC2145 (error): Argument mixes string and array. Use * or separate argument.
##########
File path: build/sbt-launch-lib.bash
##########
@@ -186,7 +186,7 @@ run() {
${SBT_OPTS:-$default_sbt_opts} \
$(get_mem_opts $sbt_mem) \
${java_opts} \
- ${java_args[@]} \
+ "${java_args[@]}" \
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
Review comment:
SC2145 (error): Argument mixes string and array. Use * or separate argument.
##########
File path: dev/create-release/release-build.sh
##########
@@ -341,8 +341,8 @@ if [[ "$1" == "package" ]]; then
fi
if [[ $PUBLISH_SCALA_2_12 = 1 ]]; then
- echo "Packages to build: ${!BINARY_PKGS_ARGS[@]}"
- for key in ${!BINARY_PKGS_ARGS[@]}; do
+ echo "Packages to build: ${!BINARY_PKGS_ARGS[*]}"
+ for key in "${!BINARY_PKGS_ARGS[@]}"; do
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
fi
-VERSION=$("$MVN" help:evaluate -Dexpression=project.version $@ \
+VERSION=$("$MVN" help:evaluate -Dexpression=project.version "$@" \
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
##########
File path: sql/core/src/test/scripts/gen-avro.sh
##########
@@ -22,7 +24,7 @@ cd -
rm -rf $BASEDIR/gen-java
mkdir -p $BASEDIR/gen-java
-for input in `ls $BASEDIR/avro/*.avdl`; do
+for input in $BASEDIR/avro/*.avdl; do
Review comment:
SC2045 (error): Iterating over ls output is fragile. Use globs.
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
"${BUILD_COMMAND[@]}"
# Make directories
rm -rf "$DISTDIR"
mkdir -p "$DISTDIR/jars"
echo "Spark $VERSION$GITREVSTRING built for Hadoop $SPARK_HADOOP_VERSION" > "$DISTDIR/RELEASE"
-echo "Build flags: $@" >> "$DISTDIR/RELEASE"
+echo "Build flags: $*" >> "$DISTDIR/RELEASE"
# Copy jars
cp "$SPARK_HOME"/assembly/target/scala*/jars/* "$DISTDIR/jars/"
# Only create the yarn directory if the yarn artifacts were built.
-if [ -f "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar ]; then
- mkdir "$DISTDIR/yarn"
- cp "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar "$DISTDIR/yarn"
-fi
+for file in "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar
Review comment:
SC2144 (error): -f doesn't work with globs. Use a for loop.
##########
File path: sql/core/src/test/scripts/gen-avro.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
+
Review comment:
SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
##########
File path: sql/core/src/test/scripts/gen-thrift.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
+
Review comment:
SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
##########
File path: resource-managers/kubernetes/integration-tests/dev/dev-run-integration-tests.sh
##########
@@ -200,5 +200,5 @@ properties+=(
-P$HADOOP_PROFILE \
-Pkubernetes \
-Pkubernetes-integration-tests \
- ${properties[@]}
+ "${properties[@]}"
Review comment:
SC2068 (error): Double quote array expansions to avoid re-splitting elements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r821669552
##########
File path: dev/check-license
##########
@@ -44,7 +44,7 @@ acquire_rat_jar () {
# We failed to download
rm "$JAR"
printf "Our attempt to download rat locally to ${JAR} failed. Please install rat manually.\n"
- exit -1
+ exit 255
Review comment:
Sure, I will add a style link to the modified place and explain the reason for the modification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830543980
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! "${tags[*]}" =~ "r" ]]; then
Review comment:
cc @dongjoon-hyun. Could you look at this?
It seems that we run k8s integratation tests with `test.exclude.tags=minikube,r,local`, and the magic actually means to check `if one of the arguments is "r"`? If this, then `if [[ ! " ${tags[*]} " =~ " r " ]]; then` or a loop check can work well with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r822255664
##########
File path: dev/check-license
##########
@@ -44,7 +44,7 @@ acquire_rat_jar () {
# We failed to download
rm "$JAR"
printf "Our attempt to download rat locally to ${JAR} failed. Please install rat manually.\n"
- exit -1
+ exit 255
Review comment:
BTW，`shellcheck` support exclude check rules, thus if we support `exit -1`, we can excude SC2242 in GA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830514121
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
"${BUILD_COMMAND[@]}"
# Make directories
rm -rf "$DISTDIR"
mkdir -p "$DISTDIR/jars"
echo "Spark $VERSION$GITREVSTRING built for Hadoop $SPARK_HADOOP_VERSION" > "$DISTDIR/RELEASE"
-echo "Build flags: $@" >> "$DISTDIR/RELEASE"
+echo "Build flags: $*" >> "$DISTDIR/RELEASE"
# Copy jars
cp "$SPARK_HOME"/assembly/target/scala*/jars/* "$DISTDIR/jars/"
# Only create the yarn directory if the yarn artifacts were built.
-if [ -f "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar ]; then
- mkdir "$DISTDIR/yarn"
- cp "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar "$DISTDIR/yarn"
-fi
+for file in "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar
Review comment:
I think it will fail after this change too, if there are multiple matching files; looping isn't the intent here. We could maybe check if there is exactly one match? that's the 'right' fix. If there is 1 file, I suppose this logic is the same so it's OK, but it's also a bit wrong in the same way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r821651852
##########
File path: dev/check-license
##########
@@ -44,7 +44,7 @@ acquire_rat_jar () {
# We failed to download
rm "$JAR"
printf "Our attempt to download rat locally to ${JAR} failed. Please install rat manually.\n"
- exit -1
+ exit 255
Review comment:
Can we get an explanation for a few of the changes that occur frequently? like is this equivalent to returning -1?
##########
File path: dev/create-release/release-build.sh
##########
@@ -181,7 +181,7 @@ BASE_PROFILES="-Pmesos -Pyarn -Pkubernetes"
PUBLISH_SCALA_2_13=1
SCALA_2_13_PROFILES="-Pscala-2.13"
-if [[ $SPARK_VERSION < "3.2" ]]; then
+if [ "$(echo $SPARK_VERSION | awk -F "." '{if ($1 <= 3 && $2 < 2) print $1}')" ]; then
Review comment:
Or this too, was it nonstandard bash?
##########
File path: dev/create-release/release-build.sh
##########
@@ -53,7 +53,7 @@ if [ $# -eq 0 ]; then
exit_with_usage
fi
-if [[ $@ == *"help"* ]]; then
+if [[ "$*" == *"help"* ]]; then
Review comment:
Likewise what is the difference here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r828632005
##########
File path: dev/create-release/release-build.sh
##########
@@ -181,7 +181,7 @@ BASE_PROFILES="-Pmesos -Pyarn -Pkubernetes"
PUBLISH_SCALA_2_13=1
SCALA_2_13_PROFILES="-Pscala-2.13"
-if [[ $SPARK_VERSION < "3.2" ]]; then
+if [ "$(echo $SPARK_VERSION | awk -F "." '{if ($1 <= 3 && $2 < 2) print $1}')" ]; then
Review comment:
There indeed has some proble here. `10.0` will pass `$SPARK_VERSION < "3.2"`. The condition can be changed to `echo $SPARK_VERSION | awk -F "." '{if (($1 < 3) || ($1 == 3 && $2 < 2)) print $1}'`.
Hm, maybe skip the check and leave waning comment here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r828428242
##########
File path: dev/create-release/release-build.sh
##########
@@ -341,8 +341,8 @@ if [[ "$1" == "package" ]]; then
fi
if [[ $PUBLISH_SCALA_2_12 = 1 ]]; then
- echo "Packages to build: ${!BINARY_PKGS_ARGS[@]}"
- for key in ${!BINARY_PKGS_ARGS[@]}; do
+ echo "Packages to build: ${!BINARY_PKGS_ARGS[*]}"
+ for key in "${!BINARY_PKGS_ARGS[@]}"; do
Review comment:
The case it's worried about isn't applicable here. I'm trying to reason about whether this breaks something by re-quoting the space-separated args. Maybe not. It's hard to test some of these but I am kind of nervous about these changes to the release script especially that we won't find problems with until release time. Maybe we can be more conservative and only make changes if we're clear it fixes a potential problem, otherwise suppress
##########
File path: dev/make-distribution.sh
##########
@@ -123,26 +123,26 @@ fi
if [ ! "$(command -v "$MVN")" ] ; then
echo -e "Could not locate Maven command: '$MVN'."
echo -e "Specify the Maven command with the --mvn flag"
- exit -1;
+ exit 255;
fi
-VERSION=$("$MVN" help:evaluate -Dexpression=project.version $@ \
+VERSION=$("$MVN" help:evaluate -Dexpression=project.version "$@" \
Review comment:
Same, this doesn't seem to apply
##########
File path: dev/create-release/release-build.sh
##########
@@ -181,7 +181,7 @@ BASE_PROFILES="-Pmesos -Pyarn -Pkubernetes"
PUBLISH_SCALA_2_13=1
SCALA_2_13_PROFILES="-Pscala-2.13"
-if [[ $SPARK_VERSION < "3.2" ]]; then
+if [ "$(echo $SPARK_VERSION | awk -F "." '{if ($1 <= 3 && $2 < 2) print $1}')" ]; then
Review comment:
Hm, nothing here is actually using decimals - it's a lexicographic comparison. This alternative is less readable, but I also think it's wrong? "2.4" would not pass this check. We can revert and suppress this warning, or fix the logic to handle 3.x and <= 2.x possibilities separately in this clause
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35763:
URL: https://github.com/apache/spark/pull/35763#issuecomment-1061839868
The explaination can be found here.
[SC2199](https://github.com/koalaman/shellcheck/wiki/SC2199) [SC2148](https://github.com/koalaman/shellcheck/wiki/SC2148) [SC2242](https://github.com/koalaman/shellcheck/wiki/SC2242) [SC2072](https://github.com/koalaman/shellcheck/wiki/SC2072) [SC2068](https://github.com/koalaman/shellcheck/wiki/SC2068) [SC1143](https://github.com/koalaman/shellcheck/wiki/SC1143) [SC2145](https://github.com/koalaman/shellcheck/wiki/SC2145) [SC2144](https://github.com/koalaman/shellcheck/wiki/SC2144) [SC2045](https://github.com/koalaman/shellcheck/wiki/SC2045)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830606765
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
"${BUILD_COMMAND[@]}"
# Make directories
rm -rf "$DISTDIR"
mkdir -p "$DISTDIR/jars"
echo "Spark $VERSION$GITREVSTRING built for Hadoop $SPARK_HADOOP_VERSION" > "$DISTDIR/RELEASE"
-echo "Build flags: $@" >> "$DISTDIR/RELEASE"
+echo "Build flags: $*" >> "$DISTDIR/RELEASE"
# Copy jars
cp "$SPARK_HOME"/assembly/target/scala*/jars/* "$DISTDIR/jars/"
# Only create the yarn directory if the yarn artifacts were built.
-if [ -f "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar ]; then
- mkdir "$DISTDIR/yarn"
- cp "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar "$DISTDIR/yarn"
-fi
+for file in "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar
Review comment:
Maybe we can use find to check file count first, then if there's only one file, use cp.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830505472
##########
File path: sbin/start-history-server.sh
##########
@@ -32,7 +32,7 @@ fi
# Any changes need to be reflected there.
CLASS="org.apache.spark.deploy.history.HistoryServer"
-if [[ "$@" = *--help ]] || [[ "$@" = *-h ]]; then
+if [[ "$*" = *--help ]] || [[ "$*" = *-h ]]; then
Review comment:
Same with here. We actually want to check whether the args is end of `-h` or `--help`,
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
"${BUILD_COMMAND[@]}"
# Make directories
rm -rf "$DISTDIR"
mkdir -p "$DISTDIR/jars"
echo "Spark $VERSION$GITREVSTRING built for Hadoop $SPARK_HADOOP_VERSION" > "$DISTDIR/RELEASE"
-echo "Build flags: $@" >> "$DISTDIR/RELEASE"
+echo "Build flags: $*" >> "$DISTDIR/RELEASE"
# Copy jars
cp "$SPARK_HOME"/assembly/target/scala*/jars/* "$DISTDIR/jars/"
# Only create the yarn directory if the yarn artifacts were built.
-if [ -f "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar ]; then
- mkdir "$DISTDIR/yarn"
- cp "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar "$DISTDIR/yarn"
-fi
+for file in "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar
Review comment:
It will fall if there are multiple files matching at runtime. If we remove this rule, it should be tested in GA.
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! "${tags[*]}" =~ "r" ]]; then
Review comment:
This is the [explaination](https://stackoverflow.com/questions/3348443/a-confusion-about-array-versus-array-in-the-context-of-a-bash-comple) for `${array[@]}` and `${array[*]}`. It seems that we always use it as `"${array[*]}"`, which combie array to one string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830495226
##########
File path: dev/make-distribution.sh
##########
@@ -170,24 +170,28 @@ BUILD_COMMAND=("$MVN" clean package -DskipTests $@)
# Actually build the jar
echo -e "\nBuilding with..."
-echo -e "\$ ${BUILD_COMMAND[@]}\n"
+echo -e "\$ ${BUILD_COMMAND[*]}\n"
"${BUILD_COMMAND[@]}"
# Make directories
rm -rf "$DISTDIR"
mkdir -p "$DISTDIR/jars"
echo "Spark $VERSION$GITREVSTRING built for Hadoop $SPARK_HADOOP_VERSION" > "$DISTDIR/RELEASE"
-echo "Build flags: $@" >> "$DISTDIR/RELEASE"
+echo "Build flags: $*" >> "$DISTDIR/RELEASE"
# Copy jars
cp "$SPARK_HOME"/assembly/target/scala*/jars/* "$DISTDIR/jars/"
# Only create the yarn directory if the yarn artifacts were built.
-if [ -f "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar ]; then
- mkdir "$DISTDIR/yarn"
- cp "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar "$DISTDIR/yarn"
-fi
+for file in "$SPARK_HOME"/common/network-yarn/target/scala*/spark-*-yarn-shuffle.jar
Review comment:
For the same reason as in https://github.com/koalaman/shellcheck/issues/1217 I think we want to suppress this. Looping doesn't make sense here anyway, even if there were multiple files matching
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! "${tags[*]}" =~ "r" ]]; then
Review comment:
Same
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r830543980
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! "${tags[*]}" =~ "r" ]]; then
Review comment:
cc @dongjoon-hyun. Could you look at this?
It seems that we run k8s integratation tests with `test.exclude.tags=minikube,r,local`, and the magic actually means to check `if one of the arguments is "r"`? If this, then `if [[ " ${tags[*]} " =~ " r " ]]; then` or a loop check can work well with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r828319101
##########
File path: build/sbt-launch-lib.bash
##########
@@ -186,7 +186,7 @@ run() {
${SBT_OPTS:-$default_sbt_opts} \
$(get_mem_opts $sbt_mem) \
${java_opts} \
- ${java_args[@]} \
+ "${java_args[@]}" \
Review comment:
I think this is ok, my one concern would be someone _might_ be depending on the shell expansion here for something like class path but since this is a build only tool I think that's an OK risk to take given the tradeoffs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r828633349
##########
File path: dev/create-release/release-build.sh
##########
@@ -341,8 +341,8 @@ if [[ "$1" == "package" ]]; then
fi
if [[ $PUBLISH_SCALA_2_12 = 1 ]]; then
- echo "Packages to build: ${!BINARY_PKGS_ARGS[@]}"
- for key in ${!BINARY_PKGS_ARGS[@]}; do
+ echo "Packages to build: ${!BINARY_PKGS_ARGS[*]}"
+ for key in "${!BINARY_PKGS_ARGS[@]}"; do
Review comment:
It's ok to me, we can skip this rule.
##########
File path: dev/create-release/release-build.sh
##########
@@ -181,7 +181,7 @@ BASE_PROFILES="-Pmesos -Pyarn -Pkubernetes"
PUBLISH_SCALA_2_13=1
SCALA_2_13_PROFILES="-Pscala-2.13"
-if [[ $SPARK_VERSION < "3.2" ]]; then
+if [ "$(echo $SPARK_VERSION | awk -F "." '{if ($1 <= 3 && $2 < 2) print $1}')" ]; then
Review comment:
There indeed has some proble here. `10.0` will not pass `$SPARK_VERSION < "3.2"`. The condition can be changed to `echo $SPARK_VERSION | awk -F "." '{if (($1 < 3) || ($1 == 3 && $2 < 2)) print $1}'`.
Hm, maybe skip the check and leave waning comment here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jackylee-ch commented on a change in pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35763:
URL: https://github.com/apache/spark/pull/35763#discussion_r832897327
##########
File path: resource-managers/kubernetes/integration-tests/scripts/setup-integration-test-env.sh
##########
@@ -118,7 +118,7 @@ then
# Build SparkR image
tags=(${EXCLUDE_TAGS//,/ })
- if [[ ! ${tags[@]} =~ "r" ]]; then
+ if [[ ! " ${tags[*]} " =~ " r " ]]; then
Review comment:
cc @dongjoon-hyun. Could you look at this?
It seems that we run k8s integratation tests with `test.exclude.tags=minikube,r,local`, and the logic here actually means to check if one of the arguments is "r"? If this, then `if [[ ! " ${tags[*]} " =~ " r " ]]; then` can work well with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35763: [SPARK-38433][BUILD] change the shell code style with shellcheck
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35763:
URL: https://github.com/apache/spark/pull/35763#issuecomment-1061516587
cc @dongjoon-hyun @HyukjinKwon @srowen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org