Merge JoinMarket-Org/joinmarket-clientserver#1414: Apply all current ShellCheck suggestions and add `lint-shell.sh`

22c13b0ab7 Remove unused code instead of commenting out (Kristaps Kaupe)
b928713880 Add ShellCheck linter script (Kristaps Kaupe)
4f0eebc68d Apply all current shellcheck suggestions to rest of the scripts (Kristaps Kaupe)
f0b9872ff9 Apply all current shellcheck suggestions to install.sh (Kristaps Kaupe)

Pull request description:

  Based on #1175, but also covers `install.sh` changes since then, all other shell scripts and adds ShellCheck linter script. Not adding this to CI for now. I'm thinking we could split off all the linting to separate job, don't see a point in running them under all OS / Python version combinations.

Top commit has no ACKs.

Tree-SHA512: 936320280f1ec9d8aa315c70be4d33fcb06d324531a17b30987c0be308d5351aa45dfac05cecadca8e9977a8f797b62ccd91d5d5fe92c5d0eacc6ebdf4dcd1c6

install.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit
 
 check_exists() {
     command -v "$1" > /dev/null
@@ -12,9 +12,9 @@ num_cores() {
 
 # This is needed for systems where GNU is not the default make, like FreeBSD.
 if check_exists gmake; then
-    make=gmake
+    make="gmake"
 else
-    make=make
+    make="make"
 fi
 
 sha256_verify ()
@@ -122,12 +122,12 @@ tor_deps_install ()
 
 deb_deps_check ()
 {
-    apt-cache policy ${deb_deps[@]} | grep "Installed.*none"
+    apt-cache policy "${deb_deps[@]}" | grep "Installed.*none"
 }
 
 deb_deps_install ()
 {
-    deb_deps=( ${@} )
+    deb_deps=( "${@}" )
     if deb_deps_check; then
         clear
         sudo_command=''
@@ -135,12 +135,12 @@ deb_deps_install ()
             echo "
                 sudo password required to run :
 
-                \`apt-get install ${deb_deps[@]}\`
+                \`apt-get install ${deb_deps[*]}\`
                 "
             sudo_command="sudo"
         fi
 
-        if ! $sudo_command apt-get install -y --no-install-recommends ${deb_deps[@]}; then
+        if ! $sudo_command apt-get install -y --no-install-recommends "${deb_deps[@]}"; then
               return 1
         fi
     fi
@@ -148,8 +148,8 @@ deb_deps_install ()
 
 dar_deps_install ()
 {
-    dar_deps=( ${@} )
+    dar_deps=( "${@}" )
-    if ! brew install ${dar_deps[@]}; then
+    if ! brew install "${dar_deps[@]}"; then
         return 1
     fi
 }
@@ -157,7 +157,7 @@ dar_deps_install ()
 check_skip_build ()
 {
     if [[ ${reinstall} == false ]] && [[ -d "$1" ]]; then
-        read -n 1 -p "Directory ${1} exists.  Remove and recreate? (y/N) " q
+        read -r -n 1 -p "Directory ${1} exists.  Remove and recreate? (y/N) " q
         echo ""
         if [[ "${q}" =~ Y|y ]]; then
             rm -rf "./${1}"
@@ -179,6 +179,7 @@ venv_setup ()
         reinstall='true'
     fi
     "${python}" -m venv "${jm_source}/jmvenv" || return 1
+    # shellcheck source=/dev/null
     source "${jm_source}/jmvenv/bin/activate" || return 1
     pip install --upgrade pip
     pip install --upgrade setuptools
@@ -190,7 +191,7 @@ dep_get ()
     pkg_name="$1" pkg_hash="$2" pkg_url="$3"
     pkg_pubkeys="$4" pkg_sig="$5" pkg_hash_file="$6" pkg_hash_file_sig="$7"
 
-    pushd cache
+    pushd cache || return 1
     if [ ! -f "${pkg_name}" ] || ! sha256_verify "${pkg_hash}" "${pkg_name}"; then
         http_get "${pkg_url}/${pkg_name}" "${pkg_name}"
     fi
@@ -213,7 +214,7 @@ dep_get ()
         gpg_verify "../../pubkeys/third-party/${pkg_pubkeys}" "${pkg_sig}"
     fi
     tar -xzf "${pkg_name}" -C ../
-    popd
+    popd || return 1
 }
 
 # add '--disable-docs' to libffi ./configure so makeinfo isn't needed
@@ -278,7 +279,7 @@ libffi_install ()
     if ! dep_get "${libffi_lib_tar}" "${libffi_lib_sha}" "${libffi_url}"; then
         return 1
     fi
-    pushd "${libffi_version}"
+    pushd "${libffi_version}" || return 1
     if ! libffi_patch_disable_docs; then
         return 1
     fi
@@ -287,7 +288,7 @@ libffi_install ()
     else
         return 1
     fi
-    popd
+    popd || return 1
 }
 
 libsecp256k1_build()
@@ -320,13 +321,13 @@ libsecp256k1_install()
     if ! dep_get "${secp256k1_lib_tar}.tar.gz" "${secp256k1_lib_sha}" "${secp256k1_lib_url}"; then
         return 1
     fi
-    pushd "secp256k1-${secp256k1_lib_tar}"
+    pushd "secp256k1-${secp256k1_lib_tar}" || return 1
     if libsecp256k1_build; then
         $make install
     else
         return 1
     fi
-    popd
+    popd || return 1
 }
 
 libsodium_build ()
@@ -359,13 +360,13 @@ libsodium_install ()
     if ! dep_get "${sodium_lib_tar}" "${sodium_lib_sha}" "${sodium_url}" "${sodium_pubkeys}" "${sodium_lib_tar}.sig"; then
         return 1
     fi
-    pushd "${sodium_version}"
+    pushd "${sodium_version}" || return 1
     if libsodium_build; then
         $make install
     else
         return 1
     fi
-    popd
+    popd || return 1
 }
 
 tor_root ()
@@ -411,7 +412,7 @@ tor_install ()
     if ! dep_get "${tor_tar}" "${tor_sha}" "${tor_url}" "${tor_pubkeys}" "" "${tor_tar}.sha256sum" "${tor_tar}.sha256sum.asc"; then
         return 1
     fi
-    pushd "${tor_version}"
+    pushd "${tor_version}" || return 1
     if tor_build; then
         $make install
         echo "# Default JoinMarket Tor configuration
@@ -423,7 +424,7 @@ CookieAuthentication 1
     else
         return 1
     fi
-    popd
+    popd || return 1
 }
 
 joinmarket_install ()
@@ -434,7 +435,7 @@ joinmarket_install ()
         reqs+=( 'gui.txt' )
     fi
 
-    for req in ${reqs[@]}; do
+    for req in "${reqs[@]}"; do
         if [ "$with_jmvenv" == 1 ]; then pip_command=pip; else pip_command=pip3; fi
         $pip_command install -r "requirements/${req}" || return 1
     done
@@ -444,8 +445,8 @@ joinmarket_install ()
             echo "Installing XDG desktop entry"
             cp -f "$(dirname "$0")/docs/images/joinmarket_logo.png" \
                 ~/.local/share/icons/
-            cat "$(dirname "$0")/joinmarket-qt.desktop" | \
+            sed "s/\\\$JMHOME/$(dirname "$(realpath "$0")" | sed 's/\//\\\//g')/" \
-                sed "s/\\\$JMHOME/$(dirname "$(realpath "$0")" | sed 's/\//\\\//g')/" > \
+                "$(dirname "$0")/joinmarket-qt.desktop" > \
                     ~/.local/share/applications/joinmarket-qt.desktop
         fi
     fi
@@ -456,7 +457,7 @@ parse_flags ()
     while :; do
         case $1 in
             --develop)
-                develop_build='1'
+                # no-op for backwards compatibility
                 ;;
             --disable-os-deps-check)
                 use_os_deps_check='0'
@@ -504,7 +505,7 @@ parse_flags ()
                     echo "Invalid option $1"
                 fi
                 echo "
-Usage: "${0}" [options]
+Usage: ${0} [options]
 
 Options:
 
@@ -525,7 +526,7 @@ Options:
     done
 
     if [[ ${with_qt} == '' ]]; then
-        read -n 1 -p "
+        read -r -n 1 -p "
         INFO: Joinmarket-Qt for GUI Taker and Tumbler modes is available.
         Install Qt dependencies (~160mb)? (y/N) "
         echo ""
@@ -573,7 +574,6 @@ install_get_os ()
 main ()
 {
     # flags
-    develop_build=''
     python='python3'
     build_local_tor=''
     no_gpg_validation=''
@@ -583,7 +583,7 @@ main ()
     with_jmvenv='1'
     with_sudo='1'
     reinstall='false'
-    if ! parse_flags ${@}; then
+    if ! parse_flags "${@}"; then
         return 1
     fi
 
@@ -593,11 +593,10 @@ main ()
     else
         jm_root=""
     fi
-    jm_deps="${jm_source}/deps"
     export PKG_CONFIG_PATH="${jm_root}/lib/pkgconfig:${PKG_CONFIG_PATH}"
     export LD_LIBRARY_PATH="${jm_root}/lib:${LD_LIBRARY_PATH}"
     export C_INCLUDE_PATH="${jm_root}/include:${C_INCLUDE_PATH}"
-    export MAKEFLAGS="-j $(num_cores)"
+    MAKEFLAGS="-j $(num_cores)" && export MAKEFLAGS
 
     # os check
     install_os="$( install_get_os )"
@@ -611,6 +610,7 @@ main ()
             echo "Joinmarket Python virtual environment could not be setup. Exiting."
             return 1
         fi
+        # shellcheck source=/dev/null
         source "${jm_root}/bin/activate"
     fi
     if [[ ${build_local_tor} == "1" ]]; then
@@ -620,7 +620,7 @@ main ()
         fi
     fi
     mkdir -p "deps/cache"
-    pushd deps
+    pushd deps || return 1
     if ! libsecp256k1_install; then
         echo "libsecp256k1 was not built. Exiting."
         return 1
@@ -639,7 +639,7 @@ main ()
             return 1
         fi
     fi
-    popd
+    popd || return 1
     if ! joinmarket_install; then
         echo "Joinmarket was not installed. Exiting."
         if [ "$with_jmvenv" == 1 ]; then deactivate; fi
@@ -655,4 +655,4 @@ main ()
         from this directory, to activate the virtual environment."
     fi
 }
-main ${@}
+main "${@}"

scripts/joinmarket-qt.sh

@@ -1,5 +1,6 @@
 #!/usr/bin/env bash
-cd $(dirname "$0")/.. && \
+# shellcheck source=/dev/null
+cd "$(dirname "$0")/.." && \
 source jmvenv/bin/activate && \
 cd scripts && \
 python3 joinmarket-qt.py

test/Dockerfiles/build_docker.sh

@@ -20,10 +20,9 @@ build_docker ()
     core_url="https://bitcoincore.org/bin/bitcoin-core-${core_version}/${core_dist}"
     declare -A deps=( [${core_dist}]="${core_url}" )
     jm_root="${TRAVIS_BUILD_DIR}"
-    owner_name="${TRAVIS_REPO_SLUG%\/*}"
     repo_name="${TRAVIS_REPO_SLUG#*\/}"
 
-    for dep in ${!deps[@]}; do
+    for dep in "${!deps[@]}"; do
         if [[ ! -r "${HOME}/downloads/${dep}" ]]; then
             curl --retry 5 -L "${deps[${dep}]}" -o "$HOME/downloads/${dep}"
         fi
@@ -31,7 +30,7 @@ build_docker ()
 
     mkdir -p "${jm_root}/deps/cache"
     find "$HOME/downloads" -type f -exec cp -v {} "${jm_root}/deps/cache/" \;
-    cd "${jm_root}/../"
+    cd "${jm_root}/../" || return 1
 
     docker build \
         --shm-size=1G \

test/lint/lint-all.sh

@@ -0,0 +1,30 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2017-2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#
+# This script runs all contrib/devtools/lint-* files, and fails if any exit
+# with a non-zero status code.
+
+# This script is intentionally locale dependent by not setting "export LC_ALL=C"
+# in order to allow for the executed lint scripts to opt in or opt out of locale
+# dependence themselves.
+
+set -u
+
+SCRIPTDIR=$(dirname "${BASH_SOURCE[0]}")
+LINTALL=$(basename "${BASH_SOURCE[0]}")
+
+EXIT_CODE=0
+
+for f in "${SCRIPTDIR}"/lint-*; do
+  if [ "$(basename "$f")" != "$LINTALL" ]; then
+    if ! "$f"; then
+      echo "^---- failure generated from $f"
+      EXIT_CODE=1
+    fi
+  fi
+done
+
+exit ${EXIT_CODE}

test/lint/lint-python.sh

@@ -13,6 +13,7 @@ elif flake8 --version | grep -q "Python 2"; then
 fi
 
 if [[ $# == 0 ]]; then
+    # shellcheck disable=SC2046
     flake8 $(git ls-files "*.py") --extend-exclude "${EXCLUDE_PATTERNS}"
 else
     flake8 "$@"

test/lint/lint-shell.sh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2018-2021 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#
+# Check for shellcheck warnings in shell scripts.
+
+export LC_ALL=C
+
+# Disabled warnings:
+disabled=(
+)
+
+EXIT_CODE=0
+
+if ! command -v shellcheck > /dev/null; then
+    echo "Skipping shell linting since shellcheck is not installed."
+    exit $EXIT_CODE
+fi
+
+SHELLCHECK_CMD=(shellcheck --external-sources --check-sourced --source-path=SCRIPTDIR)
+EXCLUDE="--exclude=$(IFS=','; echo "${disabled[*]}")"
+# Check shellcheck directive used for sourced files
+mapfile -t SOURCED_FILES < <(git ls-files | xargs gawk '/^# shellcheck shell=/ {print FILENAME} {nextfile}')
+mapfile -t GUIX_FILES < <(git ls-files contrib/guix contrib/shell | xargs gawk '/^#!\/usr\/bin\/env bash/ {print FILENAME} {nextfile}')
+mapfile -t FILES < <(git ls-files -- '*.sh' | grep -vE 'src/(leveldb|secp256k1|minisketch|univalue)/')
+if ! "${SHELLCHECK_CMD[@]}" "$EXCLUDE" "${SOURCED_FILES[@]}" "${GUIX_FILES[@]}" "${FILES[@]}"; then
+    EXIT_CODE=1
+fi
+
+exit $EXIT_CODE

test/run_tests.sh

@@ -126,7 +126,7 @@ parse_flags ()
                     echo "Invalid option $1"
                 fi
                 echo "
-Usage: "${0}" [options]
+Usage: ${0} [options]
 
 Options:
 
@@ -155,7 +155,7 @@ run_jm_tests ()
     btcuser="bitcoinrpc"
     btcpwd="123456abcdef"
     nirc="2"
-    if ! parse_flags ${@}; then
+    if ! parse_flags "${@}"; then
         return 1
     fi
 
@@ -176,7 +176,7 @@ run_jm_tests ()
     export LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:${VIRTUAL_ENV}/lib"
     export C_INCLUDE_PATH="${C_INCLUDE_PATH}:${VIRTUAL_ENV}/include"
 
-    pushd "${jm_source}"
+    pushd "${jm_source}" || return 1
     if [ ! -f 'miniircd.tar.gz' ] || ! sha256_verify 'ce3a4ddc777343645ccd06ca36233b5777e218ee89d887ef529ece86a917fc33' 'miniircd.tar.gz'; then
         http_get "https://github.com/JoinMarket-Org/miniircd/archive/master.tar.gz" "miniircd.tar.gz"
     fi
@@ -217,18 +217,18 @@ run_jm_tests ()
     echo "datadir=${jm_test_datadir}" >> "${jm_test_datadir}/bitcoin.conf"
     python -m pytest $additional_pytest_flags \
         ${HAS_JOSH_K_SEAL_OF_APPROVAL+--cov=jmclient --cov=jmbitcoin --cov=jmbase --cov=jmdaemon --cov-report html} \
-        --btcconf=$btcconf \
+        --btcconf="$btcconf" \
-        --btcpwd=$btcpwd \
+        --btcpwd="$btcpwd" \
-        --btcroot=$btcroot \
+        --btcroot="$btcroot" \
-        --btcuser=$btcuser \
+        --btcuser="$btcuser" \
-        --nirc=$nirc \
+        --nirc="$nirc" \
         -p no:warnings
     local success="$?"
     [[ -f ./joinmarket.cfg ]] && unlink ./joinmarket.cfg
-    if [ -f "${jm_test_datadir}/bitcoind.pid" ] && read bitcoind_pid <"${jm_test_datadir}/bitcoind.pid"; then
+    if [ -f "${jm_test_datadir}/bitcoind.pid" ] && read -r bitcoind_pid < "${jm_test_datadir}/bitcoind.pid"; then
-        kill -15 ${bitcoind_pid} || kill -9 ${bitcoind_pid}
+        kill -15 "${bitcoind_pid}" || kill -9 "${bitcoind_pid}"
     fi
-    if [[ "${HAS_JOSH_K_SEAL_OF_APPROVAL}" == true ]] && (( ${success} != 0 )); then
+    if [[ "${HAS_JOSH_K_SEAL_OF_APPROVAL}" == true ]] && (( success != 0 )); then
         tail -100 "${jm_test_datadir}/regtest/debug.log"
         find "${jm_test_datadir}"
     else
@@ -236,4 +236,4 @@ run_jm_tests ()
     fi
     return ${success:-1}
 }
-run_jm_tests ${@}
+run_jm_tests "${@}"