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.

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

build: fix inability to detect correct python command in configure #32925

Closed
wants to merge 1 commit into from
Closed

build: fix inability to detect correct python command in configure #32925

wants to merge 1 commit into from

Conversation

eli-schwartz
Copy link
Contributor

The "which" utility is not guaranteed to be installed, and if it is, its behavior is not portable.

Conversely, the "command -v" shell builtin is required to exist in all POSIX 2008 compliant shells, and is thus guaranteed to work everywhere.

Examples of open-source shells likely to be installed as /bin/sh on Linux, which implement the 12-year-old standard: ash, bash, busybox, dash, ksh, mksh and zsh.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

The "which" utility is not guaranteed to be installed, and if it is, its
behavior is not portable.

Conversely, the "command -v" shell builtin is required to exist in all
POSIX 2008 compliant shells, and is thus guaranteed to work everywhere.

Examples of open-source shells likely to be installed as /bin/sh on
Linux, which implement the 12-year-old standard: ash, bash, busybox,
dash, ksh, mksh and zsh.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 19, 2020
@sam-github
Copy link
Contributor

This seems a high-risk change, in that verifying it across all our user's systems is not possible, we'll know if its a problem only when it breaks something that used to work, and its not clear - what actual problem is it fixing? Is there a system on which the current script doesn't work?

@eli-schwartz
Copy link
Contributor Author

Exactly the problem which I described: my system does not have the "which" thirdparty utility installed.

(My system happens to be Arch Linux. The /usr/bin/which program is only a required operating system component on Debian, which I'm not running. I could optionally install GNU which, but it is not available by default and requires manual intervention to install it. This script fails to find any type of python3.* program, and eventually does the fallback on python which does happen to exist.)

...

Furthermore, if it does exist, it might not do what you think it does. For some in-depth discussions on the topic, see https://mywiki.wooledge.org/BashFAQ/081 or https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then/85250#85250

...

This is not high-risk, since the configure script must be run with /bin/sh and every single /bin/sh on any system anywhere has a builtin shell feature called command -v, as long as it was produced within the last 12 years, and a number of them have had it for much, much longer.
I have listed all the major shells in use today, and confirmed they all support it.

Conversely, the "command -v" shell builtin is required to exist in all POSIX 2008 compliant shells, and is thus guaranteed to work everywhere.

On any POSIX 2008 compliant /bin/sh, a very old standard indeed, command -v must work, and must return a successful exit code when the searched-for command exists as a path or shell function or builtin. You will know if it is a shell function, because it will be defined in your script, which is under your control... you also know that python3.8 is not a shell builtin.

It is literally the definition of portable.

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen command -v being used as a more portable approach in other projects, so LGTM. If we want to be extra careful, we could have a double-check, something like:

WHICH=($(command -v command >/dev/null && echo "command -v" || echo "which"))
"@{WHICH[@]}" python3.8 >/dev/null && exec python3.8 "$0" "$@"

This would ensure we'll use command -v or which, depending on platform availability.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2020

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2020
@eli-schwartz
Copy link
Contributor Author

Using arrays is utterly unacceptable, since this is a /bin/sh script and arrays are not part of the shell command language. They're a bashism, which might work on some systems iff the system /bin/sh happens to be a symlink to bash or ksh or zsh, but this is much less likely to work than the which program...
(If you are willing to depend on #/usr/bin/env bash, then you don't need a fallback since bash will always support command -v. The oldest version of bash I can find to test, bash 1.14 as released in June 1994, supports command -v. Before POSIX required command -v, it was still recommended as a "user portability utilities" option, and bash implemented those.)

GNU autotools has a complex boilerplate routine which uses pure shell to find programs, including routines for detecting whether the $PATH uses the : character or the ; character for splitting elements, and, for kicks, includes support for finding the program with an optional extension such as ".bat" or ".exe" (but since such extensions cannot be reliably probed, it's the responsibility of the system vendor to define a suitable $ac_executable_extensions in their config.site; autotools is common enough that those vendors do indeed create the well-documented config.site). It's only a dozen or two lines. :p

But none of this is necessary unless your actual goal is, like autotools configure scripts, to support building on systems that predate POSIX. And autotools takes their portability very seriously -- they also scrupulously go to pains to use the confusingly ambiguous, deprecated

`command`

form of command substitution, instead of $(command), since it's the only way to ensure autotools works on "the very oldest of non-POSIX-compatible bourne-shells".

@BridgeAR
Copy link
Member

Landed in b21556d

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 25, 2020
The "which" utility is not guaranteed to be installed, and if it is, its
behavior is not portable.

Conversely, the "command -v" shell builtin is required to exist in all
POSIX 2008 compliant shells, and is thus guaranteed to work everywhere.

Examples of open-source shells likely to be installed as /bin/sh on
Linux, which implement the 12-year-old standard: ash, bash, busybox,
dash, ksh, mksh and zsh.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>

PR-URL: nodejs#32925
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR closed this May 25, 2020
@eli-schwartz eli-schwartz deleted the command-v-portability branch May 25, 2020 17:31
codebytere pushed a commit that referenced this pull request Jun 18, 2020
The "which" utility is not guaranteed to be installed, and if it is, its
behavior is not portable.

Conversely, the "command -v" shell builtin is required to exist in all
POSIX 2008 compliant shells, and is thus guaranteed to work everywhere.

Examples of open-source shells likely to be installed as /bin/sh on
Linux, which implement the 12-year-old standard: ash, bash, busybox,
dash, ksh, mksh and zsh.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>

PR-URL: #32925
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
The "which" utility is not guaranteed to be installed, and if it is, its
behavior is not portable.

Conversely, the "command -v" shell builtin is required to exist in all
POSIX 2008 compliant shells, and is thus guaranteed to work everywhere.

Examples of open-source shells likely to be installed as /bin/sh on
Linux, which implement the 12-year-old standard: ash, bash, busybox,
dash, ksh, mksh and zsh.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>

PR-URL: #32925
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants