build: add support for IBM i platform #19667

ThePrez · 2018-03-29T02:41:48Z

This pull request proposes some minor changes to support builds on the IBM i platform.

Let me offer some background that (I hope) will help you make sense of these small changes.

On IBM i, we enable a bulk of our open source technologies in an environment called the Portable Application Solution Environment (PASE).
PASE is an AIX-derived runtime. As such, many (but not all) AIX-capable applications and tools can run under the belief that they are running AIX.
The build environment on IBM i is a little different from AIX in that runtime linking is needed for most open source technologies, and an explicit libpath helps avoid conflicts with software that can exist outside the PASE environment.
The GYP build system thinks it's running AIX, but we can check for IBM i by querying uname output for "OS400" (legacy name of ancestor OS).

We added support to libuv in a similar manner. For reference: https://github.com/libuv/libuv/blob/edf05b97f03472def2837ceb2d6bf61a0d0dc248/uv.gyp#L296

In future work, I'd like to add IBM i as a supported platform, and will contribute relevant documentation at that point.

Thank you for consideration.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

joyeecheung · 2018-03-29T03:43:54Z

cc @nodejs/platform-aix @nodejs/build

gireeshpunathil · 2018-03-29T04:45:15Z

Approving in general, but have couple of comments:

(-Wl,-blibpath) : what libraries are expected to be present in /QOpenSys/pkgs/lib and /QOpenSys/usr/lib ?
This hardcoded LIBPATH is a standard / well known PATH for i libraries?
I am assuming that the CI does not have an i in it, so a build will reveal nothing, other than sanctity of other platforms, especially for AIX. @nodejs/build
I am also assuming that the current libuv (1.19.x) in use by Node.js has the changes for i, please confirm.
How / when / where do we plan for documenting the platform support? Probably through a separate PR after this one lands? @nodejs/documentation

Thanks @ThePrez !

gireeshpunathil · 2018-03-29T04:59:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13916/

bnoordhuis · 2018-03-29T06:54:50Z

node.gyp

