CI: report script and doc check findings on the changed lines

.github/workflows/07-shellcheck.yml

@@ -0,0 +1,49 @@
+# Static analysis of the shell scripts changed by a pull request with
+# ShellCheck (https://www.shellcheck.net/). Many existing scripts have
+# known findings, so this fails only for new problems of "warning" or
+# "error" level (new "info" and "style" ones are shown as notices); the
+# findings are shown as annotations on the changed lines, and listed in
+# the job summary. Deliberate portability idioms of NUT scripts are
+# disabled in .shellcheckrc file.
+# Run tools/shellcheck-ci.pl locally for the same check.
+#
+# Runs on pull_request (not pull_request_target), so for PRs from forks
+# it has a read-only token and runs the copy of this workflow in the PR,
+# which is fine as it needs no write permissions.
+
+name: "GHA-07: ShellCheck"
+
+on:
+  pull_request:
+
+permissions: {}
+
+jobs:
+  shellcheck:
+    name: "ShellCheck new findings in changed scripts"
+    runs-on: ubuntu-24.04
+    permissions:
+      contents: read
+
+    steps:
+    # The pull_request checkout is a merge commit of the PR into its
+    # base branch, so its first parent is the base to compare with
+    - name: Checkout repository
+      uses: actions/checkout@v7
+      with:
+        fetch-depth: 2
+        persist-credentials: false
+
+    # ShellCheck is usually pre-installed in GitHub-hosted Ubuntu runners;
+    # both base and PR versions are checked with the same one, so its
+    # exact version does not matter much
+    - name: Install ShellCheck
+      run: |
+        command -v shellcheck >/dev/null || {
+          sudo apt-get update && sudo apt-get install -y shellcheck
+        }
+        shellcheck --version
+
+    - name: Run ShellCheck for changed scripts
+      run: |
+        perl tools/shellcheck-ci.pl HEAD^1

.shellcheckrc

@@ -0,0 +1,15 @@
+# ShellCheck (https://www.shellcheck.net/) settings for NUT scripts.
+# Found by shellcheck in the directory of a checked script or its parents.
+#
+# NUT scripts deliberately stay portable to old Bourne-style /bin/sh
+# implementations (e.g. on Solaris/illumos, AIX, HP-UX), so do not
+# suggest modern replacements for these idioms:
+
+# Use $(...) notation instead of legacy backticks `...`
+disable=SC2006
+
+# Avoid x-prefix in comparisons as it no longer serves a purpose
+disable=SC2268
+
+# expr is antiquated, consider rewriting this using $((..)), ${} or [[ ]]
+disable=SC2003

Makefile.am

@@ -282,7 +282,7 @@ EXTRA_DIST += ci_build.sh ci_build.adoc \
 EXTRA_DIST += autogen.sh
 
 # Help also the tarball users contribute properly:
-EXTRA_DIST += .editorconfig
+EXTRA_DIST += .editorconfig .shellcheckrc
 
 # automake xompilation wrapper
 EXTRA_DIST += compile
