Remove `which` from shell invocations by chrysn · Pull Request #16776 · RIOT-OS/RIOT

chrysn · 2021-08-24T13:43:17Z

Contribution description

The which command is being deprecated in Debian (see references below).

This moves all invocations in the build system from which to the successor command -v.

Testing procedure

Try building anything on a recent Debian sid system. While it'll fail on master, after #16775 you'll get several lines of

/usr/bin/which: this version of `which' is deprecated; use `command -v' in scripts instead.

that are gone after this patch sequence.

Any attempt to reintroduce $(shell which ...) in the Makefiles should also trigger the sanity checks of dist/tools/buildsystem_sanity_check/check.sh.

Issues/PRs references

This is a follow-up of (and based on) #16775. It was split out after some build process spew errors for not knowing command. As #16775 is blocking my workflows and I'd have to keep it around on each of my branches, I'd like to make that mergable quickly, and keep this for discussions of whether all of our users are now on platforms where command is available. (It has already been in use in some places in the build system, but these seem not to have been covered in that particular build platform).

debianutils' which, in its changelog, states:

  * The 'which' utility will be removed in the future.  Shell scripts
    often use it to check whether a command is available.  A more
    standard way to do this is with 'command -v'; for example:
      if command -v update-icon-caches >/dev/null; then
        update-icon-caches /usr/share/icons/...
      fi
    '2>/dev/null' is unnecessary when using 'command': POSIX says "no
    output shall be written" if the command isn't found.  It's also
    unnecessary for the debianutils version of 'which', and hides the
    deprecation warning.

Note that this PR does not change which to command, it just fixes the fallout of ill-sequenced redirects. That change is probably good to do, but right now this should be minimal enough to get merged quickly, and I don't want it held up in the question of whether everyone and their dog already implement POSIX command.

chrysn · 2021-08-24T13:57:28Z

Checking Ubuntu versions, command -v has been supported in /bin/sh since at least 10.04 (stopped 10 years ago); playing around a bit... but would appreciate any insights.

miri64 · 2021-08-24T14:24:13Z

dist/tools/buildsystem_sanity_check/check.sh

+
+    pathspec+=('**/Makefile*')
+    git -C "${RIOTBASE}" grep -n "${patterns[@]}" -- "${pathspec[@]}" \
+        | error_with_message "Don't push PKG_SOURCE_LOCAL definitions upstream"


Wrong error message ;-)

In-very-deed. Thanks, fixed. (And tests generally improved, and one for the redirects of #16775 added for good measure.)

miri64 · 2021-08-24T16:16:30Z

dist/tools/buildsystem_sanity_check/check.sh

+    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."


I am not sure this can be generally assumed. I myself, actually, sometimes use 2>&1 > /dev/null where one would use &> /dev/null in bash (which is a bashism that does not work in every shell, such as dash)

2>&1 >/dev/null sends the old stderr to the original stdout.

>/dev/null 2>&1 does as &>/dev/null and sends both to /dev/null.

This is what caused the errors in #16776 when output to stderr was suddenly misdirected into a stdout where it was to be compared to echo $?'s "0" output.

Ah, gotcha!

fjmolinas · 2021-08-27T08:42:44Z

please rebase @chrysn

chrysn · 2021-08-27T09:09:18Z

The Python error looks like there's a stray redirect somewhere, investigating...

miri64 · 2021-08-27T22:11:33Z

The Python error looks like there's a stray redirect somewhere, investigating...

I think https://github.com/RIOT-OS/RIOT/pull/16776/checks?check_run_id=3445443625#step:7:47 tells the truth.

RIOT/.github/workflows/tools-test.yml

Line 27 in 719a18c

sudo apt-get install gcc-multilib

curl or wget should be added here (though I am not sure why it is needed for building hello-world 😅)

miri64 · 2021-08-27T22:12:59Z

(though I am not sure why it is needed for building hello-world 😅)

I deem this out of scope of this PR though ;-).

chrysn · 2021-08-27T22:42:57Z

curl and wget not being found are the typical symptoms of a failing which (or command). PATH won't cut it as command is not an executable but a shell built-in required by POSIX to be implemented by any sh. Will look further tomorrow.

miri64 · 2021-08-28T09:35:13Z

PATH won't cut it as command is not an executable but a shell built-in required by POSIX to be implemented by any sh. Will look further tomorrow.

Yeah, I noticed that as well, that's why I removed the comment ;-)

fjmolinas · 2021-08-28T09:43:09Z

