-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
|
@@ -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' | ||
|
||
|
@@ -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)) | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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') | ||
}) |
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)) | ||
}) |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, okay will do.