@@ -844,8 +844,48 @@ $(SPELLCHECK_DIRS_LAST): prep-src-docs/docs/man prep-src-docs/docs $(SPELLCHECK_
 # but using the correct make target for this goal:
 # FIXME: fanned-out recipes tend to fail early despite "make -ks", so for
 #  now we retry with a not-fanned-out attempt to cover most touch-files
+# In GitHub Actions, the docs recipes collect workflow commands (for error
+# annotations) into SPELLCHECK_GHA_LOG, reported once at the end, because
+# the same errors may be reported again by the retry and there is a limit
+# on the count of annotations per step. They are also listed in the job
+# summary (GITHUB_STEP_SUMMARY), if available, with links to the lines.
+# Note: no hash characters in the awk program (octal escape is used).
+SPELLCHECK_GHA_SUMMARY_AWK = { \
+		s = $$0 ; sub(/^::error file=/, "", s) ; \
+		f = s ; sub(/,line=.*$$/, "", f) ; \
+		l = s ; sub(/^.*,line=/, "", l) ; sub(/,.*$$/, "", l) ; \
+		c = s ; sub(/^.*,col=/, "", c) ; sub(/,.*$$/, "", c) ; \
+		m = s ; sub(/^[^:]*::/, "", m) ; sub(/; fix it.*$$/, "", m) ; \
+		gsub(/[|]/, "\\|", m) ; \
+		loc = f ":" l ":" c ; \
+		if (ENVIRON["GITHUB_SERVER_URL"] != "" && ENVIRON["GITHUB_REPOSITORY"] != "" && ENVIRON["GITHUB_SHA"] != "") { \
+			loc = "[" loc "](" ENVIRON["GITHUB_SERVER_URL"] "/" ENVIRON["GITHUB_REPOSITORY"] "/blob/" ENVIRON["GITHUB_SHA"] "/" f "\043L" l ")" ; \
+		} \
+		print "| " loc " | " m " |" ; \
+	}
+
 spellcheck spellcheck-interactive: @dotMAKE@
 	+@SUBDIR_TGT_MAKEFLAGS="$${SUBDIR_TGT_MAKEFLAGS-} -k -s " ; export SUBDIR_TGT_MAKEFLAGS ; \
+	  if [ x"$${GITHUB_ACTIONS-}" = xtrue ] ; then \
+		SPELLCHECK_GHA_LOG="$(abs_top_builddir)/gha-annotations-spellchecked" ; \
+		rm -f "$${SPELLCHECK_GHA_LOG}" ; \
+		export SPELLCHECK_GHA_LOG ; \
+	  fi ; \
+	  report_gha() { \
+		if [ -n "$${SPELLCHECK_GHA_LOG-}" ] && [ -s "$${SPELLCHECK_GHA_LOG}" ] ; then \
+			sort -u "$${SPELLCHECK_GHA_LOG}" ; \
+			if [ -n "$${GITHUB_STEP_SUMMARY-}" ] ; then \
+				{ echo '### Spellcheck: unknown words' ; echo '' ; \
+				  echo '| Location | Problem |' ; echo '|---|---|' ; \
+				  sort -u "$${SPELLCHECK_GHA_LOG}" | $(AWK) '$(SPELLCHECK_GHA_SUMMARY_AWK)' ; \
+				  echo '' ; \
+				  echo 'Fix the spelling, or add valid words to `docs/nut.dict` (e.g. with `make spellcheck-interactive`, or edit it and run `make spellcheck-sortdict`).' ; \
+				  echo '' ; \
+				} >> "$${GITHUB_STEP_SUMMARY}" ; \
+			fi ; \
+		fi ; \
+		if [ -n "$${SPELLCHECK_GHA_LOG-}" ] ; then rm -f "$${SPELLCHECK_GHA_LOG}" ; fi ; \
+	  } ; \
 	  if [ x"$(NUT_MAKE_SKIP_FANOUT)" = xtrue ] ; then \
 		RES=0 ; \
 		if [ x"$(SUBDIR_MAKE_VERBOSE)" != x0 ] ; then \
@@ -861,6 +901,7 @@ spellcheck spellcheck-interactive: @dotMAKE@
 			D="`echo \"$$D\" | sed 's,^spellcheck/,,'`" ; \
 			(cd "$(builddir)/$$D" && $(MAKE) $(AM_MAKEFLAGS) SPELLCHECK_REPORT_MAYBE_UPDATED_DICT=yes -k -s $@) || RES=$$? ; \
 		done ; \
+		report_gha ; \
 		exit $$RES ; \
 	  fi ; \
 	  SUBDIR_MAKE_VERBOSE="$(SUBDIR_MAKE_VERBOSE)" ; \
@@ -871,7 +912,8 @@ spellcheck spellcheck-interactive: @dotMAKE@
 	  ( $(MAKE) $(AM_MAKEFLAGS) SPELLCHECK_TGT='$@' SUBDIR_MAKE_VERBOSE="$${SUBDIR_MAKE_VERBOSE}" -k -s $(SPELLCHECK_DIRS) && exit ; \
 	    echo "WARNING: FAILED fanned-out attempt in $@, retrying with NUT_MAKE_SKIP_FANOUT" >&2 ; \
 	    $(MAKE) $(AM_MAKEFLAGS) NUT_MAKE_SKIP_FANOUT=true SPELLCHECK_TGT='$@' -k -s $(SPELLCHECK_DIRS) \
-	  ) || exit ; \
+	  ) || { RES=$$? ; report_gha ; exit $$RES ; } ; \
+	  report_gha ; \
 	  if [ x'$@' = xspellcheck-interactive ] ; then \
 		echo "SUCCESS: $@: follow up with spellcheck-quick to revise and update timestamps"; \
 		$(MAKE) $(AM_MAKEFLAGS) spellcheck-quick ; \
