makefiles: Reject 2>&1 >/dev/null, and weed out remaining offenders
#16806
Conversation
chrysn
added
Area: build system
| @@ -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 $$?)) | |||
| ifneq (0,$(shell command -v iotlab-experiment -h > /dev/null 2>&1; echo $$?)) | |||
There was a problem hiding this comment.
| ifneq (0,$(shell command -v iotlab-experiment -h > /dev/null 2>&1; echo $$?)) | |
| ifneq (0,$(shell command -v iotlab-experiment -h > /dev/null; echo $$?)) |
why not leave stderr in place?
There was a problem hiding this comment.
Because the complexity of this is what keeps Make from being clever.
The whole line can be simplified to ifeq (,$(shell command -v iotlab-experiment)), but then this fails on every GNU Make < 4.3 (ie. every Ubuntu ever released) like what broke everything this morning.
There was a problem hiding this comment.
Put more generally, what this PR is about is just about forcing out and fixing the particular bug of using 2>&1 >/dev/null. There is a whole other issue about unifying the which and command calls in #16807, but I'd like to keep the focus here to fixing the sequence -- especially because eagerly fixing bycatch of shell command has been disastrous before.
There was a problem hiding this comment.
So this will behave different without the stderr redirect?
There was a problem hiding this comment.
It might; I'd need to dig up some of the old offending environments where make tries to forego the actual shell forking to evaluate it.
Once we're sure that no old make versions are around on users' systems any more (and we don't have a strict dependency there, but I'm holding off #16807 for some more time), we can reevaluate all these, but until then, I'm trying to minimally fix the errors around command.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
chrysn
removed
the
CI: ready for build
stale
bot
removed
the
State: stale
chrysn
added
the
CI: ready for build
chrysn
force-pushed
the
which-hunt-the-good-parts
branch
from
35b9d13 to
b64a16d
Compare
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: RIOT-OS#16775
This is a follow-up for [16775], and was not caught there as that search was limited to `which`. Note that while this line can be simplified, the redirects ensure that GNU Make < 4.3 will not optimize it into its own built-in shell that does not know `command`. [16775]: RIOT-OS#16775
chrysn
force-pushed
the
which-hunt-the-good-parts
branch
from
b64a16d to
25e11d9
Compare
|
Rebased to fix merge conflicts. |
Contribution description
Shell redirects in the form of
2>&1 >/dev/nulldo not, as one might think, redirect both stdout and err to /dev/null, but put stdout to /dev/null and redirect stderr to the process's standard output.This adds an automated test for such redirects, and removes two more redirects that were spotted with its help.
Testing procedure
2>&1 >/dev/nulltrips up the static checks.Issues/PRs references
This is a follow-up on #16775 with the unproblematic parts of #16776 (which had to be reverted; the problematic parts come up in a different PR with more caution this time).