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

Unix entry points: correctly detect the availability of a python binary #15071

Merged
merged 2 commits into from
Sep 20, 2021

Conversation

eli-schwartz
Copy link
Contributor

The "which" program is not suitable for determining the existence or location of a python binary, because it is a non-standard program that is often not installed. It is often possible to manually install it as a third-party addon anyway, but this is a very bad thing to rely on in order to get good out of the box behavior.

If and when it is installed, it may display various incorrect behaviors, including unparseable output on stdout instead of stderr, outputting the contents of ~/.cshrc aliases, and more.

For more details on why never to use "which", see:

https://mywiki.wooledge.org/BashFAQ/081
https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then/85250#85250

Instead, use the POSIX 2008 mandated builtin "command -v" implemented by all valid /bin/sh implementations, including ash, bash, busybox sh, dash, ksh, mksh, and zsh.

"command -v" has an actual standards body guaranteeing the format of the output, and that it is usable as a filesystem path to an executable, for all shells produced in the last 13 years. It is thus unlikely to break in the same way "which" does.

The "which" program is not suitable for determining the existence or
location of a python binary, because it is a non-standard program that
is often not installed. It is often possible to manually install it as a
third-party addon anyway, but this is a very bad thing to rely on in
order to get good out of the box behavior.

If and when it is installed, it may display various incorrect behaviors,
including unparseable output on stdout instead of stderr, outputting the
contents of ~/.cshrc aliases, and more.

For more details on why never to use "which", see:

https://mywiki.wooledge.org/BashFAQ/081
https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then/85250#85250

Instead, use the POSIX 2008 mandated builtin "command -v" implemented by
all valid /bin/sh implementations, including ash, bash, busybox sh,
dash, ksh, mksh, and zsh.

"command -v" has an actual standards body guaranteeing the format of the
output, and that it is usable as a filesystem path to an executable, for
all shells produced in the last 13 years. It is thus unlikely to break
in the same way "which" does.
@welcome
Copy link

welcome bot commented Sep 20, 2021

Thank you for submitting a pull request! Someone will be along to review it shortly.

@eli-schwartz
Copy link
Contributor Author

Welcome bot promising me that contrary to what it looks like, someone will actually respond to the PR.

Oh dear, that seems ominous.

@kripken kripken requested a review from sbc100 September 20, 2021 20:22
@kripken
Copy link
Member

kripken commented Sep 20, 2021

I'm not qualified to review the actual change here, but about the bot message, that is meant to be welcoming... Were you just joking, or was the bot message actually confusing for you? (Did something else give an indication that the PR would not be responded to?) If so, we could just remove the bot entirely.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 20, 2021

To modify these entry points you will want to edit tools/run_python.sh and tools/run_python_compiler.sh and then run tools/create_entry_points.py.

Other than that, this change seem good. Given that have had zero reports of this being an issue in practive, it seems like your concerns about which bring "often not installed" might be somewhat overblown, but that isn't a reason to keep using it if there is a better alternative. Thanks for pointing this out.

@eli-schwartz
Copy link
Contributor Author

The bot message confused me, because I haven't really seen such a thing before. I usually just get responses from reviewers in short order.

I wasn't sure if the message was in response to that having been a problem people were concerned about, or what...

@eli-schwartz
Copy link
Contributor Author

@sbc100 it is available on debian because it's a required base system component on debian, so that probably helps. It's also possibly the main reason why people are aware that "which" exists -- because debian prefers it in their own in-house dpkg maintainer scripts.

It is not, for example, installed on Arch Linux.

...

Anyway I've previously had people be worried about merging similar changes because they weren't sure how portable it is, so I've tended to start off with being very specific about why it might not work. From a technical design standpoint the flakiness it introduces can lead to very odd behavior indeed. From a userbase standpoint, people with the issue are statistically insignificant in the sea of Debian and Ubuntu and further reduced by the likelihood that e.g. desktop OS profiles have installed it for the sake of interactive use.

I believe I've correctly edited the templates, though. My second commit even mentions that it is the result of rerunning the entry points python generator script.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Sweet.

Thanks for taking care of this. Its nice that its a shell built-in too since it means one less process exec for each compiler command.

Regarding Arch linux, does that mean that we don't have any arch linux users then? Or is that that majority of arch linux users (at least all the ones that use emscripten) happen to have a working /usr/bin/which installed?

IIRC there is actually an arch linux package .. how did they get away without this patch? (https://github.com/archlinux/svntogit-community/tree/packages/emscripten/trunk)

@sbc100 sbc100 merged commit 9d63033 into emscripten-core:main Sep 20, 2021
@kripken
Copy link
Member

kripken commented Sep 21, 2021

Thanks for the feedback @eli-schwartz , maybe we should remove the bot then, I'll look into that.

kripken added a commit that referenced this pull request Sep 21, 2021
The bot used to be very important while we required people to add
themselves to AUTHORS - it was a useful reminder for that. But we've
removed that requirement. Now the bot just says a friendly message,
but we've gotten negative feedback on that,

#15071 (comment)

I think I agree that a content-less message like that kind of sends an
unclear signal, as the normal expectation is to get someone to review it.
That is, the bot message sounds defensive. Let's just remove it.
@eli-schwartz eli-schwartz deleted the command-v-portability branch June 30, 2022 13:20
akoeplinger added a commit to akoeplinger/emsdk that referenced this pull request Jun 28, 2024
On a Linux distro that doesn't have the `which` program installed we're getting the following error:

    ./emsdk install latest
    ./emsdk: line 39: exec: python: not found

It's failing to detect the installed `python3` and falls back to using `python`, but this distro doesn't provide a python -> python3 symlink so we fail.

Fix this by using `command -v` instead which is a POSIX standard.
The same change went into emscripten a couple years ago: emscripten-core/emscripten#15071
sbc100 pushed a commit to emscripten-core/emsdk that referenced this pull request Jul 1, 2024
On a Linux distro that doesn't have the `which` program installed we're
getting the following error:

    $ ./emsdk install latest
    ./emsdk: line 39: exec: python: not found

It's failing to detect the installed `python3` and falls back to using
`python`, but this distro doesn't provide a python -> python3 symlink so
we fail.

Fix this by using `command -v` instead which is a POSIX standard.
The same change went into emscripten a couple years ago:
emscripten-core/emscripten#15071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants