Skip to content

Commit

Permalink
Address third set of review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
mhdawson committed Nov 16, 2015
1 parent 2319a7d commit a742444
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 52 deletions.
2 changes: 1 addition & 1 deletion lib/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function build (gyp, argv, callback) {
var platformMake = 'make'
if (process.platform === 'aix') {
platformMake = 'gmake'
} else if (process.platform.indexOf('bsd') != -1) {
} else if (process.platform.indexOf('bsd') !== -1) {
platformMake = 'gmake'
}

Expand Down
2 changes: 1 addition & 1 deletion lib/configure.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ function configure (gyp, argv, callback) {
fs.accessSync(node_exp_file, fs.R_OK)
// exp file found, stop looking
break
} catch(exception) {
} catch (exception) {
// this candidate was not found or not readable, do nothing
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/find-node-directory.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ function findNodeDirectory(scriptLocation, processObj) {
node_root_dir = path.join(npm_parent_directory, '../..')
}
log.verbose('node-gyp root', 'in install directory, root = '
+ node_root_dir)
+ node_root_dir)
} else {
// We don't know where we are, try working it out from the location
// of the node binary
Expand Down
115 changes: 66 additions & 49 deletions test/test-find-node-directory.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,85 +2,100 @@ 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(1)
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'));
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj = {execPath: '/x/y/bin/node', platform: currentPlatform}
t.equal(
findNodeDirectory('/x/deps/npm/node_modules/node-gyp/lib', processObj),
path.join('/x'))
}
})

// we should find the directory based on the directory
// the script is running in and it should match the layout
// in a installed tree where npm is installed in
// .... /lib/node_modules/npm or .../node_modules/npm
// dependingo on the patform
// depending on the patform
test('test find-node-directory - node build', function (t) {
t.plan(1)
var processObj = {execPath: '/x/y/bin/node', platform: process.platform}
if (process.platform === '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'));
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj = {execPath: '/x/y/bin/node', platform: currentPlatform}
if (currentPlatform === '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'))
}
}
})

// 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(1)
var processObj = {execPath: '/x/y/bin/node', platform: process.platform}
t.equal(
findNodeDirectory('/nothere/npm/node_modules/node-gyp/lib', processObj),
path.join('/x/y'));
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj = {execPath: '/x/y/bin/node', platform: currentPlatform}
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(1)
var processObj;
if (process.platform === 'win32') {
processObj = {execPath: '/x/y/Release/node', platform: process.platform}
} else {
processObj = {execPath: '/x/y/out/Release/node', platform: process.platform}
}
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj
if (currentPlatform === 'win32') {
processObj = {execPath: '/x/y/Release/node', platform: currentPlatform}
} else {
processObj = {execPath: '/x/y/out/Release/node',
platform: currentPlatform}
}

t.equal(
findNodeDirectory('/nothere/npm/node_modules/node-gyp/lib', processObj),
path.join('/x/y'));
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(1)
var processObj;
if (process.platform === 'win32') {
processObj = {execPath: '/a/b/Debug/node', platform: process.platform}
} else {
processObj = {execPath: '/a/b/out/Debug/node', platform: process.platform}
}
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj
if (currentPlatform === 'win32') {
processObj = {execPath: '/a/b/Debug/node', platform: currentPlatform}
} else {
processObj = {execPath: '/a/b/out/Debug/node', platform: currentPlatform}
}

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

// we shoudl 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(1)
var processObj = {execPath: '/x/y/z/y', platform: process.platform}
t.equal(findNodeDirectory('/a/b/c/d', processObj), '');
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj = {execPath: '/x/y/z/y', platform:currentPlatform}
t.equal(findNodeDirectory('/a/b/c/d', processObj), '')
}
})

// we should find the directory based on the directory
Expand All @@ -90,9 +105,11 @@ test('test find-node-directory - not found', function (t) {
// 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(1)
var processObj = {execPath: '/x/y/bin/node', platform: process.platform}
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'));
t.plan(platforms.length)
for (currentPlatform in platforms) {
var processObj = {execPath: '/x/y/bin/node', platform: currentPlatform}
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'))
}
})

0 comments on commit a742444

Please sign in to comment.