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

Replace fs.accessSync calls #955

Closed
wants to merge 2 commits 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
46 changes: 35 additions & 11 deletions lib/configure.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = exports = configure
module.exports.test = { findPython: findPython }
module.exports.test = { findAccessibleSync: findAccessibleSync,
findPython: findPython }

/**
* Module dependencies.
Expand All @@ -20,6 +21,7 @@ var fs = require('graceful-fs')
, execFile = cp.execFile
, win = process.platform == 'win32'
, findNodeDirectory = require('./find-node-directory')
, msgFormat = require('util').format

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

Expand Down Expand Up @@ -226,22 +228,21 @@ function configure (gyp, argv, callback) {
// - the out/Release directory
// - the out/Debug directory
// - the root directory
var node_exp_file = ''
var node_exp_file = undefined
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) {
// this candidate was not found or not readable, do nothing
}
var logprefix = 'find exports file'
node_exp_file = findAccessibleSync(logprefix, node_root_dir, candidates)
if (node_exp_file !== undefined) {
log.verbose(logprefix, 'Found exports file: %s', node_exp_file)
} else {
var msg = msgFormat('Could not find node.exp file in %s', node_root_dir)
log.error(logprefix, 'Could not find exports file')
return callback(new Error(msg))
}
}
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 break out this block into a separate function?

Also, the try body should be smaller. It should basically just be the fs.statSync() call, nothing more, else there is too much space for bugs to hide.

Copy link
Member Author

@richardlau richardlau Jun 17, 2016

Choose a reason for hiding this comment

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

Yes, okay will do.


Expand Down Expand Up @@ -310,6 +311,29 @@ function configure (gyp, argv, callback) {

}

/**
* Returns the first file or directory from an array of candidates that is
* readable by the current user, or undefined if none of the candidates are
* readable.
*/
function findAccessibleSync (logprefix, dir, candidates) {
for (var next = 0; next < candidates.length; next++) {
var candidate = path.resolve(dir, candidates[next])
try {
var fd = fs.openSync(candidate, 'r')
} catch (e) {
// this candidate was not found or not readable, do nothing
log.silly(logprefix, 'Could not open %s: %s', candidate, e.message)
continue
}
fs.closeSync(fd)
log.silly(logprefix, 'Found readable %s', candidate)
return candidate
}

return undefined
}
Copy link
Member

@bnoordhuis bnoordhuis Jun 17, 2016

Choose a reason for hiding this comment

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

Question: if the sole purpose is to find out if the file can be read from, why don't you try to open it in read-only mode? It's shorter and less error-prone, IMO.

for (var i = 0; i < candidates.length; i += 1) {
  var filename = path.resolve(candidates[i])
  try {
    var fd = fs.openSync(filename, 'r')
  } catch (e) {
    continue
  }
  fs.closeSync(fd)
  return filename
}

EDIT: That will also Just Work on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point, that's much more concise. Should it still be broken out into its own function, or in-lined as it was originally with the fs.accessSync call?

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 keep it in a separate function, easier to test in isolation.


function findPython (python, callback) {
checkPython()

Expand Down
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@
"node": ">= 0.8.0"
},
"devDependencies": {
"tape": "~4.2.0"
"tape": "~4.2.0",
"bindings": "~1.2.1",
"nan": "^2.0.0",
"require-inject": "~1.3.0"
},
"scripts": {
"test": "tape test/test-*"
Expand Down
11 changes: 11 additions & 0 deletions test/node_modules/hello_world/binding.gyp

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions test/node_modules/hello_world/hello.cc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/node_modules/hello_world/hello.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions test/node_modules/hello_world/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions test/test-addon.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict'

var test = require('tape')
var execFile = require('child_process').execFile
var path = require('path')
var addonPath = path.resolve(__dirname, 'node_modules', 'hello_world')
var nodeGyp = path.resolve(__dirname, '..', 'bin', 'node-gyp.js')

test('build simple addon', function (t) {
t.plan(3)

// Set the loglevel otherwise the output disappears when run via 'npm test'
var cmd = [nodeGyp, 'rebuild', '-C', addonPath, '--loglevel=verbose']
var proc = execFile(process.execPath, cmd, function (err, stdout, stderr) {
var logLines = stderr.toString().trim().split(/\r?\n/)
var lastLine = logLines[logLines.length-1]
t.strictEqual(err, null)
t.strictEqual(lastLine, 'gyp info ok', 'should end in ok')
try {
var binding = require('hello_world')
t.strictEqual(binding.hello(), 'world')
} catch (error) {
t.error(error, 'load module')
}
})
proc.stdout.setEncoding('utf-8')
proc.stderr.setEncoding('utf-8')
})
86 changes: 86 additions & 0 deletions test/test-find-accessible-sync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
'use strict'

var test = require('tape')
var path = require('path')
var requireInject = require('require-inject')
var configure = requireInject('../lib/configure', {
'graceful-fs': {
'closeSync': function (fd) { return undefined },
'openSync': function (path) {
if (readableFiles.some(function (f) { return f === path} )) {
return 0
} else {
var error = new Error('ENOENT - not found')
throw error
}
}
}
})

var dir = path.sep + 'testdir'
var readableFile = 'readable_file'
var anotherReadableFile = 'another_readable_file'
var readableFileInDir = 'somedir' + path.sep + readableFile
var readableFiles = [
path.resolve(dir, readableFile),
path.resolve(dir, anotherReadableFile),
path.resolve(dir, readableFileInDir)
]

test('find accessible - empty array', function (t) {
t.plan(1)

var candidates = []
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, undefined)
})

test('find accessible - single item array, readable', function (t) {
t.plan(1)

var candidates = [ readableFile ]
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, path.resolve(dir, readableFile))
})

test('find accessible - single item array, readable in subdir', function (t) {
t.plan(1)

var candidates = [ readableFileInDir ]
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, path.resolve(dir, readableFileInDir))
})

test('find accessible - single item array, unreadable', function (t) {
t.plan(1)

var candidates = [ 'unreadable_file' ]
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, undefined)
})


test('find accessible - multi item array, no matches', function (t) {
t.plan(1)

var candidates = [ 'non_existent_file', 'unreadable_file' ]
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, undefined)
})


test('find accessible - multi item array, single match', function (t) {
t.plan(1)

var candidates = [ 'non_existent_file', readableFile ]
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, path.resolve(dir, readableFile))
})

test('find accessible - multi item array, return first match', function (t) {
t.plan(1)

var candidates = [ 'non_existent_file', anotherReadableFile, readableFile ]
var found = configure.test.findAccessibleSync('test', dir, candidates)
t.strictEqual(found, path.resolve(dir, anotherReadableFile))
})