From e10573088aaef8e5172960314dd18bcb8cd4bb70 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Thu, 24 Apr 2025 00:38:30 +0200 Subject: [PATCH] Clean up shell scripts Some changes fix actual problems, e.g. missing spaces in square bracket tests, and backticks unintentionally causing code execution when intended as formatting. Others, such as conservative quoting probably work fine in the old version in most situations, but it's nice to have some additional safety. Using `{ ..; }` instead of `(..)` is just a small performance enhancement. Many of these issues were identified by shellcheck, which might be useful in CI as well. --- benchmarks/driver.sh | 2 +- build_tools/git_version_gen.sh | 4 ++-- build_tools/mac_notarize.sh | 10 ++++------ build_tools/make_pkg.sh | 8 ++++---- build_tools/make_vendor_tarball.sh | 2 +- build_tools/osx_package_scripts/add-shell | 13 ++++++------- build_tools/osx_package_scripts/postinstall | 2 +- build_tools/osx_package_scripts/preinstall | 4 ++-- docker/context/fish_run_tests.sh | 8 +++++--- docker/docker_run_tests.sh | 9 +++++---- osx/install.sh | 2 +- 11 files changed, 32 insertions(+), 32 deletions(-) diff --git a/benchmarks/driver.sh b/benchmarks/driver.sh index 46891c880..b2b9db1ce 100755 --- a/benchmarks/driver.sh +++ b/benchmarks/driver.sh @@ -1,6 +1,6 @@ #!/bin/sh -if [ "$#" -gt 2 -o "$#" -eq 0 ]; then +if [ "$#" -gt 2 ] || [ "$#" -eq 0 ]; then echo "Usage: driver.sh /path/to/fish [/path/to/other/fish]" exit 1 fi diff --git a/build_tools/git_version_gen.sh b/build_tools/git_version_gen.sh index fde954793..fcf73c3f1 100755 --- a/build_tools/git_version_gen.sh +++ b/build_tools/git_version_gen.sh @@ -47,7 +47,7 @@ then # HACK: Git failed, so we keep the current version file. # This helps in case you built fish as a normal user # and then try to `sudo make install` it. - date +%s > ${OUTPUT_DIR}fish-build-version-witness.txt + date +%s > "${OUTPUT_DIR}"fish-build-version-witness.txt exit 0 fi @@ -67,4 +67,4 @@ test "$VN" = "$VC" || { # Output the fish-build-version-witness.txt # See https://cmake.org/cmake/help/v3.4/policy/CMP0058.html -date +%s > ${OUTPUT_DIR}fish-build-version-witness.txt +date +%s > "${OUTPUT_DIR}"fish-build-version-witness.txt diff --git a/build_tools/mac_notarize.sh b/build_tools/mac_notarize.sh index 8d5eef396..502a582e6 100755 --- a/build_tools/mac_notarize.sh +++ b/build_tools/mac_notarize.sh @@ -1,4 +1,4 @@ -#!/usr/bin/env bash +#!/bin/sh # Helper to notarize an .app.zip or .pkg file. @@ -13,7 +13,7 @@ for INPUT in "$@"; do echo "Processing $INPUT" test -f "$INPUT" || die "Not a file: $INPUT" ext="${INPUT##*.}" - (test "$ext" = "zip" || test "$ext" = "pkg") || die "Unrecognized extension: $ext" + { test "$ext" = "zip" || test "$ext" = "pkg"; } || die "Unrecognized extension: $ext" xcrun notarytool submit "$INPUT" --keychain-profile AC_PASSWORD --wait @@ -21,9 +21,7 @@ for INPUT in "$@"; do TMPDIR=$(mktemp -d) echo "Extracting to $TMPDIR" unzip -q "$INPUT" -d "$TMPDIR" - # Force glob expansion. - STAPLE_TARGET="$TMPDIR"/* - STAPLE_TARGET=$(echo $STAPLE_TARGET) + STAPLE_TARGET=$(echo "$TMPDIR"/*) else STAPLE_TARGET="$INPUT" fi @@ -35,7 +33,7 @@ for INPUT in "$@"; do INPUT_FULL=$(realpath "$INPUT") rm -f "$INPUT" cd "$(dirname "$STAPLE_TARGET")" - zip -r -q "$INPUT_FULL" $(basename "$STAPLE_TARGET") + zip -r -q "$INPUT_FULL" "$(basename "$STAPLE_TARGET")" fi echo "Processed $INPUT" diff --git a/build_tools/make_pkg.sh b/build_tools/make_pkg.sh index 6f70b63db..3188f95f6 100755 --- a/build_tools/make_pkg.sh +++ b/build_tools/make_pkg.sh @@ -12,7 +12,7 @@ usage() { echo " -p Password for the .p12 files (necessary to access the certificates)" echo " -e (Optional) Path to an entitlements XML file" echo " -n Enables notarization. This will fail if code signing is not also enabled." - echo " -j Path to JSON file generated with `rcodesign encode-app-store-connect-api-key` (required for notarization)" + echo " -j Path to JSON file generated with \`rcodesign encode-app-store-connect-api-key\` (required for notarization)" echo exit 1 } @@ -45,7 +45,7 @@ while getopts "sf:i:p:e:nj:" opt; do esac done -if [ -n "$SIGN" ] && ([ -z "$P12_APP_FILE" ] || [-z "$P12_INSTALL_FILE"] || [ -z "$P12_PASSWORD" ]); then +if [ -n "$SIGN" ] && { [ -z "$P12_APP_FILE" ] || [ -z "$P12_INSTALL_FILE" ] || [ -z "$P12_PASSWORD" ]; }; then usage fi @@ -103,7 +103,7 @@ mkdir -p "$PKGDIR/build_x86_64" "$PKGDIR/build_arm64" "$PKGDIR/root" "$PKGDIR/in # Fatten them up. for FILE in "$PKGDIR"/root/usr/local/bin/*; do - X86_FILE="$PKGDIR/build_x86_64/$(basename $FILE)" + X86_FILE="$PKGDIR/build_x86_64/$(basename "$FILE")" rcodesign macho-universal-create --output "$FILE" "$FILE" "$X86_FILE" chmod 755 "$FILE" done @@ -145,7 +145,7 @@ fi # Make the app's /usr/local/bin binaries universal. Note fish.app/Contents/MacOS/fish already is, courtesy of CMake. cd "$PKGDIR/build_arm64" for FILE in fish.app/Contents/Resources/base/usr/local/bin/*; do - X86_FILE="$PKGDIR/build_x86_64/fish.app/Contents/Resources/base/usr/local/bin/$(basename $FILE)" + X86_FILE="$PKGDIR/build_x86_64/fish.app/Contents/Resources/base/usr/local/bin/$(basename "$FILE")" rcodesign macho-universal-create --output "$FILE" "$FILE" "$X86_FILE" # macho-universal-create screws up the permissions. diff --git a/build_tools/make_vendor_tarball.sh b/build_tools/make_vendor_tarball.sh index 93443f5df..f8c878441 100755 --- a/build_tools/make_vendor_tarball.sh +++ b/build_tools/make_vendor_tarball.sh @@ -45,7 +45,7 @@ cd "$PREFIX_TMPDIR" mkdir .cargo cargo vendor --manifest-path "$wd/Cargo.toml" > .cargo/config.toml -tar cfvJ $path.xz vendor .cargo +tar cfvJ "$path".xz vendor .cargo cd - rm -r "$PREFIX_TMPDIR" diff --git a/build_tools/osx_package_scripts/add-shell b/build_tools/osx_package_scripts/add-shell index d12fc0620..e3992477d 100755 --- a/build_tools/osx_package_scripts/add-shell +++ b/build_tools/osx_package_scripts/add-shell @@ -4,12 +4,12 @@ if test $# -eq 0 then - echo usage: $0 shellname [shellname ...] + echo "usage: $0 shellname [shellname ...]" exit 1 fi -scriptname=`basename "$0"` -if [[ $UID -ne 0 ]]; then +scriptname=$(basename "$0") +if [ "$(id -u)" -ne 0 ]; then echo "${scriptname} must be run as root" exit 1 fi @@ -20,6 +20,7 @@ tmpfile=${file}.tmp set -o noclobber +# shellcheck disable=SC2064 trap "rm -f $tmpfile" EXIT if ! cat $file > $tmpfile @@ -32,15 +33,13 @@ EOF fi # Append a newline if it doesn't exist -if [ "$(tail -c1 "$tmpfile"; echo x)" != $'\nx' ]; then - echo "" >> "$tmpfile" -fi +[ -z "$(tail -c1 "$tmpfile")" ] || echo "" >> "$tmpfile" for i do if ! grep -q "^${i}$" "$tmpfile" then - echo $i >> "$tmpfile" + echo "$i" >> "$tmpfile" fi done diff --git a/build_tools/osx_package_scripts/postinstall b/build_tools/osx_package_scripts/postinstall index abd59603a..addb5f7a1 100755 --- a/build_tools/osx_package_scripts/postinstall +++ b/build_tools/osx_package_scripts/postinstall @@ -1,3 +1,3 @@ #!/bin/sh -x -./add-shell ${DSTVOLUME}usr/local/bin/fish +./add-shell "${DSTVOLUME}"usr/local/bin/fish diff --git a/build_tools/osx_package_scripts/preinstall b/build_tools/osx_package_scripts/preinstall index 359174617..451d1a6fc 100755 --- a/build_tools/osx_package_scripts/preinstall +++ b/build_tools/osx_package_scripts/preinstall @@ -1,7 +1,7 @@ #!/bin/sh -x echo "Removing any previous installation" -pkgutil --pkg-info ${INSTALL_PKG_SESSION_ID} && pkgutil --only-files --files ${INSTALL_PKG_SESSION_ID} | while read installed - do rm -v ${DSTVOLUME}${installed} +pkgutil --pkg-info "${INSTALL_PKG_SESSION_ID}" && pkgutil --only-files --files "${INSTALL_PKG_SESSION_ID}" | while read -r installed + do rm -v "${DSTVOLUME}${installed}" done echo "... removed" diff --git a/docker/context/fish_run_tests.sh b/docker/context/fish_run_tests.sh index 2adb0f21d..75697d566 100755 --- a/docker/context/fish_run_tests.sh +++ b/docker/context/fish_run_tests.sh @@ -1,4 +1,6 @@ -#!/bin/bash +#!/bin/sh + +set -e # This script is copied into the root directory of our Docker tests. # It is the entry point for running Docker-based tests. @@ -10,10 +12,10 @@ cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug /fish-source "$@" # Spawn a shell if FISH_RUN_SHELL_BEFORE_TESTS is set. if test -n "$FISH_RUN_SHELL_BEFORE_TESTS" then - bash -i || exit + bash -i fi -ninja && ninja fish_run_tests +(set +e; ninja && ninja fish_run_tests) RES=$? # Drop the user into a shell if FISH_RUN_SHELL_AFTER_TESTS is set. diff --git a/docker/docker_run_tests.sh b/docker/docker_run_tests.sh index f6ee6d494..9d6bc00de 100755 --- a/docker/docker_run_tests.sh +++ b/docker/docker_run_tests.sh @@ -1,8 +1,8 @@ -#!/bin/bash +#!/bin/sh usage() { cat << EOF -Usage: $(basename $0) [--shell-before] [--shell-after] DOCKERFILE +Usage: $(basename "$0") [--shell-before] [--shell-after] DOCKERFILE Options: --shell-before Before the tests start, run a bash shell --shell-after After the tests end, run a bash shell @@ -18,7 +18,7 @@ export DOCKER_BUILDKIT=1 set -e # Get fish source directory. -FISH_SRC_DIR=$(cd "$( dirname "${BASH_SOURCE[0]}" )"/.. >/dev/null && pwd) +FISH_SRC_DIR=$(cd "$( dirname "$0" )"/.. >/dev/null && pwd) # Parse args. while [ $# -gt 1 ]; do @@ -36,7 +36,7 @@ while [ $# -gt 1 ]; do shift done -DOCKERFILE=${@:$OPTIND:1} +DOCKERFILE="$1" test -n "$DOCKERFILE" || usage # Construct a docker image. @@ -47,6 +47,7 @@ docker build \ "$FISH_SRC_DIR"/docker/context/ # Run tests in it, allowing them to fail without failing this script. +# shellcheck disable=SC2086 # $DOCKER_EXTRA_ARGS should have globbing and splitting applied. docker run -it \ --mount type=bind,source="$FISH_SRC_DIR",target=/fish-source,readonly \ $DOCKER_EXTRA_ARGS \ diff --git a/osx/install.sh b/osx/install.sh index 6e3ce6cb2..ca8063f3a 100755 --- a/osx/install.sh +++ b/osx/install.sh @@ -5,7 +5,7 @@ set -e # Make sure we're run as root scriptname=$(basename "$0") -if [ "$UID" -ne 0 ]; then +if [ "$(id -u)" -ne 0 ]; then echo "${scriptname} must be run as root" exit 1 fi