+            ['"<(real_os_name)"=="OS400"', {
+              'ldflags': [
+                '-Wl,-blibpath:/QOpenSys/pkgs/lib:/QOpenSys/usr/lib',
+                '-Wl,-bbigtoc', '-Wl,-brtl'


Can you put the second argument on its own line? And maybe add a comma to keep future diffs smaller, same on lines 104 and 369.

bnoordhuis · 2018-03-29T10:16:15Z

Building addon /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/addons-napi/test_number/
  COPY Release/binding.node
gypmake[2]: write error
gyp ERR! build error 
 info gyp ERR! stack

Probably an infrastructure issue. I think I've seen that happen before.

New CI: https://ci.nodejs.org/job/node-test-pull-request/13920/

ThePrez · 2018-03-29T14:51:55Z

@gireeshpunathil, thanks for the review! Direct responses to your questions:

/QOpenSys/pkgs/lib and /QOpenSys/usr/lib are analogous to /lib or /usr/lib on other platforms. For the node binary, it will find libcrypto, libssl, libstdc++, libgcc_s, libpthreads, and libc. Also, that's where npm packages might find their needed libraries.
Yes, the hardcoded LIBPATH is a standard path. /QOpenSys/usr/lib contains all the base libraries shipped with the OS (like libc, libpthread), and /QOpenSys/pkgs/lib has the others.
Correct. The CI does not currently have an i in it.
The libuv support for IBM i was added in version 1.17.0. We do need a small patch to avoid crashes under load.
Once this PR lands, I'll plan on documenting the platform support for IBM i in a separate PR. I'd like to add IBM i at Tier 2 eventually. I'm unsure whether we should add it as "experimental" in the meantime.

ThePrez · 2018-03-29T14:53:00Z

@bnoordhuis, I addressed your suggestions via an amended commit. Thanks!

mhdawson

LGTM

mhdawson · 2018-04-04T21:48:19Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/14058/

MylesBorins · 2018-04-05T15:44:13Z

Not planning to block and don't have context to review, but would appreciate if we could get sign off from someone who is not an IBM employee. Have full faith in those involved, but think it would be good optics.

bnoordhuis · 2018-04-05T17:30:39Z

How do you envision that? I don't think there's anyone with a IBM i at their disposal to test the PR on.

mhdawson · 2018-04-05T22:40:00Z

@cjihrig any chance you would sign off?

mhdawson · 2018-04-05T22:43:21Z

Failure on debian looks unrelated- #19841

mhdawson · 2018-04-05T22:45:23Z

Failure on freebsd was:#19804

CI trying again:https://ci.nodejs.org/job/node-test-pull-request/14082/

cjihrig

LGTM. These same folks have been doing a fine job maintaining IBM i in libuv.

mhdawson · 2018-04-09T21:05:39Z

Lets hope 3rd time is the charm and no infra issues: https://ci.nodejs.org/job/node-test-pull-request/14158/

mhdawson · 2018-04-09T21:18:22Z

And again:https://ci.nodejs.org/job/node-test-pull-request/14160/

jasnell

rubber-stampy lgtm

mhdawson · 2018-04-10T14:52:46Z

Ok, given the mix of CI runs, all of the tests have passed at least once. In addition, there are open issues for the flaky tests that failed in one of the CI runs, going to land.

PR-URL: #19667 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson · 2018-04-10T15:01:46Z

Landed as 8170f4f

PR-URL: #19667 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 29, 2018

gireeshpunathil approved these changes Mar 29, 2018

View reviewed changes

bnoordhuis approved these changes Mar 29, 2018

View reviewed changes

build: add support for IBM i platform

c4af8bf

mhdawson approved these changes Apr 4, 2018

View reviewed changes

cjihrig approved these changes Apr 5, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018

jasnell approved these changes Apr 9, 2018

View reviewed changes

mhdawson closed this Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add support for IBM i platform #19667

build: add support for IBM i platform #19667

ThePrez commented Mar 29, 2018

joyeecheung commented Mar 29, 2018

gireeshpunathil commented Mar 29, 2018

gireeshpunathil commented Mar 29, 2018

bnoordhuis Mar 29, 2018

bnoordhuis commented Mar 29, 2018

ThePrez commented Mar 29, 2018

ThePrez commented Mar 29, 2018

mhdawson left a comment

mhdawson commented Apr 4, 2018

MylesBorins commented Apr 5, 2018

bnoordhuis commented Apr 5, 2018

mhdawson commented Apr 5, 2018

mhdawson commented Apr 5, 2018

mhdawson commented Apr 5, 2018

cjihrig left a comment

mhdawson commented Apr 9, 2018

mhdawson commented Apr 9, 2018

jasnell left a comment

mhdawson commented Apr 10, 2018 •

edited

Loading

mhdawson commented Apr 10, 2018

build: add support for IBM i platform #19667

build: add support for IBM i platform #19667

Conversation

ThePrez commented Mar 29, 2018

Checklist

joyeecheung commented Mar 29, 2018

gireeshpunathil commented Mar 29, 2018

gireeshpunathil commented Mar 29, 2018

bnoordhuis Mar 29, 2018

Choose a reason for hiding this comment

bnoordhuis commented Mar 29, 2018

ThePrez commented Mar 29, 2018

ThePrez commented Mar 29, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 4, 2018

MylesBorins commented Apr 5, 2018

bnoordhuis commented Apr 5, 2018

mhdawson commented Apr 5, 2018

mhdawson commented Apr 5, 2018

mhdawson commented Apr 5, 2018

cjihrig left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 9, 2018

mhdawson commented Apr 9, 2018

jasnell left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 10, 2018 • edited Loading

mhdawson commented Apr 10, 2018

mhdawson commented Apr 10, 2018 •

edited

Loading