build: Updates to enable AIX support #2364

mhdawson · 2015-08-12T18:09:09Z

These are the core changes that allow AIX to compile. There
are still some test failures as there are some patches needed for
libuv and npm that we'll need to contribute through those
communities but this set allows node to be built on AIX and
pass most of the core tests

The change in tools/js2c.py is because AIX does not support $ in
identifier names. See the discussion/agreement in
#2272

mhdawson · 2015-08-12T18:14:14Z

@jbergstroem, FYI. I'm still working on getting an AIX machine that can be used in the build farm but these are the changes that allow AIX to compile/run.

jbergstroem · 2015-08-12T20:06:10Z

@mhdawson if possible, it'd be great if I could get access to that machine (when available) so I can review this PR.

mhdawson · 2015-08-14T13:07:28Z

Unfortunately getting the machine is not that far along. Its more in the 1-2 month timeframe that I could see it becoming a reality so I don't want to block the PR on that. It is important to me to get these in so that our builds use as close as possible to the community source.

I can't give you access to the internal machines we have for building but I could set up a screen share where I've done builds and I could show you the builds and we could look into files that you are interested in looking at. If that works I'd like to set it up for early next week as I'm away on vacation the week after that.

bnoordhuis · 2015-08-14T14:47:28Z

common.gypi

+              '__LITTLE_ENDIAN=1234',
+              '__BIG_ENDIAN=4321',
+              '__BYTE_ORDER=__BIG_ENDIAN',
+              '__FLOAT_WORD_ORDER=__BIG_ENDIAN'],


What uses this? The only thing I can find in the source tree is openssl. Doesn't it make more sense to add it to openssl.gyp(i)?

Looks like moving it to openssl.gyp works. Done.

mhdawson · 2015-08-20T22:43:28Z

@ bnoordhuis first set of updates and a few questions added

mhdawson · 2015-08-21T20:38:54Z

@ bnoordhuis I've updated to address all of the comments/discussion. With this patch Node will compile for AIX 32 bit although 64 bit won't due to the removal of the gyp part. Still a good first step I think as it will get it compiling on 32 bit.

I also opened an issue to get the gyp part upstreamed here: https://code.google.com/p/gyp/issues/detail?id=495&q=AIX

While doing this I found the IBM'r who originally added the AIX support in gyp so I'm hoping he can help get the updates accepted in a reasonable timeframe.

I'm heading out on vacation for a week so if there are any additional comments to address I won't respond until then. If it looks ok and there are approvals when I get back I'll squash and land.

bnoordhuis · 2015-08-22T12:20:54Z

configure

+    matchup = {
+      '_ARCH_PPC'   : 'ppc',
+      '_ARCH_PPC64' : 'ppc64',
+    }


This is a no-op, isn't it? The second definition of matchup a few lines below overrides it again.

I assume you just mean the matchup part. Good point. Since I thought we needed those lines I it seemed like I'd gotten it right when I moved them from the original list (they were removed from earlier patches because alphabetizing had caused issues) . I'll validate that I can just remove them and AIX still builds ok.

mhdawson · 2015-09-02T14:53:16Z

FYI - pull request for gyp change - https://codereview.chromium.org/1319663007

mhdawson · 2015-09-02T15:21:23Z

@bnoordhuis confirmed that we don't need those lines and added commit to remove them.

mhdawson · 2015-09-05T00:07:05Z

I think this is ready to go, just waiting on a final l.g.t.m

rvagg · 2015-09-05T01:17:00Z

tools/js2c.py

@@ -310,7 +310,7 @@ def JS2C(source, target):
    else:
      ids.append((id, len(lines)))

-    escaped_id = id.replace('/', '$')
+    escaped_id = id.replace('/', '_')


what's up with this?

If I am not wrong this was discussed with @vkurchatkin before.

For posterity, it's mentioned in the commit log: sigils in identifiers are not supported on AIX.

rvagg · 2015-09-05T01:21:38Z

I'm clearly not qualified to review most of it but that's because most of it is isolated to AIX specific blocks, aside from the /dev/stdin, js2c.py, ENOTEMPTY and SIGLOST changes it seems to be nicely separate. I'd like to hear more about the js2c change and the ENOTEMPTY & SIGLOST seem to make sense even if AIX wasn't the only one where those cases held.

So, lgtm pending feedback on js2c and also a CI run as we need to verify that everything else still works.

/cc @nodejs/build

jbergstroem · 2015-09-07T03:21:12Z

I'm with @rvagg here. Problem also being I can't verify by running tests on an actual AIX platform.

mhdawson · 2015-09-10T23:48:46Z

Yup the js2c change was discussed here #2272 as noted in the initial description.

I've run on AIX as well as pLinux and xLinux locally, and of course would run through CI job before landing. CI job started here: https://ci.nodejs.org/job/node-test-pull-request/277/. Will check results when I'm back in the office next week.

mhdawson · 2015-09-14T14:29:03Z

CI run was clean except for freebsd101-32 which was failing in the prior CI runs at the time (ie for CI runs for other PRs run around that time).

@rvagg do you have any other questions in respect to the js2c change ?

bnoordhuis · 2015-09-14T14:34:10Z

configure

@@ -542,7 +546,10 @@ def is_arm_hard_float_abi():
 def host_arch_cc():
  """Host architecture check using the CC command."""

