Fix calling node/nodejs binary in nodejs_version
#1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The primary nodejs executable being called `nodejs` is a Debian-ism, and is not compatible with upstream nodejs and non-Debian distros, because upstream just calls the binary `node`. - This makes the `node_version` method fail on non-Debian systems, where `nodejs` is actually called `node` instead. - Instead of just executing the `nodejs` command, we now first search for it in `PATH`, using `distutils.spawn.find_executable`, which should be available on Python 2.4+. - When no node executable is found, a NodeBinaryNotFoundError is now raised. - If one of the node executables is found, we return the result of `node/nodejs --version` as before.
xolox
added a commit
that referenced
this pull request
Sep 17, 2016
|
Hey Maikel and thanks for 1) reporting this issue and 2) providing a fix! Obviously I know so little about Node.js that I never realized that the official name of the interpreter is simply I changed things around quite a bit while merging your pull request but conceptually the same fix is now implemented. I just pushed the results to GitHub, if Travis CI doesn't complain I will publish this to PyPI as well. |
|
Travis CI agrees (including the new test I added) so this is now on PyPI! |
|
This means So I made an AUR package for it (and its dependencies): https://aur.archlinux.org/packages/npm-accel/ |
xolox
added a commit
that referenced
this pull request
Sep 17, 2016
Refer to pull request #1 for details: #1 (comment)
|
Thanks for that Maikel! I've added a mention of the AUR package in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary nodejs executable being called
nodejsis a Debian-ism, and is not compatible with upstream nodejs and non-Debian distros, because upstream just calls the binarynode.This makes the
node_versionwhich callsnodejs --versionmethod fail on non-Debian systems, wherenodejsis actually callednodeinstead.Instead of just executing the
nodejscommand, we now first search for it inPATH, usingdistutils.spawn.find_executable, which should be available on Python 2.4+. (see also: http://stackoverflow.com/a/12611523)When no node executable is found, a NodeBinaryNotFoundError is now raised.
If one of the node executables is found, we return the result of
node/nodejs --versionas before.Edit: to clarify: the reasoning behind Debian not calling node
node, is that/usr/bin/nodeis actually in use by a package that pre-exists nodejs (ax25-node). Since the search fornodejsis before the search fornode, this should not give any problems for users that have both installed. There might be a false positive whenax25-nodeis installed, but nodejs is not, but I think we can assume here npm users have nodejs installed already anyway ;)