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

Add AIX support #1295

Merged
merged 1 commit into from
Nov 10, 2016
Merged

Add AIX support #1295

merged 1 commit into from
Nov 10, 2016

Conversation

gdams
Copy link

@gdams gdams commented Nov 10, 2016

Currently AIX isn't supported in nvm so I have made a few small changes to make this work:
-Change tar to gtar for AIX
-Change make to gmake for AIX
-Set ARCH as ppc64 for AIX

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

What's AIX? Can it be run on travis-ci or any other CI system?

I'm very hesitant to add support for an OS that isn't either incredibly popular, or doesn't allow me to automatically verify that it works in CI.

else
HOST_ARCH="$(command uname -m)"
fi

Copy link
Member

Choose a reason for hiding this comment

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

please leave these blank lines

if (
[ -n "${TMPDIR-}" ] && \
command mkdir -p "${TMPDIR}" && \
command tar -x${tar_compression_flag}f "${TARBALL}" -C "${TMPDIR}" --strip-components 1 && \
command $tar -x${tar_compression_flag}f "${TARBALL}" -C "${TMPDIR}" --strip-components 1 && \
Copy link
Member

Choose a reason for hiding this comment

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

please surround the tar variable in quotes, and use curly braces for the expansion

@@ -1693,6 +1699,7 @@ nvm_download_artifact() {
fi

local SLUG
echo $NVM_ARCH
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it's just here for debugging?

@@ -1851,7 +1868,7 @@ nvm_install_source() {
[ -f "${TARBALL}" ] && \
TMPDIR="$(dirname "${TARBALL}")/files" && \
command mkdir -p "${TMPDIR}" && \
command tar -x${tar_compression_flag}f "${TARBALL}" -C "${TMPDIR}" --strip-components 1 && \
command $tar -x${tar_compression_flag}f "${TARBALL}" -C "${TMPDIR}" --strip-components 1 && \
Copy link
Member

Choose a reason for hiding this comment

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

also here

fi

Copy link
Member

Choose a reason for hiding this comment

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

please remove trailing whitespace

@ljharb ljharb added the feature requests I want a new feature in nvm! label Nov 10, 2016
@gdams
Copy link
Author

gdams commented Nov 10, 2016

made those changes. AIX is an IBM platform, we have tested these changes on our AIX machines and observed no errors

@ljharb
Copy link
Member

ljharb commented Nov 10, 2016

@GeorgeAdams95 yes but I need to be sure, going forward, that future changes don't break it, and relying on humans to test it isn't scalable. Is there any free open source CI platform that can run tests on AIX?

@gdams
Copy link
Author

gdams commented Nov 10, 2016

@ljharb, It is very hard to get hold of AIX, let alone have a CI platform. Can we just add the code but not officially support the OS. It is only really going to be used by IBM and the Node.js community

@ljharb
Copy link
Member

ljharb commented Nov 10, 2016

That's a lot of risk to take on. IBM is a member of the node foundation, I think if they want their OS supported, they need to invest in a way for it to be supported.

Currently AIX isn't supported in nvm so I have made a few small changes
to make this work:
change tar to gtar for AIX
change make to gmake for AIX
Set ARCH as ppc64 for AIX
@sam-github
Copy link

@ljharb AIX is supported in CI by the node foundation, see https://ci.nodejs.org/job/node-test-commit-aix/, pretty sure that was an IBM contribution. But that doesn't help nvm (not unless you convince the foundation to give you access to their CI, which I understand is under discussion, but they said no for now, unless I misremember the discussion).

IBM isn't going to be afford to give every node project an AIX machine, and it can't convince Travis to have AIX, so.... where else do you think we should invest? How do we get more platform support into nvm? Do you suggest we just wait until nvm, or whatever the new working group comes up, has joined the foundation and then re-request this?

The working group will likely want all the platforms on https://nodejs.org/en/download/ (you may not have noticed, but this includes AIX) to be supported, and this PR helps for nvm, at least.

@ljharb
Copy link
Member

ljharb commented Nov 10, 2016

Thanks for the additional info.

Given that AIX is a supported platform, and given that the node CI runs it (and that nvm doesn't need to be a foundation member to necessarily use the infra), I think this is worth taking now, even without tests.

I would ask you to be vigilant about nvm changes in the future, and hopefully we can catch AIX breakage sooner than later, should it occur.

@ljharb ljharb added the OS: AIX label Nov 10, 2016
@gdams
Copy link
Author

gdams commented Nov 10, 2016

@ljharb we will be hopefully using this often so hopefully can detect issues as they occur

@mhdawson
Copy link

@ljharb thanks. I can confirm that we (IBM) contributed the AIX hardware to the node.js community and that we actively work to keep Node.js community CI jobs green across AIX and other platforms we've contributed H/W to the community CI for.

As @sam-github says once the new version management workgroup makes progress lets make sure to revisit and if we get to the point were we can add CI coverage for nvm using the community infrastructure I'm happy to help to try and make that happen.

In the mean time, @GeorgeAdams95 and @ljharb is there set of tests that can be easily run from Jenkins CI ? If so I'l see if @GeorgeAdams95 can look at adding a run at some interval in our internal CI.

@gdams
Copy link
Author

gdams commented Nov 10, 2016

@mhdawson we could run the existing test suite on jenkins occasionally ?

@gdams
Copy link
Author

gdams commented Nov 10, 2016

@mhdawson i'll look into it and see if we can run the existing tests on AIX in our CI

@ljharb ljharb merged commit 8f82eab into nvm-sh:master Nov 10, 2016
@ljharb ljharb added the installing node Issues with installing node/io.js versions. label Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. OS: AIX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants