-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Unix entry points: correctly detect the availability of a python binary #15071
Conversation
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.
Thank you for submitting a pull request! Someone will be along to review it shortly. |
Oh dear, that seems ominous. |
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. |
To modify these entry points you will want to edit 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. |
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... |
@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. |
There was a problem hiding this 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)
Thanks for the feedback @eli-schwartz , maybe we should remove the bot then, I'll look into that. |
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.
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
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
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.