Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove which from shell invocations (second attempt) #16807

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove which from shell invocations (second attempt) #16807

wants to merge 3 commits into from

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Sep 3, 2021

Contribution description

This is #16776 re-done, with some parts on the faster track of #16806 (which this is based on).

It removes all invocations of the unstandardized which with the POSIX standard command -v, because Debian's which complains loudly when used.

Trouble

Thing is: GNU Make < 4.3 (all recent Ubuntus) thinks it's clever and falls apart completely with this. That's why this is a Draft PR, and more kept for contemplation than for immediate merging.

Testing procedure

None so far.

Don't switch off "skip build tests".

Issues/PRs references

This was done already in #16776 and reverted in #16803.

@chrysn chrysn added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: build system Area: Build system labels Sep 3, 2021
@github-actions 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 Sep 3, 2021
@chrysn chrysn removed Area: pkg Area: External package ports Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Sep 3, 2021
@chrysn
Copy link
Member Author

chrysn commented Sep 3, 2021

Relevant Debian issue: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=993582

@chrysn
Copy link
Member Author

chrysn commented Sep 19, 2021

Putting this on hold per flag for probably a long time (probably until Ubuntu 22.04 LTS, which doesn't even have a name yet, is used in CI).

@chrysn chrysn added the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Sep 19, 2021
@chrysn chrysn mentioned this pull request Sep 19, 2021
Merged
@stale
Copy link

stale bot commented Apr 16, 2022

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 stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 16, 2022
@chrysn
Copy link
Member Author

chrysn commented Apr 16, 2022

I'm giving this another period of getting-stale -- the pressing need is gone, but the concern that which is not standardized wheres command is remains. Letting this sit for some more time, and when all the evil make versions have gone out of use, doing this all over should be easy.

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Apr 16, 2022
@chrysn chrysn removed the State: waiting for CI update State: The PR requires an Update to CI to be performed first label Apr 16, 2022
@stale
Copy link

stale bot commented Nov 2, 2022

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 stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022
@chrysn
Copy link
Member Author

chrysn commented Nov 2, 2022

We now do have the CI updated. Giving it a run as it is before rebasing.

@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 2, 2022
@chrysn chrysn marked this pull request as ready for review November 2, 2022 08:10
@Teufelchen1
Copy link
Contributor

@chrysn Ping. If you rebase & fix conflicts, I'll give it a review.

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 26, 2024
@chrysn chrysn force-pushed the which-hunt-the-next-generation branch from 430566f to 5463917 Compare March 26, 2024 20:51
@github-actions github-actions bot added Area: pkg Area: External package ports Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Mar 26, 2024
@chrysn
makefiles: Replace which with command -v
23f426c 
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
@chrysn
makefiles: Reject shell which in Makefiles
87df0fa 
... as that command is deprecated at least on Debian, and a good
replacement is available in the form of `command -v`.
@chrysn chrysn force-pushed the which-hunt-the-next-generation branch from 5463917 to 87df0fa Compare March 26, 2024 20:58
@chrysn chrysn requested a review from basilfx as a code owner March 26, 2024 20:58
@github-actions github-actions bot added the Area: boards Area: Board ports label Mar 26, 2024
@riot-ci
Copy link

riot-ci commented Mar 26, 2024 •
edited
Loading

Murdock results

✔️ PASSED

ad43fde makefiles: Replace faulty command command

Success Failures Total Runtime
10031 0 10031 11m:21s

Artifacts

@chrysn
makefiles: Replace faulty command command
ad43fde 
While some implementations of `command` accept extra arguments like
`which` does, and act on them in sequence, POSIX does not prescribe that,
and dash just processes one argument.
@chrysn
Copy link
Member Author

chrysn commented Mar 26, 2024

CI discovered a really nasty bug in the migration from which to command-v: Different implementations of the latter may or may not tolerate multiple arguments.

@chrysn
Copy link
Member Author

chrysn commented Apr 6, 2024

@Teufelchen1, could you review this after the hardfreeze?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@basilfx basilfx Awaiting requested review from basilfx basilfx is a code owner

Assignees
No one assigned
Labels
Area: boards Area: Board ports 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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrysn @Teufelchen1 @riot-ci