Add support for AIX #753

mhdawson · 2015-10-02T15:25:12Z

For AIX we need to use gmake

For AIX we need to set up the path to the exp file
which contains the symbols needed for linking.
The file will either be in one of the following
depeding on whether are are in installed or
development environment:

the include/node directory
the out/Release directory

The work to add the required node.exp file is being covered in this PR nodejs/node#3114

mhdawson · 2015-10-02T15:26:47Z

I've tested this in a build of Node from master. I've looked through the readme for node-gyp to see how changes are normally tested but I've not found any instructions. Can somebody point me at were it is documented what testing should be done ?

@bnoordhuis - as discussed can you take a look

rvagg · 2015-10-03T01:57:50Z

lib/build.js

@@ -21,6 +21,7 @@ exports.usage = 'Invokes `' + (win ? 'msbuild' : 'make') + '` and builds the mod
 function build (gyp, argv, callback) {
  var release = processRelease(argv, gyp, process.version, process.release)
    , makeCommand = gyp.opts.make || process.env.MAKE
+      || (process.platform.indexOf('aix') != -1 && 'gmake')


the mixed use of && and the ternary operator below makes this a bit confusing, the duplicate resolving of gmake suggests redundancy, and, shouldn't it just be aix when it's AIX? the indexOf is for the *bsd variants I think.

Can I suggest squashing both these lines to: ((/(^aix$)|bsd/).test(process.platform) ? 'gmake' : 'make')

Maybe platform-specific overrides are better done in parseArgv / parseOpts in lib/node-gyp.js.

it is kind of of ugly. What do you think of this. It's more verbose but also more obvious whats going on

function build (gyp, argv, callback) {
var platformMake = 'make';
if (process.platform == 'aix') {
platformMake = 'gmake';
} else if (process.platform.indexOf('bsd') != -1) {
platformMake = 'gmake');
}

var release = processRelease(argv, gyp, process.version, process.release)
, makeCommand = gyp.opts.make || process.env.MAKE || platformMake
, command = win ? 'msbuild' : makeCommand
, buildDir = path.resolve('build')
, configPath = path.resolve(buildDir, 'config.gypi')
, jobs = gyp.opts.jobs || process.env.JOBS
, buildType
, config
, arch
, nodeDir
, copyDevLib

rvagg · 2015-10-03T02:06:06Z

I'm pretty sure the findNodeRootDirectory() approach is wrong here and makes too many unsafe assumptions. Can you have a look at how it finds config.gypi and emulate that instead? I understand the problem of finding node.exp when using --nodedir but that's only a minor case, the most important one being finding the file in ~/.node-gyp/version/include/node/node.exp.

rvagg · 2015-10-03T02:09:38Z

re tests, there isn't much in the way of tests here. There are some docker tests you can run in test/docker.sh but obviously only for Linux but it's worth making sure you haven't broken anything else. Other testing has to be manual, which can be quite tricky when we haven't got any headers tarballs on nodejs.org that you can trial with! Once we get an AIX machine (or two, or three, whatever) in CI then we could produce tarballs for releases || nightlies with node.exp in it and test with that.

But, that raises another question, is node.exp specific to the arch used? Does it need a new one for BE, LE, 32-bit, 64-bit? If so, then it can't go into the headers tarball and you're going to need to scrap a lot of this code and emulate the node.lib download process we have for Windows and we're going to need to set up new directories in nodejs.org/download/ to contain node.exp for all of the different architectures.

rvagg · 2015-10-03T02:23:44Z

OK, another rethink on this, backing up on what I've just said I realise your approach now is to find node.exp as it's been installed on the system along with node from a binary tarball or some other official package, not necessarily have it in the headers or download from nodejs.org like node.lib. This is something that many of us have talked about for some time now wrt general headers because they are installed on many systems but node-gyp will always download them again anyway. A disparity that's existed since 0.10.

In that case, perhaps this is the right approach, but I personally don't want to see this logic baked in to node-gyp and would rather see it split off to a separate module that can locate the Node include dir on any system if it exists. That way it can be independently tested and iterated on rather than becoming part of the big logic puzzle that node-gyp currently is. I know that's not the expedient path here but as it is there's no clear way to test this logic in here. Perhaps a middle ground is to do what I did with process-release.js (which I would also like to extract to a separate package) and treat it as independent and test it independently.

bnoordhuis · 2015-10-03T10:40:43Z

lib/configure.js

+    if (process.platform == 'aix') {
+      var node_root_dir = findNodeRootDirectory();
+      node_exp_file = path.resolve(node_root_dir, 'include/node/node.exp')
+      if (!fs.existsSync(node_exp_file)) {


fs.existsSync() emits deprecation warnings.

Noted will replace with one of the suggested alternates

mhdawson · 2015-10-26T21:14:30Z

Sorry for taking so long to get back to this one. First part in Node is now in so will be moving on to this one soon. I am out at a conference this week but it will my priority after that.

mhdawson · 2015-11-06T23:45:34Z

Have updates to reflect the comments in my local repo, just need to test now

mhdawson · 2015-11-09T20:57:53Z

@bnoordhuis @rvagg updated to address review comments

bnoordhuis · 2015-11-13T11:06:15Z

lib/build.js

+  if (process.platform == 'aix') {
+    platformMake = 'gmake';
+  } else if (process.platform.indexOf('bsd') != -1) {
+    platformMake = 'gmake';


Style nits: no trailing semicolons (for better or worse)

EDIT: And maybe use strict inequality while you're here.

mhdawson · 2015-11-13T23:45:30Z

@bnoordhuis updated to address your latest comments. Added the unit tests and took a closer look as you had found a place were the code was obviously stale. I fixed up the initial checks to be for 'node_modules' and 'deps' and validated directories on windows and fixed a few issues there as well. I've kicked off tests on my local system and of course once we have agreement I'll do a full CI run before landing.

bnoordhuis · 2015-11-14T12:16:57Z

test/test-find-node-directory.js

+    t.equal(
+      findNodeDirectory('/y/lib/node_modules/npm/node_modules/node-gyp/lib',
+                         processObj), path.join('/y'));
+  }


Can you test both branches here? I.e., rewrite it so it's called once with processObj.platform === 'win32' and once without?

I'd recommend doing that everywhere where you call findNodeDirectory() with { platform: process.platform }. The idea is that all code paths are tested on all platforms.

bnoordhuis · 2015-11-14T12:22:50Z

test/test-find-node-directory.js

+  var processObj = {execPath: '/x/y/bin/node', platform: process.platform}
+  t.equal(
+    findNodeDirectory('/x/deps/npm/node_modules/node-gyp/lib', processObj),
+                      path.join('/x'));


There are a few stray semicolons in this file.

Thanks its hard to stomp out that habit.

mhdawson · 2015-11-16T16:40:29Z

@bnoordhuis @thefourtheye updated to address latest comments

bnoordhuis · 2015-11-16T17:29:35Z

LGTM. @thefourtheye WDYT?

thefourtheye · 2015-11-17T01:00:58Z

lib/configure.js

+    // for AIX we need to set up the path to the exp file
+    // which contains the symbols needed for linking.
+    // The file will either be in one of the following
+    // depeding on whether are are in installed or


depending on whether it is

thefourtheye · 2015-11-17T01:21:40Z

Have few minor comments but overall change LGTM

mhdawson · 2015-11-17T14:52:27Z

@thefourtheye addressed your comments. If it looks ok to you I can squash into one commit

thefourtheye · 2015-11-18T02:39:39Z

lib/configure.js

+           fs.accessSync(node_exp_file, fs.R_OK)
+           // exp file found, stop looking
+           break
+         } catch (exception) {


If the file is not found anywhere, node_exp_file will still be node.exp at the end of the loop. Is that okay or you want to reset to empty string here?

I think its ok as if this does occur I don't think its going to be any clearer what the problem is with it set to an empty string.

Cool, let's land it then. LGTM

For AIX we need to use gmake For AIX we need to set up the path to the exp file which contains the symbols needed for linking. The file will either be in one of the following depeding on whether are are in installed or development environment: - the include/node directory - the out/Release directory

mhdawson · 2015-11-18T17:22:11Z

Squashed down to a single commit

For AIX we need to use gmake. For AIX we need to set up the path to the exp file which contains the symbols needed for linking. The file will either be in one of the following depeding on whether are are in installed or development environment: - the include/node directory - the out/Release directory PR-URL: #753 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

bnoordhuis · 2015-11-18T17:26:02Z

Thanks Michael, landed in 9bfa087.

rvagg reviewed Oct 3, 2015
View reviewed changes

bnoordhuis reviewed Oct 3, 2015
View reviewed changes

bnoordhuis mentioned this pull request Oct 19, 2015

use standard cp arguments #571

Closed

bnoordhuis reviewed Nov 13, 2015
View reviewed changes

bnoordhuis reviewed Nov 14, 2015
View reviewed changes

thefourtheye reviewed Nov 17, 2015
View reviewed changes

thefourtheye reviewed Nov 18, 2015
View reviewed changes

mhdawson force-pushed the aix1 branch from 7719c39 to 2ab257a Compare November 18, 2015 17:21

bnoordhuis closed this Nov 18, 2015

richardlau mentioned this pull request Jun 13, 2016

Replace fs.accessSync calls #955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AIX #753

Add support for AIX #753

mhdawson commented Oct 2, 2015

mhdawson commented Oct 2, 2015

rvagg Oct 3, 2015

bnoordhuis Oct 3, 2015

mhdawson Nov 6, 2015

rvagg commented Oct 3, 2015

rvagg commented Oct 3, 2015

rvagg commented Oct 3, 2015

bnoordhuis Oct 3, 2015

mhdawson Nov 6, 2015

mhdawson commented Oct 26, 2015

mhdawson commented Nov 6, 2015

mhdawson commented Nov 9, 2015

bnoordhuis Nov 13, 2015

mhdawson Nov 13, 2015

mhdawson commented Nov 13, 2015

bnoordhuis Nov 14, 2015

bnoordhuis Nov 14, 2015

bnoordhuis Nov 14, 2015

mhdawson Nov 16, 2015

mhdawson commented Nov 16, 2015

bnoordhuis commented Nov 16, 2015

thefourtheye Nov 17, 2015

mhdawson Nov 17, 2015

thefourtheye commented Nov 17, 2015

mhdawson commented Nov 17, 2015

thefourtheye Nov 18, 2015

mhdawson Nov 18, 2015

thefourtheye Nov 18, 2015

mhdawson commented Nov 18, 2015

bnoordhuis commented Nov 18, 2015

Add support for AIX #753

Add support for AIX #753

Conversation

mhdawson commented Oct 2, 2015

mhdawson commented Oct 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Oct 3, 2015

rvagg commented Oct 3, 2015

rvagg commented Oct 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Oct 26, 2015

mhdawson commented Nov 6, 2015

mhdawson commented Nov 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Nov 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Nov 16, 2015

bnoordhuis commented Nov 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Nov 17, 2015

mhdawson commented Nov 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Nov 18, 2015

bnoordhuis commented Nov 18, 2015