bmarwell commented on code in PR #79:
URL: https://github.com/apache/maven-wrapper/pull/79#discussion_r1062317792
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -71,38 +71,38 @@ esac
if [ -z "$JAVA_HOME" ] ; then
if [ -r /etc/gentoo-release ] ; then
- JAVA_HOME=`java-config --jre-home`
+ JAVA_HOME=$(java-config --jre-home)
fi
fi
# For Cygwin, ensure paths are in UNIX format before anything is touched
if $cygwin ; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --unix "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --unix "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --unix "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --unix "$CLASSPATH")
fi
# For Mingw, ensure paths are in UNIX format before anything is touched
if $mingw ; then
- [ -n "$JAVA_HOME" ] &&
- JAVA_HOME="`(cd "$JAVA_HOME"; pwd)`"
+ [ -n "$JAVA_HOME" ] && [ -d "$JAVA_HOME" ] &&
+ JAVA_HOME="$(cd "$JAVA_HOME" || (echo "cannot cd into $JAVA_HOME."; exit
1); pwd)"
Review Comment:
This prevents going into a non-existent directory (i.e. stay in CWD) and
replace JAVA_HOME with basically the current working dir.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -281,20 +286,21 @@ MAVEN_OPTS="$(concat_lines
"$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS"
# For Cygwin, switch paths to Windows format before running java
if $cygwin; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --path --windows "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --path --windows "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --windows "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --windows "$CLASSPATH")
[ -n "$MAVEN_PROJECTBASEDIR" ] &&
- MAVEN_PROJECTBASEDIR=`cygpath --path --windows "$MAVEN_PROJECTBASEDIR"`
+ MAVEN_PROJECTBASEDIR=$(cygpath --path --windows "$MAVEN_PROJECTBASEDIR")
fi
# Provide a "standardized" way to retrieve the CLI args that will
# work with both Windows and non-Windows executions.
-MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $@"
+MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $*"
Review Comment:
WHile `$@` works, it will in fact expand to this:
`MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $1" "$2" "$3" "$4"` (etc)
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -71,38 +71,38 @@ esac
if [ -z "$JAVA_HOME" ] ; then
if [ -r /etc/gentoo-release ] ; then
- JAVA_HOME=`java-config --jre-home`
+ JAVA_HOME=$(java-config --jre-home)
fi
fi
# For Cygwin, ensure paths are in UNIX format before anything is touched
if $cygwin ; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --unix "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --unix "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --unix "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --unix "$CLASSPATH")
fi
# For Mingw, ensure paths are in UNIX format before anything is touched
if $mingw ; then
- [ -n "$JAVA_HOME" ] &&
- JAVA_HOME="`(cd "$JAVA_HOME"; pwd)`"
+ [ -n "$JAVA_HOME" ] && [ -d "$JAVA_HOME" ] &&
+ JAVA_HOME="$(cd "$JAVA_HOME" || (echo "cannot cd into $JAVA_HOME."; exit
1); pwd)"
fi
if [ -z "$JAVA_HOME" ]; then
- javaExecutable="`which javac`"
- if [ -n "$javaExecutable" ] && ! [ "`expr \"$javaExecutable\" : '\([^
]*\)'`" = "no" ]; then
+ javaExecutable="$(which javac)"
+ if [ -n "$javaExecutable" ] && ! [ "$(expr "\"$javaExecutable\"" : '\([^
]*\)')" = "no" ]; then
# readlink(1) is not available as standard on Solaris 10.
- readLink=`which readlink`
- if [ ! `expr "$readLink" : '\([^ ]*\)'` = "no" ]; then
+ readLink=$(which readlink)
+ if [ ! "$(expr "$readLink" : '\([^ ]*\)')" = "no" ]; then
if $darwin ; then
- javaHome="`dirname \"$javaExecutable\"`"
- javaExecutable="`cd \"$javaHome\" && pwd -P`/javac"
+ javaHome="$(dirname "\"$javaExecutable\"")"
+ javaExecutable="$(cd "\"$javaHome\"" && pwd -P)/javac"
else
- javaExecutable="`readlink -f \"$javaExecutable\"`"
+ javaExecutable="$(readlink -f "\"$javaExecutable\"")"
fi
- javaHome="`dirname \"$javaExecutable\"`"
- javaHome=`expr "$javaHome" : '\(.*\)/bin'`
+ javaHome="$(dirname "\"$javaExecutable\"")"
+ javaHome=$(expr "$javaHome" : '\(.*\)/bin')
Review Comment:
Not sure about those escaped quotes. Any idea why they exist?
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -193,42 +193,48 @@ else
else
wrapperUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/@@project.version@@/maven-wrapper-@@project.version@@.jar";
fi
- while IFS="=" read key value; do
+ while IFS="=" read -r key value; do
case "$key" in (wrapperUrl) wrapperUrl="$value"; break ;;
esac
done < "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.properties"
log "Downloading from: $wrapperUrl"
if $cygwin; then
- wrapperJarPath=`cygpath --path --windows "$wrapperJarPath"`
+ wrapperJarPath=$(cygpath --path --windows "$wrapperJarPath")
fi
if command -v wget > /dev/null; then
log "Found wget ... using wget"
+ wgetrc=0
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--quiet"
if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then
wget $QUIET "$wrapperUrl" -O "$wrapperJarPath"
+ wgetrc=$?
else
wget $QUIET --http-user="$MVNW_USERNAME"
--http-password="$MVNW_PASSWORD" "$wrapperUrl" -O "$wrapperJarPath"
+ wgetrc=$?
fi
- [ $? -eq 0 ] || rm -f "$wrapperJarPath"
+ [ $wgetrc -eq 0 ] || rm -f "$wrapperJarPath"
elif command -v curl > /dev/null; then
log "Found curl ... using curl"
+ curlrc=0
[ "$MVNW_VERBOSE" = true ] && QUIET="" || QUIET="--silent"
if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then
curl $QUIET -o "$wrapperJarPath" "$wrapperUrl" -f -L
+ curlrc=$?
else
curl $QUIET --user "$MVNW_USERNAME:$MVNW_PASSWORD" -o
"$wrapperJarPath" "$wrapperUrl" -f -L
+ curlrc=$?
fi
- [ $? -eq 0 ] || rm -f "$wrapperJarPath"
+ [ $curlrc -eq 0 ] || rm -f "$wrapperJarPath"
Review Comment:
instead of adding a new var, we could use sth like `if wget ...` directly. I
would really remove the original line 229 though (`[ $? -eq 0]`), because it is
easy to forget about it and add yet another command in between, e.g. a
debugging print or echo statement which will make the check wrong.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -193,42 +193,48 @@ else
else
wrapperUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/@@project.version@@/maven-wrapper-@@project.version@@.jar";
fi
- while IFS="=" read key value; do
+ while IFS="=" read -r key value; do
Review Comment:
without `-r` it will eat backslashes
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -150,17 +150,17 @@ find_maven_basedir() {
fi
# workaround for JBEAP-8937 (on Solaris 10/Sparc)
if [ -d "${wdir}" ]; then
- wdir=`cd "$wdir/.."; pwd`
+ wdir=$(cd "$wdir/.." || exit 1; pwd)
fi
# end of workaround
done
- printf '%s' "$(cd "$basedir"; pwd)"
+ printf '%s' "$(cd "$basedir" || exit 1; pwd)"
Review Comment:
We *could* print a message here using `|| ( echo ""; exit 1); pwd)`
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -237,8 +243,7 @@ else
fi
if [ -e "$javaClass" ]; then
log " - Running MavenWrapperDownloader.java ..."
- ("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader
"$wrapperUrl" "$wrapperJarPath")
- [ $? -eq 0 ] || rm -f "$wrapperJarPath"
+ ("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader
"$wrapperUrl" "$wrapperJarPath") || rm -f "$wrapperJarPath"
Review Comment:
not sure why the parenthesis are there. Kept it, but can probably be removed.
##########
maven-wrapper-distribution/src/resources/mvnw:
##########
@@ -281,20 +286,21 @@ MAVEN_OPTS="$(concat_lines
"$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS"
# For Cygwin, switch paths to Windows format before running java
if $cygwin; then
[ -n "$JAVA_HOME" ] &&
- JAVA_HOME=`cygpath --path --windows "$JAVA_HOME"`
+ JAVA_HOME=$(cygpath --path --windows "$JAVA_HOME")
[ -n "$CLASSPATH" ] &&
- CLASSPATH=`cygpath --path --windows "$CLASSPATH"`
+ CLASSPATH=$(cygpath --path --windows "$CLASSPATH")
[ -n "$MAVEN_PROJECTBASEDIR" ] &&
- MAVEN_PROJECTBASEDIR=`cygpath --path --windows "$MAVEN_PROJECTBASEDIR"`
+ MAVEN_PROJECTBASEDIR=$(cygpath --path --windows "$MAVEN_PROJECTBASEDIR")
fi
# Provide a "standardized" way to retrieve the CLI args that will
# work with both Windows and non-Windows executions.
-MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $@"
+MAVEN_CMD_LINE_ARGS="$MAVEN_CONFIG $*"
export MAVEN_CMD_LINE_ARGS
WRAPPER_LAUNCHER=org.apache.maven.wrapper.MavenWrapperMain
+# shellcheck disable=SC2086 # safe args
exec "$JAVACMD" \
$MAVEN_OPTS \
$MAVEN_DEBUG_OPTS \
Review Comment:
It would be better to use arrays here if possible.
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
