Fix support for installing node on SmartOS #854

misterdjules · 2015-09-28T23:10:09Z

uname on SmartOS cannot be used to guess if 32 and/or 64 bits binaries
are supported, and its output is different than other uname commands on
other operating systems.

This change uses pkg_info to determine what types of binaries pkgsrc would
install. If pkg_info fails to run or is not present, this change falls
back to using isainfo -n, which determines what the kernel supports.

This change also allows users to install node binaries from Solaris
derivatives. It doesn't remove the limitation that prevents them from
installing io.js binaries because not all io.js versions have binaries
for Solaris derivatives.

misterdjules · 2015-09-28T23:13:07Z

I'm not sure how to write unit tests for nvm_get_arch, as what I have in mind currently would mean reimplementing nvm_get_arch to test it. I would appreciate any idea on how to do that cleanly.

Also, currently without these changes and as of commit 8aebf86, when running make test 3 tests fail. No other tests fail when running tests with these changes.

I tested these changes on SmartOS with bash, and on OSX with sh, ksh and zsh.

ljharb · 2015-09-28T23:35:57Z

nvm.sh

@@ -1485,7 +1502,7 @@ nvm() {
      if [ "_$NVM_OS" = "_freebsd" ]; then
        # node.js and io.js do not have a FreeBSD binary
        nobinary=1
-      elif [ "_$NVM_OS" = "_sunos" ] && ([ "$NVM_IOJS" = true ] || [ "$NVM_NODE_MERGED" = true ]); then
+      elif [ "_$NVM_OS" = "_sunos" ] && [ "$NVM_IOJS" = true ]; then


It looks like https://iojs.org/dist/v3.3.1/ is the only io.js version that has a sunos binary - could we change this to only set nobinary to 1 if the version is less than v3.3.1?

Sure, no problem.

Updated the PR according to this comment.

ljharb · 2015-09-29T00:57:42Z

This seems like a great change - thank you! The things I'll need to think about before being confident enough to merge it:

unit test coverage, part of which you mentioned
is it possible to run sunOS tests?
making absolutely sure that existing install scenarios aren't broken

I'll test this out myself tonight/tomorrow, and ask a few others to do so as well, to help with this.

misterdjules · 2015-09-29T01:01:06Z

unit test coverage, part of which you mentioned

I'm still thinking about how to write a test that would not be the exact copy of the implementation itself :)

is it possible to run sunOS tests?

Do you mean to manually check if this change works on sunOS?

ljharb · 2015-09-29T01:03:22Z

no, i mean, add something to travis that would ensure this doesn't break in the future

misterdjules · 2015-09-29T01:10:02Z

@ljharb Actually, I can probably just mock uname, isainfo and pkg_info for all supported archs and have them output static constant output. Then I can override PATH for each test so that uname, isainfo and pkg_info point to the mocked version (or to nothing if any of these commands should not be present for a given supported arch), and finally check that nvm_get_arch returns the expect output.

Sounds good? I'll update that PR soon, although maybe not tonight.

ljharb · 2015-09-29T02:01:39Z

For nvm_get_arch that sounds awesome.

Are isainfo or pkginfo part of POSIX? What shells/platforms do they work on?

misterdjules · 2015-09-29T04:23:14Z

isainfo is not POSIX, it works on SmartOS and Solaris, and maybe on other Solaris derivatives. pkginfo is not POSIX, it is often available on SmartOS, and might be usable on Solaris. It's not mandatory though in the current state of this PR.

misterdjules · 2015-09-29T06:05:26Z

Updated this PR with a first shot at writing tests for nvm_get_arch. Please let me know if that looks like it's going towards the right direction :)

EDIT: obviously, tests for other archs/OSes then {SmartOS,OSX}/{x64/i386} are missing, I'll add more if we validate this approach.

ljharb · 2015-09-30T03:21:03Z

nvm.sh

+  # isainfo to get the instruction set supported by the
+  # kernel.
+  if [ "_$NVM_OS" = "_sunos" ]; then
+    HOST_ARCH=$(pkg_info -Q MACHINE_ARCH pkg_install)


should this check nvm_has pkg_info? currently if it fails, it'll fall through to echoing the empty string, which isn't likely what nvm_get_arch should do.

Right, good catch 👍 The original intent was to catch this case with:

if [ $EXIT_CODE -ne 0 ]; then HOST_ARCH=$(isainfo -n) fi

but the local command before the test for $? would overwrite $?. So I moved the local declaration at the top of the function and left it as is. I also added tests to catch that problem.

I think having a nvm_has_pkg_info function is a bit overkill for this use case, but If you'd still prefer to handle it that way I'm fine with it too.

We already have nvm_has - i don't mean a new function, i mean calling nvm_has with the argument pkg_info

ljharb · 2015-09-30T17:49:20Z

Thanks, I think this is almost there! I'll test it out myself today, since my comments are mostly about tests.

misterdjules · 2015-09-30T18:33:21Z

@ljharb Updated this PR according to your feedback. I've been adding commits to make changes related to code reviews easier to separate from the rest, but obviously once this is ready to land I'll squash them into one commit.

ljharb · 2015-10-01T04:54:42Z

test/mocks/isainfo_amd64

+if [ "_$1" = "_-n" ]; then
+  echo "amd64"
+else
+	echo "amd64 i386"


ljharb · 2015-10-01T05:00:20Z

I've tested this out, and it looks good - some tab characters to replace with spaces, but otherwise please rebase as you see fit, and I'll merge!

misterdjules · 2015-10-01T05:03:01Z

@ljharb That's great, thank you for the thoughtful and kind reviews, I really appreciate it. Will update and squash soon.

uname on SmartOS cannot be used to guess if 32 and/or 64 bits binaries are supported, and its output is different than other uname commands on other operating systems. This change uses pkg_info to determine what types of binaries pkgsrc would install. If pkg_info fails to run or is not present, this change falls back to using isainfo -n, which determines what the kernel supports. It allows users to install node binaries on Solaris derivatives. io.js can also be installed on Solaris derivatives starting with version v3.3.1.

misterdjules · 2015-10-01T05:12:07Z

Fixed the two formatting issues, and squashed all commits into one.

Fix support for installing node on SmartOS

ljharb · 2015-10-01T07:05:01Z

Thanks for being so thorough with this PR!

misterdjules · 2015-10-01T17:10:02Z

@ljharb Thank you for merging it! Do you know when that will make it into a release? I know significant users of node and SmartOS who are looking forward to being able to use nvm on SmartOS :)