@@ -1063,15 +1105,27 @@ dist-hook:
 ### Still, there remains value in also checking the script syntax
 ### by the very version of the shell interpreter that would run
 ### these scripts in production usage of the resulting packages.
+### POSIX shell scripts are checked with each interpreter listed in the
+### SHELL_PROGS envvar (system /bin/sh by default), like the NDE self-test
+### below does; Bourne-Again shell scripts are always checked with bash.
 ###
 check-scripts-syntax:
-	@echo 'NOTE: modern bash complains about scripts using backticks (warning not error), which we ignore in NUT codebase for portability reasons: `...` obsolete, use $$(...)'
+	@echo 'NOTE: some shells (e.g. ksh93) complain about scripts using backticks (warning not error), which we ignore in NUT codebase for portability reasons, so such messages are hidden: `...` obsolete, use $$(...)'
 	@RUNBASH=bash; if [ -x /bin/bash ] && /bin/bash -c 'echo $${BASH_VERSION}' | $(EGREP) '^[456789]\.' ; then RUNBASH=/bin/bash ; else if [ -x /usr/bin/env ] ; then RUNBASH="/usr/bin/env bash"; fi; fi ; \
+	 [ -n "$${SHELL_PROGS-}" ] || SHELL_PROGS="/bin/sh" ; \
+	 echo "Checking POSIX shell scripts with SHELL_PROGS='$$SHELL_PROGS', and Bourne-Again shell scripts with '$$RUNBASH'" ; \
+	 check_syntax() { \
+	    echo "+ $$1 -n $$F" ; \
+	    OUT="`$$1 -n \"$$F\" 2>&1`" ; RES=$$? ; \
+	    if [ -n "$$OUT" ] ; then printf '%s\n' "$$OUT" | $(EGREP) -v '`[.][.][.]` obsolete, use [$$][(][.][.][.][)]' >&2 ; fi ; \
+	    if [ "$$RES" != 0 ] ; then echo "ERROR: Syntax check failed for script file: $$F (with $$1)" >&2 ; return $$RES ; fi ; \
+	 } ; \
 	 for F in `git ls-files || find . -type f` ; do \
 	    case "`file \"$$F\"`" in \
-	        *"Bourne-Again shell script"*) ( set -x ; $$RUNBASH -n "$$F" ; ) ;; \
-	        *"POSIX shell script"*|*"shell script"*) ( set -x ; /bin/sh -n "$$F" ; ) ;; \
-	    esac || { RES=$$? ; echo "ERROR: Syntax check failed for script file: $$F" >&2 ; exit $$RES ; } ; \
+	        *"Bourne-Again shell script"*) check_syntax "$$RUNBASH" || exit ;; \
+	        *"POSIX shell script"*|*"shell script"*) \
+	            for SH in $$SHELL_PROGS ; do check_syntax "$$SH" || exit ; done ;; \
+	    esac ; \
 	done
 	@echo 'SUCCESS: Shell scripts syntax is acceptable, no fatal issues were found'
 

NEWS.adoc

