Revert "Remove which from shell invocations"

[miri64]

Reverts #16776

Builds are failing due to this PR having been merged.

[fjmolinas]

ACK, I still would like this, but lets fix builds and understand the issue later.

[miri64]

Should we try #16804 first?

[kfessel]

this should have been a neq

[miri64]

This is a revert PR. Not touching anything here.

[chrysn]

Good catch, will process in the followup second attempt.

[chrysn]

So what has happened (recapping also the undone while the tests are running, with nothing more to do since it's already ACKed):

1. The which command has been around since 1978 on BSDs (which(1)), but was never standardized.
2. GNU make, at some unknown point probably around here, decided that it can parse simple $(shell) invocations without fanning out to /bin/sh or the configured shell. This is not clearly documented.
3. The command command (try looking for that with your favorite search engine...) was added to POSIX 4 in the early 2000s (command documentation, history)
4. Some points in the RIOT build system relied on which not to produce stderr output -- not intentionally, but because there never was any, and so their redirects got untested (see patch for examples).
5. GNU make updates its list of known shell built-in functions in its 4.3 release in January 2020 ("src/job.c (sh_cmds): [SV 57625] Update builtin shell command list")
6. Debian maintainers decided to deprecate which in August 2021, recommending to use command instead (news file).
7. Builds suddenly started breaking for me, with "which is deprecated" warnings all over the place.
8. 2021-08-24: I prepare makefiles: Sort >/dev/null and 2>&1 #16775 to mitigate 4. alone. Consequently, builds work on my system again, albeit spewing deprecation warnings. This is merged 2021-08-27.
9. 2021-08-24: I prepare the larger changes of using command everywhere in Remove which from shell invocations #16776; tests include running command in bash and /bin/sh on Ubuntu versions down to Ubuntu Precise (2012's LTS), concluding that the command is sufficiently wide-spread. The change set includes checks against the redirect mixups of 4, and against using shell which in Makefiles.
10. 2021-08-24: I set "CI: ready for build" and "CI: skip compile test" because the execution of the build infrastructure was also tested in the static checks (indicated by them failing at first), and no actual compiled code would change. (Also, I was using the changes locally, and to some extent maybe the old habit of the "I have to set CI ready to let the static tests run" of old played into it)
11. Some (which precisely is lost to me) of the static checks kept failing with command: No such command. I tried to reproduce them, but to no avail.
12. 2021-09-02: Rebasing 16776 onto the latest master, the static check failures of the offending PR went away. While suspicious, I attribute this to other updates to the infrastructure and go ahead merging.
13. 2021-09-03: @benpicco (on Matrix) and @miri64 (on Github) notice build failures. Revert "Remove which from shell invocations" #16803 and Largely revert "Merge pull request #16776 from chrysn-pull-requests/shell-make-fixes-2" #16804 are created to fix the fallout by reverting; regular operation is restored.

So, takeaways and repetition mitigation:

• I owe you all a round of carbonated, possibly alcoholic beverages at the next physical meeting.
• which is here to stay (and spew warnings) for some time; not even Ubuntu Focal (which CI is not yet switched to) has a sufficiently recent Make to use command. Possible workarounds of forcing command through a shell are up for exploration, to avoid everyone suffering deprecation warnings all over the place for years. Possibly, Debian maintainers might reconsider their deprecation timeline in light of this.
• Better trigger discipline around "skip compile tests":
  • "Must be this tall to set 'skip compile tests'" rule? Could take the form of four-eyes, "someone else than the submitter" or just "maintainers who are not me".
  • An intermediate "reduce compile tests" mode? Could take the pressure of other button, or even obsolete it completely. In such a mode, a subset of builds could be run quite quickly (say, all examples on native and one example on two non-native boards). With regular builds taking 40min to 1h, and assuming good scaling, this could be done in a matter of minutes, just as quick as the static and doc tests.

[edit: addendum
In investigating the failures at 11., I had suspected something similar at 8664eb8 -- but misattributed it to github workflows instead of the underlying Makefiles, which did indeed behave as described in that commit.
]

[miri64]

* "[Must be this tall to set 'skip compile tests'](https://bholley.net/images/posts/thistall.jpg)" rule? Could take the form of four-eyes,  "someone else than the submitter" or just "maintainers who are not me".

My rule of thumb is: any change containing changes to C-files or the build system => fingers away from 'skip compile tests'.

[chrysn]

containing changes to C-files

Most of our documentation is built from C files, and doc-only updates (working in the several commented-out paragraphs at the start of the files) would be classical candidates. (A slim build could catch if, for example, a stray */ or trigraph ended the comment there in a way the syntax highlighter doesn't catch.)

[miri64]

>10 years of RIOT development made me too paranoid to trust some random static syntax checker, to do GCC's job properly :P

[kfessel]

A "CI:build just default target" would be nice especialy for documentation change in compiled files

[miri64]

A "CI:build just default target" would be nice especialy for documentation change in compiled files

Not sure what the semantics of that would be different from the current CI: reday for build. The default target is make all.

[kfessel]

maybe CI:skip non default targets