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

JBIDE-20351: Improving Node detection for Ubuntu #507

Merged
merged 1 commit into from
Aug 3, 2015

Conversation

ibuziuk
Copy link
Member

@ibuziuk ibuziuk commented Jul 23, 2015

No description provided.

@alexeykazakov
Copy link
Contributor

What will happen if there are both "node" (not a node.js package) and "nodejs" (node.js package) available? Which one will be used? Incorrect "node"? If so, shouldn't we look for nodejs on Linux first and if we fail then look for "node"? Is there any good way to distinguish a "fake" node and a real one?
And BTW we need to add test for nodejs detection and for "node" too if we don't have it already.

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 23, 2015

What do you mean by "fake" ? Both "nodejs" and "node" possible names for Node.js executables

@maxandersen
Copy link
Member

yes, but on ubuntu it seems you can have both node and nodejs available at the same time and node won't be the right one.

@alexeykazakov
Copy link
Contributor

Exactly. "node" in Ubuntu is usually a command from Amateur Packet Radio Node Program package. Not from Node.JS. It's why they use nodejs. So in Ubuntu, if you have both "node" and "nodejs" then it's more likely that "nodejs" is the right one and "node" is the wrong one.

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 23, 2015

Ok got it. Will update PR tomorrow. Need to figure out how to distinguish two of those executables

@alexeykazakov
Copy link
Contributor

Don't forget to add tests for "nodejs"/"node" detection ;)

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 24, 2015

got an idea of how to distinguish Amateur Packet Radio Node from Node.js executables. Basically, those two have a significant installed size difference - Amateur Packet Radio Node ~ 34Kb ( http://packages.ubuntu.com/trusty/node) and Node.js ~ 7 - 11 Mb depending on version. So, depending on the file's metadata we can find out whether "node" is trustworthy. WDYT ?
In terms of test, should I mock node locations?

@alexeykazakov
Copy link
Contributor

What I see in your code. If there are both "nodejs" and "node" on Linux then "nodejs" is used. If there is only "node" then you use it only if its size more than 5MB.
But I guess this won't work if user created a symlink named "node" to the "nodejs". The size of this link will be small. You can resolve symlinks using a canonical path. And I would decries this limit of "node" files to something insane for a real node.js binary. 300KB or something like that. So if they shrink some feature version of node.js we will still be able to detect it. IMO it's better to detect a wrong "node" (it's a very well known problem for node.js users in Ubuntu) than to not recognize a "node" tuned by some advanced user.
And sorry for being a pain in the butt but I'm still waiting for tests ;)

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 25, 2015

tests are not at all a pain in the butt ;-) I just need a piece of advice how to write good tests for this particular case. I mean how this public detectNode() method can be properly tested ?
Today I figure out that we might probably over complicate this problem with Amateur Packet Radio Node program. Here are my thoughts:

  1. if we detect nodejs first on linux there will be no problem because...

  2. Let's assume that user know that native node must be pre-installed. Than there is no problem - he will either install nodejs ("nodejs" binary) our install nodejs-legacy ("node" binary). If he had this Amateur Packet Radio package and still want to use Node.js as "node" command he will have to perform apt-get remove node and remove Radio package node. As you said, this is a well-known issue. details here - http://askubuntu.com/questions/235655/node-js-conflicts-sbin-node-vs-usr-bin-node

  3. If the user doesn't know that native node must be preinstalled. If there is no Amateur Packet Radio package on his ubuntu, we are fine - pop-up will be shown with info that node.js was not detected, so now he aware of the fact that he must pre-install Node.js . if he has this package and we have detected it, once "bower update" shortcut will be executed, he will get pop-up that launch was not successful and preferences with node / bower might be the case. Once he will figure out that he must install nodejs we will have no problem.

@ibuziuk
Copy link
Member Author

ibuziuk commented Jul 25, 2015

and one more point here: in order to install native bower user will need to install node.js and npm firstly. So, if he has only Amateur Packet Radio node the error pop-up for "bower update" will be shown anyways, cause bower will not be detected. Once node.js, npm and bower installed the detection will work just fine.

@alexeykazakov
Copy link
Contributor

OK that makes sense. So just using nodejs if it's available will solve the problem. If it's not available then we should look for "node". That's it.
And I will think about tests.

@alexeykazakov alexeykazakov merged commit 0135d23 into jbosstools:master Aug 3, 2015
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.

None yet

3 participants