ljharb · 2015-10-02T08:52:10Z

Released in v0.28.0!

misterdjules · 2015-10-02T15:30:14Z

@ljharb That's great, thank you very much once again for your help!

ljharb reviewed Sep 28, 2015
View reviewed changes

misterdjules force-pushed the fix-node-smartos-support branch from f4a69e6 to 8241fcb Compare September 29, 2015 00:57

misterdjules force-pushed the fix-node-smartos-support branch from 8241fcb to b9c13c4 Compare September 29, 2015 06:04

misterdjules force-pushed the fix-node-smartos-support branch 2 times, most recently from e449b03 to b3050ee Compare September 29, 2015 16:12

ljharb reviewed Sep 30, 2015
View reviewed changes

misterdjules force-pushed the fix-node-smartos-support branch 2 times, most recently from b419a03 to 94b93e4 Compare October 1, 2015 04:23

ljharb reviewed Oct 1, 2015
View reviewed changes

test/mocks/isainfo_amd64

if [ "_$1" = "_-n" ]; then

echo "amd64"

else

echo "amd64 i386"

Copy link

Member

ljharb Oct 1, 2015

Choose a reason for hiding this comment

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

s/\t/ /

misterdjules force-pushed the fix-node-smartos-support branch from 94b93e4 to 2d692d9 Compare October 1, 2015 05:11

ljharb added the installing node Issues with installing node/io.js versions. label Oct 1, 2015

ljharb added a commit that referenced this pull request Oct 1, 2015

Merge pull request #854 from misterdjules/fix-node-smartos-support

f73ee40

Fix support for installing node on SmartOS

ljharb merged commit f73ee40 into nvm-sh:master Oct 1, 2015

misterdjules mentioned this pull request Jun 10, 2016

Please rename "sunos" binaries because of broken ABI nodejs/node#7115

Closed

ljharb added the OS: Solaris label Jun 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix support for installing node on SmartOS #854

Fix support for installing node on SmartOS #854

misterdjules commented Sep 28, 2015

misterdjules commented Sep 28, 2015

ljharb Sep 28, 2015

misterdjules Sep 29, 2015

misterdjules Sep 29, 2015

ljharb commented Sep 29, 2015

misterdjules commented Sep 29, 2015

ljharb commented Sep 29, 2015

misterdjules commented Sep 29, 2015

ljharb commented Sep 29, 2015

misterdjules commented Sep 29, 2015

misterdjules commented Sep 29, 2015

ljharb Sep 30, 2015

misterdjules Sep 30, 2015

ljharb Sep 30, 2015

ljharb commented Sep 30, 2015

misterdjules commented Sep 30, 2015

ljharb Oct 1, 2015

ljharb commented Oct 1, 2015

misterdjules commented Oct 1, 2015

misterdjules commented Oct 1, 2015

ljharb commented Oct 1, 2015

misterdjules commented Oct 1, 2015

ljharb commented Oct 2, 2015

misterdjules commented Oct 2, 2015

Fix support for installing node on SmartOS #854

Fix support for installing node on SmartOS #854

Conversation

misterdjules commented Sep 28, 2015

misterdjules commented Sep 28, 2015

ljharb Sep 28, 2015

Choose a reason for hiding this comment

misterdjules Sep 29, 2015

Choose a reason for hiding this comment

misterdjules Sep 29, 2015

Choose a reason for hiding this comment

ljharb commented Sep 29, 2015

misterdjules commented Sep 29, 2015

ljharb commented Sep 29, 2015

misterdjules commented Sep 29, 2015

ljharb commented Sep 29, 2015

misterdjules commented Sep 29, 2015

misterdjules commented Sep 29, 2015

ljharb Sep 30, 2015

Choose a reason for hiding this comment

misterdjules Sep 30, 2015

Choose a reason for hiding this comment

ljharb Sep 30, 2015

Choose a reason for hiding this comment

ljharb commented Sep 30, 2015

misterdjules commented Sep 30, 2015

ljharb Oct 1, 2015

Choose a reason for hiding this comment

ljharb commented Oct 1, 2015

misterdjules commented Oct 1, 2015

misterdjules commented Oct 1, 2015

ljharb commented Oct 1, 2015

misterdjules commented Oct 1, 2015

ljharb commented Oct 2, 2015

misterdjules commented Oct 2, 2015