-  k = cc_macros()
+  if sys.platform.startswith('aix'):
+    k = cc_macros('gcc')


Can you add a comment here explaining why it hard-codes gcc?

bnoordhuis · 2015-09-14T14:39:20Z

LGTM with a request.

mhdawson · 2015-09-14T22:16:32Z

Incorporated comments from bnoordhuis and revalidated in local build, just hoping for final ok from @rvagg

rvagg · 2015-09-15T01:23:27Z

lgtm

These are the core changes that allow AIX to compile. There are still some test failures as there are some patches needed for libuv and npm that we'll need to contribute through those communities but this set allows node to be built on AIX and pass most of the core tests The change in js2c is because AIX does not support $ in identifier names. See the discussion/agreement in nodejs#2272 PR-URL: nodejs#2364 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

These are the core changes that allow AIX to compile. There are still some test failures as there are some patches needed for libuv and npm that we'll need to contribute through those communities but this set allows node to be built on AIX and pass most of the core tests The change in js2c is because AIX does not support $ in identifier names. See the discussion/agreement in #2272 PR-URL: #2364 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

Fishrock123 · 2015-09-15T17:20:07Z

This is landable on 4.x, right?

mhdawson · 2015-09-15T17:21:05Z

Landed as 2a17c7f

These are the core changes that allow AIX to compile. There are still some test failures as there are some patches needed for libuv and npm that we'll need to contribute through those communities but this set allows node to be built on AIX and pass most of the core tests The change in js2c is because AIX does not support $ in identifier names. See the discussion/agreement in #2272 PR-URL: #2364 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rod Vagg <[email protected]>

mhdawson · 2015-09-16T13:46:35Z

I want this to be landed on 4.X, should I put together a PR ?

mhdawson · 2015-09-17T23:17:00Z

Landed in 4.x now so closing

mhdawson added the build Issues and PRs related to build files or the CI. label Aug 12, 2015

bnoordhuis reviewed Aug 14, 2015
View reviewed changes

bnoordhuis reviewed Aug 22, 2015
View reviewed changes

rvagg reviewed Sep 5, 2015
View reviewed changes

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

mhdawson self-assigned this Sep 10, 2015

bnoordhuis reviewed Sep 14, 2015
View reviewed changes

brendanashworth mentioned this pull request Sep 15, 2015

Internal modules problem with $ in identifiers on AIX #2272

Closed

mhdawson force-pushed the aixchanges branch from ca40362 to 5a19293 Compare September 15, 2015 13:53

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 15, 2015

Fishrock123 mentioned this pull request Sep 15, 2015

Release proposal: v4.1.0 #2844

Closed

7 tasks

mhdawson added the land-on-v4.x label Sep 15, 2015

mhdawson closed this Sep 17, 2015

rvagg mentioned this pull request Sep 22, 2015

Release proposal: v4.1.1 #2995

Merged

jasnell added the lts-landed-on-v4.x label Oct 7, 2015

MylesBorins removed the land-on-v4.x label Nov 16, 2015

mhdawson deleted the aixchanges branch May 9, 2016 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Updates to enable AIX support #2364

build: Updates to enable AIX support #2364

mhdawson commented Aug 12, 2015

mhdawson commented Aug 12, 2015

jbergstroem commented Aug 12, 2015

mhdawson commented Aug 14, 2015

bnoordhuis Aug 14, 2015

mhdawson Aug 20, 2015

mhdawson commented Aug 20, 2015

mhdawson commented Aug 21, 2015

bnoordhuis Aug 22, 2015

mhdawson Sep 1, 2015

mhdawson commented Sep 2, 2015

mhdawson commented Sep 2, 2015

mhdawson commented Sep 5, 2015

rvagg Sep 5, 2015

thefourtheye Sep 7, 2015

bnoordhuis Sep 14, 2015

rvagg commented Sep 5, 2015

jbergstroem commented Sep 7, 2015

mhdawson commented Sep 10, 2015

mhdawson commented Sep 14, 2015

bnoordhuis Sep 14, 2015

bnoordhuis commented Sep 14, 2015

mhdawson commented Sep 14, 2015

rvagg commented Sep 15, 2015

Fishrock123 commented Sep 15, 2015

mhdawson commented Sep 15, 2015

mhdawson commented Sep 16, 2015

mhdawson commented Sep 17, 2015

build: Updates to enable AIX support #2364

build: Updates to enable AIX support #2364

Conversation

mhdawson commented Aug 12, 2015

mhdawson commented Aug 12, 2015

jbergstroem commented Aug 12, 2015

mhdawson commented Aug 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Aug 20, 2015

mhdawson commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Sep 2, 2015

mhdawson commented Sep 2, 2015

mhdawson commented Sep 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Sep 5, 2015

jbergstroem commented Sep 7, 2015

mhdawson commented Sep 10, 2015

mhdawson commented Sep 14, 2015

Choose a reason for hiding this comment

bnoordhuis commented Sep 14, 2015

mhdawson commented Sep 14, 2015

rvagg commented Sep 15, 2015

Fishrock123 commented Sep 15, 2015

mhdawson commented Sep 15, 2015

mhdawson commented Sep 16, 2015

mhdawson commented Sep 17, 2015