From d596ba87b270846b4defa2d5aa4528fd84faea79 Mon Sep 17 00:00:00 2001 From: Michal Berger Date: Mon, 23 Mar 2020 15:14:18 +0100 Subject: [PATCH] check_format: Introduce shfmt tooling for .sh style enforcement Add new dev tool for enforcing proper formatting of the Bash code across the entire repo. This is done in order of defining a common set of good practices to follow when writing .sh|Bash code. As powerful as shfmt may be, it allows only for some specific rules to be enforced, hence it still needs to work side by side with shellcheck syntax-wise. If it comes to style, following rules are being enforced: * indent_style = tab - Lines must be indented with tabs. The exception from this rule is the use of heredocs with < foo over >foo. In addition, shfmt will enforce its own Bash-style for different parts of the code as well. Examples and more details can be found here: https://github.com/mvdan/sh Change-Id: I6e5c8d79e6dba9c6471010f3d0f563dd34e62fd6 Signed-off-by: Michal Berger Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1418 Reviewed-by: Karol Latecki Reviewed-by: Ben Walker Reviewed-by: Jim Harris Reviewed-by: Darek Stojaczyk Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins --- scripts/check_format.sh | 78 +++++++++++++++++++++++++++++++++++++++++ scripts/pkgdep.sh | 52 +++++++++++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/scripts/check_format.sh b/scripts/check_format.sh index 6eee1bf639..bc724fb9b1 100755 --- a/scripts/check_format.sh +++ b/scripts/check_format.sh @@ -227,6 +227,84 @@ else echo "You do not have pycodestyle or pep8 installed so your Python style is not being checked!" fi +shfmt="shfmt-3.1.0" +if hash "$shfmt" 2> /dev/null; then + shfmt_cmdline=() silly_plural=() + + silly_plural[1]="s" + + commits=() sh_files=() sh_files_repo=() sh_files_staged=() + + mapfile -t sh_files_repo < <(git ls-files '*.sh') + # Fetch .sh files only from the commits that are targeted for merge + while read -r _ commit; do + commits+=("$commit") + done < <(git cherry -v origin/master) + + mapfile -t sh_files < <(git diff --name-only HEAD origin/master "${sh_files_repo[@]}") + # In case of a call from a pre-commit git hook + mapfile -t sh_files_staged < <( + IFS="|" + git diff --cached --name-only "${sh_files_repo[@]}" | grep -v "${sh_files[*]}" + ) + + if ((${#sh_files[@]})); then + printf 'Checking .sh formatting style...\n\n' + printf ' Found %u updated|new .sh file%s from the following commits:\n' \ + "${#sh_files[@]}" "${silly_plural[${#sh_files[@]} > 1 ? 1 : 0]}" + printf ' * %s\n' "${commits[@]}" + + if ((${#sh_files_staged[@]})); then + printf ' Found %u .sh file%s in staging area:\n' \ + "${#sh_files_staged[@]}" "${silly_plural[${#sh_files_staged[@]} > 1 ? 1 : 0]}" + printf ' * %s\n' "${sh_files_staged[@]}" + sh_files+=("${sh_files_staged[@]}") + fi + + printf ' Running %s against the following file%s:\n' "$shfmt" "${silly_plural[${#sh_files[@]} > 1 ? 1 : 0]}" + printf ' * %s\n' "${sh_files[@]}" + + shfmt_cmdline+=(-i 0) # indent_style = tab|indent_size = 0 + shfmt_cmdline+=(-bn) # binary_next_line = true + shfmt_cmdline+=(-ci) # switch_case_indent = true + shfmt_cmdline+=(-ln bash) # shell_variant = bash (default) + shfmt_cmdline+=(-d) # diffOut - print diff of the changes and exit with != 0 + shfmt_cmdline+=(-sr) # redirect operators will be followed by a space + + diff=$PWD/$shfmt.patch + + # Explicitly tell shfmt to not look for .editorconfig. .editorconfig is also not looked up + # in case any formatting arguments has been passed on its cmdline. + if ! SHFMT_NO_EDITORCONFIG=true "$shfmt" "${shfmt_cmdline[@]}" "${sh_files[@]}" > "$diff"; then + # In case shfmt detects an actual syntax error it will write out a proper message on + # its stderr, hence the diff file should remain empty. + if [[ -s $diff ]]; then + diff_out=$(< "$diff") + fi + + cat <<- ERROR_SHFMT + + * Errors in style formatting have been detected. + ${diff_out:+* Please, review the generated patch at $diff + + # _START_OF_THE_DIFF + + ${diff_out:-ERROR} + + # _END_OF_THE_DIFF + } + + ERROR_SHFMT + rc=1 + else + rm -f "$diff" + printf 'OK\n' + fi + fi +else + printf '%s not detected, Bash style formatting check is skipped\n' "$shfmt" +fi + if hash shellcheck 2> /dev/null; then echo -n "Checking Bash style..." diff --git a/scripts/pkgdep.sh b/scripts/pkgdep.sh index 456aa30629..4152ca6c34 100755 --- a/scripts/pkgdep.sh +++ b/scripts/pkgdep.sh @@ -28,6 +28,53 @@ function install_all_dependencies() { INSTALL_DOCS=true } +function install_shfmt() { + # Fetch version that has been tested + local shfmt_version=3.1.0 + local shfmt=shfmt-$shfmt_version + local shfmt_dir=${SHFMT_DIR:-/opt/shfmt} + local shfmt_dir_out=${SHFMT_DIR_OUT:-/usr/bin} + local shfmt_url + local os + + if hash "$shfmt" && [[ $("$shfmt" --version) == "v$shfmt_version" ]]; then + echo "$shfmt already installed" + return 0 + fi 2> /dev/null + + os=$(uname -s) + + case "$os" in + Linux) shfmt_url=https://github.com/mvdan/sh/releases/download/v$shfmt_version/shfmt_v${shfmt_version}_linux_amd64 ;; + FreeBSD) shfmt_url=https://github.com/mvdan/sh/releases/download/v$shfmt_version/shfmt_v${shfmt_version}_freebsd_amd64 ;; + *) + echo "Not supported OS (${os:-Unknown}), skipping" + return 0 + ;; + esac + + mkdir -p "$shfmt_dir" + mkdir -p "$shfmt_dir_out" + + echo "Fetching ${shfmt_url##*/}"... + local err + if err=$(curl -f -Lo"$shfmt_dir/$shfmt" "$shfmt_url" 2>&1); then + chmod +x "$shfmt_dir/$shfmt" + ln -sf "$shfmt_dir/$shfmt" "$shfmt_dir_out" + else + cat <<- CURL_ERR + + * Fetching $shfmt_url failed, $shfmt will not be available for format check. + * Error: + + $err + + CURL_ERR + return 0 + fi + echo "$shfmt installed" +} + INSTALL_CRYPTO=false INSTALL_DEV_TOOLS=false INSTALL_PMEM=false @@ -119,6 +166,7 @@ if [ -s /etc/redhat-release ]; then yum install -y git astyle sg3_utils pciutils # Additional (optional) dependencies for showing backtrace in logs yum install -y libunwind-devel || true + install_shfmt fi if [[ $INSTALL_PMEM == "true" ]]; then # Additional dependencies for building pmem based backends @@ -167,6 +215,7 @@ elif [ -f /etc/debian_version ]; then apt-get install -y libunwind-dev || true # Additional dependecies for nvmf performance test script apt-get install -y python3-paramiko + install_shfmt fi if [[ $INSTALL_PMEM == "true" ]]; then # Additional dependencies for building pmem based backends @@ -205,6 +254,7 @@ elif [ -f /etc/SuSE-release ] || [ -f /etc/SUSE-brand ]; then pciutils ShellCheck # Additional (optional) dependencies for showing backtrace in logs zypper install libunwind-devel || true + install_shfmt fi if [[ $INSTALL_PMEM == "true" ]]; then # Additional dependencies for building pmem based backends @@ -232,6 +282,7 @@ elif [ $(uname -s) = "FreeBSD" ]; then # Tools for developers pkg install -y devel/astyle bash py27-pycodestyle \ misc/e2fsprogs-libuuid sysutils/sg3_utils nasm + install_shfmt fi if [[ $INSTALL_DOCS == "true" ]]; then # Additional dependencies for building docs @@ -272,6 +323,7 @@ elif [ -f /etc/arch-release ]; then makepkg -si --needed --noconfirm; cd .. && rm -rf lcov-git; popd" + install_shfmt fi if [[ $INSTALL_PMEM == "true" ]]; then # Additional dependencies for building pmem based backends