From 6b50033766b7701b2e20bc8646085e5edf4a27c0 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 24 Aug 2021 15:11:20 +0200 Subject: [PATCH 1/4] makefiles: Replace `which` with `command -v` As the POSIX documentation[1] of `command -v` guarantees that on error there will be no output (and there will be output in the other cases), the tests in Makefiles were simplified to test for output equality to the empty string. Redirects swallowing error outputs were removed, as the command produces no error output there (as recommended at [2]). Existing uses of `command` are simplified as well. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html [2]: https://salsa.debian.org/debian/debianutils/-/blob/master/debian/NEWS --- .github/workflows/release-test.yml | 2 +- Makefile.include | 16 ++++++++-------- .../makefile.iotlab.single.inc.mk | 4 ++-- .../makefile.openvisualizer.inc.mk | 6 +++--- makefiles/arch/riscv.inc.mk | 2 +- makefiles/boards/sam0.inc.mk | 2 +- pkg/flatbuffers/Makefile.include | 2 +- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/.github/workflows/release-test.yml b/.github/workflows/release-test.yml index 8ed73bacb533..48fb27a8224e 100644 --- a/.github/workflows/release-test.yml +++ b/.github/workflows/release-test.yml @@ -150,7 +150,7 @@ jobs: TTN_DEV_ID="riot_lorawan_1" \ TTN_DEV_ID_ABP="riot_lorawan_1_abp" \ RIOTBASE=${RIOTBASE} \ - $(which tox) -e test -- ${TOX_ARGS} \ + $(command -v tox) -e test -- ${TOX_ARGS} \ ${K} "${{ github.event.inputs.filter }}" -m "${{ matrix.pytest_mark }}" - name: junit2html and XML deploy if: always() diff --git a/Makefile.include b/Makefile.include index 3ca75ca26818..56b545e7df34 100644 --- a/Makefile.include +++ b/Makefile.include @@ -327,13 +327,13 @@ APPLICATION := $(strip $(APPLICATION)) ifeq (,$(and $(DOWNLOAD_TO_STDOUT),$(DOWNLOAD_TO_FILE))) ifeq (,$(WGET)) - ifeq (0,$(shell which wget > /dev/null 2>&1 ; echo $$?)) - WGET = $(call memoized,WGET,$(shell which wget)) + ifeq (,$(shell command -v wget)) + WGET = $(call memoized,WGET,$(shell command -v wget)) endif endif ifeq (,$(CURL)) - ifeq (0,$(shell which curl > /dev/null 2>&1 ; echo $$?)) - CURL = $(call memoized,CURL,$(shell which curl)) + ifneq (,$(shell command -v curl)) + CURL = $(call memoized,CURL,$(shell command -v curl)) endif endif ifeq (,$(WGET)$(CURL)) @@ -349,11 +349,11 @@ ifeq (,$(and $(DOWNLOAD_TO_STDOUT),$(DOWNLOAD_TO_FILE))) endif ifeq (,$(UNZIP_HERE)) - ifeq (0,$(shell which unzip > /dev/null 2>&1 ; echo $$?)) - UNZIP_HERE = $(call memoized,UNZIP_HERE,$(shell which unzip) -q) + ifneq (,$(shell command -v unzip)) + UNZIP_HERE = $(call memoized,UNZIP_HERE,$(shell command -v unzip) -q) else - ifeq (0,$(shell which 7z > /dev/null 2>&1 ; echo $$?)) - UNZIP_HERE = $(call memoized,UNZIP_HERE,$(shell which 7z) x -bd) + ifneq (,$(shell command -v 7z)) + UNZIP_HERE = $(call memoized,UNZIP_HERE,$(shell command -v 7z) x -bd) else $(warning Neither unzip nor 7z is installed.) endif diff --git a/dist/testbed-support/makefile.iotlab.single.inc.mk b/dist/testbed-support/makefile.iotlab.single.inc.mk index b831b1873eb6..425f11e051cb 100644 --- a/dist/testbed-support/makefile.iotlab.single.inc.mk +++ b/dist/testbed-support/makefile.iotlab.single.inc.mk @@ -57,7 +57,7 @@ endif IOTLAB_AUTH ?= $(HOME)/.iotlabrc IOTLAB_USER ?= $(shell cut -f1 -d: $(IOTLAB_AUTH)) -ifneq (0,$(shell command -v iotlab-experiment -h 2>&1 > /dev/null ; echo $$?)) +ifeq (,$(shell command -v iotlab-experiment)) $(info $(COLOR_RED)'iotlab-experiment' command is not available \ please consider installing it from \ https://pypi.python.org/pypi/iotlabcli$(COLOR_RESET)) @@ -66,7 +66,7 @@ endif ifeq (iotlab-a8-m3,$(BOARD)) ifneq (,$(filter flash% reset,$(MAKECMDGOALS))) - ifneq (0,$(shell command -v iotlab-ssh -h 2>&1 > /dev/null ; echo $$?)) + ifeq (,$(shell command -v iotlab-ssh)) $(info $(COLOR_RED)'iotlab-ssh' command is not available \ please consider installing it from \ https://pypi.python.org/pypi/iotlabsshcli$(COLOR_RESET)) diff --git a/dist/tools/openvisualizer/makefile.openvisualizer.inc.mk b/dist/tools/openvisualizer/makefile.openvisualizer.inc.mk index 3f1623ecf50a..3d48623693d5 100644 --- a/dist/tools/openvisualizer/makefile.openvisualizer.inc.mk +++ b/dist/tools/openvisualizer/makefile.openvisualizer.inc.mk @@ -34,9 +34,9 @@ # # Use full path in case it needs to be run with sudo -OPENV_SERVER_PATH := $(shell which openv-server) -OPENV_CLIENT_PATH := $(shell which openv-client) -OPENV_SERIAL_PATH := $(shell which openv-serial) +OPENV_SERVER_PATH := $(shell command -v openv-server) +OPENV_CLIENT_PATH := $(shell command -v openv-client) +OPENV_SERIAL_PATH := $(shell command -v openv-serial) # Openvisualizer requires to know where openwsn-fw is located OPENV_OPENWSN_FW_PATH ?= --fw-path=$(RIOTBASE)/build/pkg/openwsn diff --git a/makefiles/arch/riscv.inc.mk b/makefiles/arch/riscv.inc.mk index 0be25e230128..f563fd1b2f5b 100644 --- a/makefiles/arch/riscv.inc.mk +++ b/makefiles/arch/riscv.inc.mk @@ -25,7 +25,7 @@ TARGET_ARCH_RISCV ?= \ $(subst -gcc,,\ $(notdir \ $(word 1,\ - $(foreach triple,$(_TRIPLES_TO_TEST),$(shell which $(triple)-gcc 2> /dev/null)))))) + $(foreach triple,$(_TRIPLES_TO_TEST),$(shell command -v $(triple)-gcc)))))) TARGET_ARCH ?= $(TARGET_ARCH_RISCV) diff --git a/makefiles/boards/sam0.inc.mk b/makefiles/boards/sam0.inc.mk index 04d0004486a2..c533ce1c1c8b 100644 --- a/makefiles/boards/sam0.inc.mk +++ b/makefiles/boards/sam0.inc.mk @@ -29,7 +29,7 @@ ifeq ($(PROGRAMMER),) PROGRAMMER ?= edbg else ifeq ($(OPENOCD_DEBUG_ADAPTER),jlink) # only use JLinkExe if it's installed - ifneq (,$(shell which JLinkExe)) + ifneq (,$(shell command -v JLinkExe)) PROGRAMMER ?= jlink else PROGRAMMER ?= openocd diff --git a/pkg/flatbuffers/Makefile.include b/pkg/flatbuffers/Makefile.include index 1a399bee7646..a81a61719fac 100644 --- a/pkg/flatbuffers/Makefile.include +++ b/pkg/flatbuffers/Makefile.include @@ -2,7 +2,7 @@ INCLUDES += -I$(PKGDIRBASE)/flatbuffers/include FLATC ?= flatc -ifneq (0,$(shell which flatc > /dev/null 2>&1 ; echo $$?)) +ifeq (,$(shell command -v flatc )) FLATC = $(RIOTTOOLS)/flatc/flatc $(call target-export-variables,all,FLATC) endif From 6bcc68b9ccf510f29fd0a1304e09adb969c16a6b Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 24 Aug 2021 15:22:42 +0200 Subject: [PATCH 2/4] makefiles: Reject `shell which` in new code ... as that command is deprecated at least on Debian, and a good replacement is available in the form of `command -v`. --- dist/tools/buildsystem_sanity_check/check.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/dist/tools/buildsystem_sanity_check/check.sh b/dist/tools/buildsystem_sanity_check/check.sh index 99fb11f78629..a7f8100ff049 100755 --- a/dist/tools/buildsystem_sanity_check/check.sh +++ b/dist/tools/buildsystem_sanity_check/check.sh @@ -334,6 +334,19 @@ check_no_pkg_source_local() { | error_with_message "Don't push PKG_SOURCE_LOCAL definitions upstream" } +check_shell_which() { + local patterns=() + local pathspec=() + + patterns+=(-e '(shell[[:blank:]]\+which') + + pathspec+=('Makefile*') + pathspec+=('**/Makefile*') + pathspec+=('**/*.mk') + git -C "${RIOTBASE}" grep -n "${patterns[@]}" -- "${pathspec[@]}" \ + | error_with_message "Don't use \`which\` in makefiles, use \`command -v\` instead." +} + error_on_input() { ! grep '' } @@ -353,6 +366,7 @@ all_checks() { check_no_pseudomodules_in_makefile_dep check_no_usemodules_in_makefile_include check_no_pkg_source_local + check_shell_which } main() { From 1c6b675d9f7687bb78cc79b72d8673b195de5d06 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 24 Aug 2021 17:50:11 +0200 Subject: [PATCH 3/4] makefiles: Reject `2>&1 > /dev/null` While this could theoretically be desired, it's usually just a mishap. It is unlikely that legitimate cases will be needed in the build system; if so, they can exclude themselves. See-Also: https://github.com/RIOT-OS/RIOT/pull/16775 --- dist/tools/buildsystem_sanity_check/check.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/dist/tools/buildsystem_sanity_check/check.sh b/dist/tools/buildsystem_sanity_check/check.sh index a7f8100ff049..5b00c61c4091 100755 --- a/dist/tools/buildsystem_sanity_check/check.sh +++ b/dist/tools/buildsystem_sanity_check/check.sh @@ -347,6 +347,19 @@ check_shell_which() { | error_with_message "Don't use \`which\` in makefiles, use \`command -v\` instead." } +check_stderr_null() { + local patterns=() + local pathspec=() + + patterns+=(-e '2>[[:blank:]]*&1[[:blank:]]*>[[:blank:]]*/dev/null') + + pathspec+=('Makefile*') + pathspec+=('**/Makefile*') + pathspec+=('**/*.mk') + git -C "${RIOTBASE}" grep -n "${patterns[@]}" -- "${pathspec[@]}" \ + | error_with_message "Redirecting stderr and stdout to /dev/null is \`>/dev/null 2>&1\`; the other way round puts the old stderr to the new stdout." +} + error_on_input() { ! grep '' } @@ -367,6 +380,7 @@ all_checks() { check_no_usemodules_in_makefile_include check_no_pkg_source_local check_shell_which + check_stderr_null } main() { From 8664eb8ffcfac544ea2aa5aa0e7259ba8239c952 Mon Sep 17 00:00:00 2001 From: chrysn Date: Fri, 27 Aug 2021 19:06:44 +0200 Subject: [PATCH 4/4] github workflows: Stick with `which` instead of `command -v` ... because even though it looks like a subshell, it apparently is not evaluated through a shell but passed right into exec. --- .github/workflows/release-test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release-test.yml b/.github/workflows/release-test.yml index 48fb27a8224e..8ed73bacb533 100644 --- a/.github/workflows/release-test.yml +++ b/.github/workflows/release-test.yml @@ -150,7 +150,7 @@ jobs: TTN_DEV_ID="riot_lorawan_1" \ TTN_DEV_ID_ABP="riot_lorawan_1_abp" \ RIOTBASE=${RIOTBASE} \ - $(command -v tox) -e test -- ${TOX_ARGS} \ + $(which tox) -e test -- ${TOX_ARGS} \ ${K} "${{ github.event.inputs.filter }}" -m "${{ matrix.pytest_mark }}" - name: junit2html and XML deploy if: always()