@@ -997,6 +997,29 @@ https://github.com/networkupstools/nut/milestone/13
       with both a current and an old (7.1, as on CentOS 7) version of the
       `aspell-en` dictionary. Local builds keep the default of
       `configure --enable-spellcheck=auto`. [PR #3752]
+    * The `make shellcheck` (and `make check-scripts-syntax`) recipe can now
+      check the POSIX shell scripts with each interpreter listed in the
+      `SHELL_PROGS` environment variable (default is still the system
+      `/bin/sh`), and hides warnings about the legacy command substitution
+      syntax (e.g. from `ksh93`) to avoid noise that obscured other problems.
+      [issues #1058, #1059]
+    * Added a "GHA-07: ShellCheck" GitHub Actions workflow which checks the
+      shell scripts changed by a pull request, and fails only for findings
+      of "warning" or "error" level that are new compared to the base branch
+      (many existing scripts have known findings), with annotations on the
+      changed lines and a list in the job summary. The same check can be run
+      locally with `perl tools/shellcheck-ci.pl`. A new `.shellcheckrc` file
+      disables the checks for deliberate portability idioms of NUT scripts
+      (backticks, `x"$var"` comparisons, `expr`).
+    * The `make spellcheck` recipes now report unknown words with the file
+      name, line and column where they were found (rather than byte offsets
+      in the `aspell` output), and in GitHub Actions also as annotations on
+      the source lines (once per unknown word, despite the retries) and in
+      the job summary.
+    * The `configure` script now also looks for `aspell` filter resources
+      under `lib64` (e.g. on CentOS 7) and in directories named by just the
+      major and minor version (e.g. `aspell-0.60` for `aspell` 0.60.6.1).
+      [issue #2068]
 
 
 Release notes for NUT 2.8.5 - what's new since 2.8.4

ci_build.sh

@@ -2332,13 +2332,19 @@ default|default-alldrv|default-alldrv:no-distcheck|default-all-errors|default-al
             # Note: no PARMAKE_FLAGS here - better have this output readably
             # ordered in case of issues (in sequential replay below).
             ( echo "`date`: Starting the quiet build attempt for target $BUILD_TYPE..." >&2
+              # Its output is discarded, so let the verbose attempt below
+              # make the GitHub Actions annotations and job summary
+              GITHUB_ACTIONS="" ; export GITHUB_ACTIONS
               $CI_TIME $MAKE $MAKE_FLAGS_QUIET SPELLCHECK_ERROR_FATAL=yes -k $PARMAKE_FLAGS "`echo \"$BUILD_TYPE\" | sed 's,^default-,,'`" >/dev/null 2>&1 \
               && echo "`date`: SUCCEEDED the spellcheck" >&2
             ) || \
             ( echo "`date`: FAILED something in spellcheck above; re-starting a verbose build attempt to give more context first:" >&2
               $CI_TIME $MAKE $MAKE_FLAGS_VERBOSE SPELLCHECK_ERROR_FATAL=yes spellcheck
               # Make end of log useful:
               echo "`date`: FAILED something in spellcheck above; re-starting a non-verbose build attempt to just summarize now:" >&2
+              # The verbose attempt above already made the GitHub Actions
+              # annotations and job summary for the errors, do not repeat
+              GITHUB_ACTIONS="" ; export GITHUB_ACTIONS
               $CI_TIME $MAKE $MAKE_FLAGS_QUIET SPELLCHECK_ERROR_FATAL=yes spellcheck
             )
             exit $?

docs/Makefile.am

@@ -885,6 +885,39 @@ ASPELL_ENV_LC_ALL = C
 ### ASPELL_ENV_LC_ALL = en_US.UTF-8
 ASPELL_OUT_NOTERRORS = (^[ \t]*[\*\@]|^$$)
 
+# Locate the errors in "aspell -a" output: it prints a banner line, and then
+# for each input line its result lines (with a column offset for unknown
+# words, which matches the original line since we prefix each line with a
+# space), followed by an empty line. Reports the errors as "file:line:col:"
+# prefixed aspell result lines, or as GitHub Actions workflow commands (for
+# annotations on the changed lines) with `-v mode=gha`; the top-level
+# spellcheck recipe collects these into SPELLCHECK_GHA_LOG, if set.
+# No annotations are made with SPELLCHECK_ERROR_FATAL=no (e.g. in distcheck
+# when the spelling is checked by another CI step). Note: no ternary
+# operators nor hash characters here, for older awk and make implementations.
+ASPELL_OUT_LOCATE_AWK = BEGIN { line = 1 } \
+	/^@/ { next } \
+	/^$$/ { line++ ; next } \
+	/^[*+-]/ { next } \
+	{ \
+		word = $$2 ; col = $$3 ; sugg = "" ; \
+		if ($$1 == "&" || $$1 == "?") { \
+			col = $$4 ; sub(/:$$/, "", col) ; \
+			sugg = $$0 ; sub(/^[^:]*: /, "", sugg) ; \
+		} \
+		if (mode == "gha") { \
+			n = split(sugg, s, ", ") ; hint = "" ; \
+			for (i = 1; i <= n && i <= 5; i++) { \
+				if (i > 1) { hint = hint ", " } \
+				hint = hint s[i] ; \
+			} \
+			if (hint != "") { hint = " (suggestions: " hint ")" } \
+			printf "::error file=%s,line=%d,col=%d,title=Spellcheck::Unknown word \"%s\"%s; fix it, or add it to docs/nut.dict if valid\n", src, line, col, word, hint ; \
+		} else { \
+			print src ":" line ":" col ": " $$0 ; \
+		} \
+	}
+
 # WARNING: The percent wildcard is a GNU extension; otherwise we need
 # a ".txt.txt-spellchecked" type of rule and files like "README" all
 # renamed to *.txt, or lots of rules for files without the extensions.
@@ -962,13 +995,24 @@ $(SPELLCHECK_BUILDDIR)/$(SPELLCHECK_SRC_ONE)-spellchecked: $(SPELLCHECK_SRCDIR)/
 		echo "  ASPELL	Spell checking on $${REPORT_PREFIX}$${REPORT_SRCDIR}$${REPORT_SRC_ONE}"; \
 	 fi ; \
 	 OUT="`(sed 's,^\(.*\)$$, \1,' | $(ASPELL) -a $(ASPELL_NUT_TEXMODE_ARGS) $(ASPELL_NUT_COMMON_ARGS) 2>&1) < '$(SPELLCHECK_SRCDIR)/$(SPELLCHECK_SRC_ONE)'`" \
-		&& { if test -n "$$OUT" ; then OUT="`echo \"$$OUT\" | $(EGREP) -b -v '$(ASPELL_OUT_NOTERRORS)' `" ; fi; \
-		     test -z "$$OUT" ; } \
+		&& { ERRS="" ; if test -n "$$OUT" ; then \
+		        printf '%s\n' "$$OUT" | $(AWK) -v src="$${REPORT_SRCDIR}$${REPORT_SRC_ONE}" '$(ASPELL_OUT_LOCATE_AWK)' > '$@.errs' ; \
+		        ERRS="`cat '$@.errs'`" ; \
+		        rm -f '$@.errs' ; \
+		     fi; \
+		     test -z "$$ERRS" ; } \
 		|| { RES=$$? ; \
-		     echo "FAILED : Aspell reported errors here:" >&2 \
-		     && echo "----- vvv" >&2 \
-		     && echo "$$OUT" >&2 \
-		     && echo "----- ^^^" >&2 ; \
+		     echo "FAILED : Aspell reported errors here:" >&2 ; \
+		     echo "----- vvv" >&2 ; \
+		     if test -n "$$ERRS" ; then echo "$$ERRS" >&2 ; else echo "$$OUT" >&2 ; fi ; \
+		     echo "----- ^^^" >&2 ; \
+		     if test x"$${GITHUB_ACTIONS-}" = xtrue && test x"$(SPELLCHECK_ERROR_FATAL)" != xno && test -n "$$ERRS" ; then \
+		        if test -n "$${SPELLCHECK_GHA_LOG-}" ; then \
+		           printf '%s\n' "$$OUT" | $(AWK) -v src="$${REPORT_SRCDIR}$${REPORT_SRC_ONE}" -v mode=gha '$(ASPELL_OUT_LOCATE_AWK)' >> "$${SPELLCHECK_GHA_LOG}" ; \
+		        else \
+		           printf '%s\n' "$$OUT" | $(AWK) -v src="$${REPORT_SRCDIR}$${REPORT_SRC_ONE}" -v mode=gha '$(ASPELL_OUT_LOCATE_AWK)' ; \
+		        fi ; \
+		     fi ; \
 		     exit $$RES; } ; \
 	 if $(EGREP) '^[ *.-]*(NOTE|TIP|IMPORTANT|CAUTION|WARNING):[ \t]*[a-z]' '$(SPELLCHECK_SRCDIR)/$(SPELLCHECK_SRC_ONE)' ; then \
 		echo "FAILED: ASCIIDOC nuance: Admonition block should contain a sentence, not starting with a lower-case letter: '$${REPORT_PREFIX}$${REPORT_SRCDIR}$${REPORT_SRC_ONE}'" >&2 ; \

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3900 utf-8
+personal_ws-1.1 en 3904 utf-8
 AAC
 AAS
 ABI
@@ -1346,6 +1346,7 @@ Shambarger
 Shara
 ShareAlike
 Shaul
+ShellCheck
 ShutDown
 Sibbald
 Sicon
@@ -1798,6 +1799,7 @@ backgrounding
 backport
 backported
 backports
+backticks
 backupspro
 bart
 baseurl
@@ -2235,6 +2237,7 @@ executables
 executeCommand
 execve
 expandsize
+expr
 extapi
 extern
 externalConsole
@@ -3352,6 +3355,7 @@ sgml
 sgs
 sha
 shellcheck
+shellcheckrc
 shellenv
 shm
 showinternals

docs/qa-guide.adoc

@@ -199,15 +199,34 @@ Shell script checks
 The `make shellcheck` recipe finds files which the `file` tool determines
 to be POSIX or Bourne-Again shell scripts, and runs them through respective
 interpreter's (`bash` or system `/bin/sh`) test mode to validate the syntax
-works.
+works. POSIX shell scripts can be checked with other interpreters instead
+of `/bin/sh` by listing them in the `SHELL_PROGS` environment variable,
+e.g. `make shellcheck SHELL_PROGS="dash ksh bash"`. Warnings that some
+shells (e.g. `ksh93`) emit about the legacy command substitution syntax
+(which NUT scripts use deliberately for portability) are hidden.
 
 Given that the `/bin/sh` implementation varies wildly on different systems
 (e.g. Korn shell, BASH, DASH and many others), this goal performed by CI on
 a large number of available platforms makes sure that the lowest-denominator
 syntax we use is actually understood everywhere.
 
-NOTE: At a later time additional tests, perhaps using the `shellcheck` tool,
-can be introduced into the stack.
+For pull requests, the "GHA-07: ShellCheck" GitHub Actions workflow also
+runs the link:https://www.shellcheck.net/[ShellCheck] tool on the shell
+scripts that they change. Many existing scripts have known findings, so it
+fails only for findings of "warning" or "error" level that are new compared
+to the base branch. The new findings are shown as annotations on the
+changed lines, and listed in the job summary. You can run the same check
+locally (with `perl` and `shellcheck` installed) before posting the
+changes, e.g.:
+
+----
+:; perl tools/shellcheck-ci.pl origin/master
+----
+
+The `.shellcheckrc` file in the root of NUT sources disables the checks for
+deliberate portability idioms of NUT scripts (e.g. the legacy command
+substitution syntax), and is also used by `shellcheck` runs in your editor
+or IDE.
 
 The `make shellcheck-nde` recipe calls `tests/nut-driver-enumerator-test.sh`
 to self-test the `scripts/upsdrvsvcctl/nut-driver-enumerator.sh.in` against
@@ -243,6 +262,13 @@ For each checked file, a `*-spellchecked` touch-file is created in respective
 `${builddir}`, so it is not re-checked until the source document, the custom
 dictionary, or the `Makefile` recipe is updated.
 
+Unknown words are reported with the file name, line and column where they
+were found (relative to the root of NUT sources), followed by the `aspell`
+report line with suggestions. When running in GitHub Actions (with the
+`GITHUB_ACTIONS=true` environment variable), they are also reported as
+annotations for the source lines, and listed in the job summary (unless
+`SPELLCHECK_ERROR_FATAL=no` is set).
+
 The ecosystem of `Makefile.am` files includes the following useful recipes:
 
 * `spellcheck`: passively check that all used words are in some dictionary