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 support for AIX #753

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions addon.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@
'DYLIB_INSTALL_NAME_BASE': '@rpath'
},
}],
[ 'OS=="aix"', {
'ldflags': [
'-Wl,-bimport:<(node_exp_file)'
],
}],
[ 'OS=="win"', {
'libraries': [
'-lkernel32.lib',
Expand Down
10 changes: 8 additions & 2 deletions lib/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@ var fs = require('graceful-fs')
exports.usage = 'Invokes `' + (win ? 'msbuild' : 'make') + '` and builds the module'

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
|| (process.platform.indexOf('bsd') != -1 ? 'gmake' : 'make')
, makeCommand = gyp.opts.make || process.env.MAKE || platformMake
, command = win ? 'msbuild' : makeCommand
, buildDir = path.resolve('build')
, configPath = path.resolve(buildDir, 'config.gypi')
Expand Down
32 changes: 32 additions & 0 deletions lib/configure.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var fs = require('graceful-fs')
, spawn = cp.spawn
, execFile = cp.execFile
, win = process.platform == 'win32'
, findNodeDirectory = require('./find-node-directory')

exports.usage = 'Generates ' + (win ? 'MSVC project files' : 'a Makefile') + ' for the current module'

Expand Down Expand Up @@ -299,6 +300,34 @@ function configure (gyp, argv, callback) {
argv.push('-I', config)
})

// 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
// depending on whether it is an installed or
// development environment:
// - the include/node directory
// - the out/Release directory
// - the out/Debug directory
// - the root directory
var node_exp_file = ''
if (process.platform === 'aix') {
var node_root_dir = findNodeDirectory()
var candidates = ['include/node/node.exp',
'out/Release/node.exp',
'out/Debug/node.exp',
'node.exp']
for (var next = 0; next < candidates.length; next++) {
node_exp_file = path.resolve(node_root_dir, candidates[next])
try {
fs.accessSync(node_exp_file, fs.R_OK)
// exp file found, stop looking
break
} catch (exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, let's land it then. LGTM

// this candidate was not found or not readable, do nothing
}
}
}

// this logic ported from the old `gyp_addon` python file
var gyp_script = path.resolve(__dirname, '..', 'gyp', 'gyp_main.py')
var addon_gypi = path.resolve(__dirname, '..', 'addon.gypi')
Expand All @@ -319,6 +348,9 @@ function configure (gyp, argv, callback) {
argv.push('-Dlibrary=shared_library')
argv.push('-Dvisibility=default')
argv.push('-Dnode_root_dir=' + nodeDir)
if (process.platform === 'aix') {
argv.push('-Dnode_exp_file=' + node_exp_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the file is not found, should we still add this flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer that we still add the flag so that we can see what occurred. In verbose mode we'll be able to see what the value for -Dnode_exp_file is

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay then

}
argv.push('-Dnode_gyp_dir=' + nodeGypDir)
argv.push('-Dnode_lib_file=' + release.name + '.lib')
argv.push('-Dmodule_root_dir=' + process.cwd())
Expand Down
61 changes: 61 additions & 0 deletions lib/find-node-directory.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
var path = require('path')
, log = require('npmlog')

function findNodeDirectory(scriptLocation, processObj) {
// set dirname and process if not passed in
// this facilitates regression tests
if (scriptLocation === undefined) {
scriptLocation = __dirname
}
if (processObj === undefined) {
processObj = process
}

// Have a look to see what is above us, to try and work out where we are
npm_parent_directory = path.join(scriptLocation, '../../../..')
log.verbose('node-gyp root', 'npm_parent_directory is '
+ path.basename(npm_parent_directory))
node_root_dir = ""

log.verbose('node-gyp root', 'Finding node root directory')
if (path.basename(npm_parent_directory) === 'deps') {
// We are in a build directory where this script lives in
// deps/npm/node_modules/node-gyp/lib
node_root_dir = path.join(npm_parent_directory, '..')
log.verbose('node-gyp root', 'in build directory, root = '
+ node_root_dir)
} else if (path.basename(npm_parent_directory) === 'node_modules') {
// We are in a node install directory where this script lives in
// lib/node_modules/npm/node_modules/node-gyp/lib or
// node_modules/npm/node_modules/node-gyp/lib depending on the
// platform
if (processObj.platform === 'win32') {
node_root_dir = path.join(npm_parent_directory, '..')
} else {
node_root_dir = path.join(npm_parent_directory, '../..')
}
log.verbose('node-gyp root', 'in install directory, root = '
+ node_root_dir)
} else {
// We don't know where we are, try working it out from the location
// of the node binary
var node_dir = path.dirname(processObj.execPath)
var directory_up = path.basename(node_dir)
if (directory_up === 'bin') {
node_root_dir = path.join(node_dir, '..')
} else if (directory_up === 'Release' || directory_up === 'Debug') {
// If we are a recently built node, and the directory structure
// is that of a repository. If we are on Windows then we only need
// to go one level up, everything else, two
if (processObj.platform === 'win32') {
node_root_dir = path.join(node_dir, '..')
} else {
node_root_dir = path.join(node_dir, '../..')
}
}
// Else return the default blank, "".
}
return node_root_dir
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a few regression tests for this function? I'd make it so that you can plug in a mock __dirname and process object.


module.exports = findNodeDirectory
115 changes: 115 additions & 0 deletions test/test-find-node-directory.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
var test = require('tape')
var path = require('path')
var findNodeDirectory = require('../lib/find-node-directory')

var platforms = ['darwin', 'freebsd', 'linux', 'sunos', 'win32', 'aix']

// we should find the directory based on the directory
// the script is running in and it should match the layout
// in a build tree where npm is installed in
// .... /deps/npm
test('test find-node-directory - node install', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj = {execPath: '/x/y/bin/node', platform: platforms[next]}
t.equal(
findNodeDirectory('/x/deps/npm/node_modules/node-gyp/lib', processObj),
path.join('/x'))
Copy link
Contributor

Choose a reason for hiding this comment

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

path.join is used all over the tests but it actually doesn't join paths. Am I totally missing the point of its usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

without the join the patch ends up being incorrect on windows, the join fixes up the '/' to be appropriate for the platform

Copy link
Contributor

Choose a reason for hiding this comment

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

But you are hardcoding them right? I need to check if the join changes the slashes

}
})

// we should find the directory based on the directory
// the script is running in and it should match the layout
// in an installed tree where npm is installed in
// .... /lib/node_modules/npm or .../node_modules/npm
// depending on the patform
test('test find-node-directory - node build', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj = {execPath: '/x/y/bin/node', platform: platforms[next]}
if (platforms[next] === 'win32') {
t.equal(
findNodeDirectory('/y/node_modules/npm/node_modules/node-gyp/lib',
processObj), path.join('/y'))
} else {
t.equal(
findNodeDirectory('/y/lib/node_modules/npm/node_modules/node-gyp/lib',
processObj), path.join('/y'))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

})

// we should find the directory based on the execPath
// for node and match because it was in the bin directory
test('test find-node-directory - node in bin directory', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj = {execPath: '/x/y/bin/node', platform: platforms[next]}
t.equal(
findNodeDirectory('/nothere/npm/node_modules/node-gyp/lib', processObj),
path.join('/x/y'))
}
})

// we should find the directory based on the execPath
// for node and match because it was in the Release directory
test('test find-node-directory - node in build release dir', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj
if (platforms[next] === 'win32') {
processObj = {execPath: '/x/y/Release/node', platform: platforms[next]}
} else {
processObj = {execPath: '/x/y/out/Release/node',
platform: platforms[next]}
}

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

// we should find the directory based on the execPath
// for node and match because it was in the Debug directory
test('test find-node-directory - node in Debug release dir', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj
if (platforms[next] === 'win32') {
processObj = {execPath: '/a/b/Debug/node', platform: platforms[next]}
} else {
processObj = {execPath: '/a/b/out/Debug/node', platform: platforms[next]}
}

t.equal(
findNodeDirectory('/nothere/npm/node_modules/node-gyp/lib', processObj),
path.join('/a/b'))
}
})

// we should not find it as it will not match based on the execPath nor
// the directory from which the script is running
test('test find-node-directory - not found', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj = {execPath: '/x/y/z/y', platform:next}
t.equal(findNodeDirectory('/a/b/c/d', processObj), '')
}
})

// we should find the directory based on the directory
// the script is running in and it should match the layout
// in a build tree where npm is installed in
// .... /deps/npm
// same test as above but make sure additional directory entries
// don't cause an issue
test('test find-node-directory - node install', function (t) {
t.plan(platforms.length)
for (var next = 0; next < platforms.length; next++) {
var processObj = {execPath: '/x/y/bin/node', platform: platforms[next]}
t.equal(
findNodeDirectory('/x/y/z/a/b/c/deps/npm/node_modules/node-gyp/lib',
processObj), path.join('/x/y/z/a/b/c'))
}
})