Commit d596ba87 authored by Michal Berger's avatar Michal Berger Committed by Jim Harris
Browse files

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
			 <<BASH redirect operator. Spaces can be used to
			 format the line only if it's already preceded
			 with a tab.

  * binary_next_line = true - Lines can start with logical operators. E.g:

			      if [[ -v foo ]] \
			      	&& [[ -v bar ]]; then
				 ...
			      fi
  * switch_case_indent = true - case|esac patterns are indented with tabs.

  * space_redirects = true - redirect operators are followed with a space.
			     E.g: > 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: default avatarMichal Berger <michalx.berger@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1418


Reviewed-by: default avatarKarol Latecki <karol.latecki@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarDarek Stojaczyk <dariusz.stojaczyk@intel.com>
Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 844c8ec3

scripts/check_format.sh

+78 −0
Original line number Diff line number Diff line
@@ -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..."

scripts/pkgdep.sh

+52 −0
Original line number Diff line number Diff line
@@ -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