(though I am not sure why it is needed for building hello-world sweat_smile)

I deem this out of scope of this PR though ;-).

taken care in #16784, lets see if no unexpected things blow up.

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

... as that command is deprecated at least on Debian, and a good replacement is available in the form of `command -v`.

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

... because even though it looks like a subshell, it apparently is not evaluated through a shell but passed right into exec.

chrysn · 2021-09-02T16:25:22Z

For reasons I can't fathom (might be #16784, but I don't see how), the spurious errors in the Python checks are now gone.

Good to go?

fjmolinas

ACK!

miri64 · 2021-09-03T08:43:22Z

For reasons I can't fathom (might be #16784, but I don't see how), the spurious errors in the Python checks are now gone.

That's how: #16776 (comment) wget/curl is now not a dependency anymore so the make flash called within that python script, now works without problems :-).

miri64 · 2021-09-03T09:26:12Z

Something went wrong :-/ #16020

miri64 · 2021-09-03T09:26:53Z

Why were compile tests skipped on a build system change?

chrysn · 2021-09-03T09:28:05Z

Revert WIP, explaining later.

miri64 · 2021-09-03T09:29:48Z

Alternatively, there is the hard way here #16803

…uests/shell-make-fixes-2" This reverts commit d4ed42a, reversing changes made to 2bf1202. Additions to the checks are left in place, but disabled while the current state would not pass them.

chrysn · 2021-09-03T09:31:58Z

#16804 is up now to fix the immediate fallout.

github-actions bot added Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: pkg Area: External package ports Area: tools Area: Supplementary tools Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Aug 24, 2021

miri64 reviewed Aug 24, 2021

View reviewed changes

github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: pkg Area: External package ports Area: tools Area: Supplementary tools Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Aug 24, 2021

miri64 reviewed Aug 24, 2021

View reviewed changes

chrysn force-pushed the shell-make-fixes-2 branch from c8dcc28 to a91d870 Compare August 27, 2021 08:45

github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Aug 27, 2021

fjmolinas mentioned this pull request Aug 28, 2021

Makefile.include: only warn if not curl, wget, unzip, 7z #16784

Merged

chrysn added 2 commits September 2, 2021 16:30

makefiles: Reject shell which in new code

6bcc68b

... as that command is deprecated at least on Debian, and a good replacement is available in the form of `command -v`.

chrysn added 2 commits September 2, 2021 16:30

makefiles: Reject 2>&1 > /dev/null

1c6b675

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

github workflows: Stick with which instead of command -v

8664eb8

... because even though it looks like a subshell, it apparently is not evaluated through a shell but passed right into exec.

chrysn force-pushed the shell-make-fixes-2 branch from cea69d1 to 8664eb8 Compare September 2, 2021 14:31

fjmolinas approved these changes Sep 3, 2021

View reviewed changes

chrysn merged commit d4ed42a into RIOT-OS:master Sep 3, 2021

chrysn deleted the shell-make-fixes-2 branch September 3, 2021 07:04

miri64 mentioned this pull request Sep 3, 2021

netdev_test_ieee802154: initial import of an IEEE 802.15.4 device mock #16020

Open

miri64 mentioned this pull request Sep 3, 2021

Revert "Remove which from shell invocations" #16803

Merged

This was referenced Sep 3, 2021

makefiles: Reject 2>&1 >/dev/null, and weed out remaining offenders #16806

Merged

Remove which from shell invocations (second attempt) #16807

Open

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

		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."

Conversation

chrysn commented Aug 24, 2021 • edited by kfessel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

chrysn commented Aug 24, 2021

Uh oh!

miri64 Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

chrysn Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

chrysn Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Aug 27, 2021

Uh oh!

chrysn commented Aug 27, 2021

Uh oh!

miri64 commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 27, 2021

Uh oh!

chrysn commented Aug 27, 2021

Uh oh!

miri64 commented Aug 28, 2021

Uh oh!

fjmolinas commented Aug 28, 2021

Uh oh!

chrysn commented Sep 2, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Sep 3, 2021

Uh oh!

miri64 commented Sep 3, 2021

Uh oh!

miri64 commented Sep 3, 2021

Uh oh!

chrysn commented Sep 3, 2021

Uh oh!

miri64 commented Sep 3, 2021

Uh oh!

chrysn commented Sep 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrysn commented Aug 24, 2021 •

edited by kfessel

Loading

miri64 commented Aug 27, 2021 •

edited

Loading