Replace fs.accessSync calls #955

richardlau · 2016-06-13T16:24:26Z

9bfa087 (#753) added calls to fs.accessSync on AIX to test if the candidate exports file exists and is readable. Unfortunately accessSync does not exist in Node 0.10.x, so the exports file is not found and results in a linking error:

ld: 0706-003 Cannot find or read import file: /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/node.exp
        ld:accessx(): A file or directory in the path name does not exist.
collect2: ld returned 255 exit status
gmake: *** [Release/obj.target/hello.node] Error 1
gyp ERR! build error
gyp ERR! stack Error: `gmake` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/home/users/riclau/sandbox/github/node-gyp/lib/build.js:276:23)
gyp ERR! stack     at ChildProcess.emit (events.js:98:17)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (child_process.js:820:12)
gyp ERR! System AIX 1

richardlau · 2016-06-13T16:26:41Z

I've replaced the call to fs.accessSync with fs.statSync since that does exist on Node 0.10.x (and above) and the alternative fs.existsSync is deprecated on later versions of Node.

There also didn't appear to be an existing test that failed because of this, so I've added one in a separate commit.

richardlau · 2016-06-13T16:31:42Z

cc @mhdawson

mhdawson · 2016-06-13T17:53:07Z

Is this page wrong

https://nodejs.org/api/fs.html#fs_fs_fstat_fd_callback

It would seem to say that fs.accessSync was added in v0.1.93 ?

The doc is also not all that clear for fs.fstatSync. Would it through if the file existed by was not readable to the current user ?

richardlau · 2016-06-13T18:07:59Z

Yes, looks like the page you have linked to is incorrect, there's no accessSync in 0.10.x, e.g. https://nodejs.org/dist/v0.10.45/docs/api/fs.html.

Just tried removing read permissions from the exports file -- fstatSync doesn't throw (so the candidate is chosen) but the subsequent gyp invocation results in a similar access error as originally:

-bash-4.2$ chmod u-r /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/include/node/node.exp
-bash-4.2$ tape test/test-addon.js
TAP version 13
# build simple addon
not ok 1 should be equal
  ---
    operator: equal
    expected: |-
      null
    actual: |-
      { [Error: Command failed: gyp info it worked if it ends with ok
...
ld: 0706-003 Cannot find or read import file: /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/include/node/node.exp
        ld:accessx(): The file access permissions do not allow the specified action.
collect2: ld returned 255 exit status
gmake: *** [Release/obj.target/hello.node] Error 1
gyp ERR! build error
gyp ERR! stack Error: `gmake` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/home/users/riclau/sandbox/github/node-gyp/lib/build.js:276:23)
gyp ERR! stack     at ChildProcess.emit (events.js:98:17)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (child_process.js:820:12)
gyp ERR! System AIX 1
...

This is better than the original case, because the file pointed to is there on the filesystem but with insufficient access permissions (in the original the code fell through to the last tried candidate location which turned out to not exist).

Flarna · 2016-06-13T18:22:16Z

I think you have to check the mode field of the stat result for read permissions to reach the same result as with access. Or you could try to read at least a part of the file.

mhdawson · 2016-06-13T18:51:36Z

Right unless we find a better check we'll be making it worse for versions other than 0.10.X. Can you look into how we can achieve a check which is closer to the original.

bnoordhuis · 2016-06-14T07:43:56Z

test/node_modules/hello_world/hello.js

@@ -0,0 +1,3 @@
+var addon = require('bindings')('hello');
+exports.hello = function() { return addon.hello() }
+


Can you drop the blank line and add 'use strict' at the top?

gibfahn · 2016-06-14T18:37:30Z

@mhdawson I would guess that the easiest solution would be to use fs.statSync if fs.accessSync is undefined (i.e. in 0.10). That way we'd be the same for 0.12 and up, and for 0.10 there'd only be a problem if someone didn't have read permissions on a node.exp file in their node directory on AIX.

Would that make sense?

Presumably the alternative is for @richardlau to implement something that mimics the functionality of fs.accessSync on v0.10. Given that fs.accessSync wasn't backported to 0.10, I'd suggest that this is overkill.

gibfahn · 2016-06-14T18:38:06Z

@bnoordhuis is there currently a test in node-gyp that builds a module?

richardlau · 2016-06-14T19:43:52Z

Updated the test commit to address comments from @bnoordhuis. I found another line that was over 80 characters so have shortened that one (via another var) as well.

Also updated the fix so that it checks the mode based on the uid and gid of the file being tested to check if the file is readable based on comments from @mhdawson and @Flarna. I also decided to add extra logging to this area, including one at log level error if the code could not find a suitable exports file to use.

richardlau · 2016-06-14T23:01:16Z

lib/configure.js

         } catch (exception) {
           // this candidate was not found or not readable, do nothing
+           log.silly(node_exp_file, 'does not exist')


Missed this, should be using prefix like other log messages. Will fix tomorrow.

bnoordhuis · 2016-06-17T11:50:42Z

lib/configure.js

+        log.verbose(prefix, 'Found exports file: %s', node_exp_file)
+      } else {
+        log.error(prefix, 'Could not find exports file')
+      }
    }


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.

Yes, okay will do.

richardlau · 2016-06-17T15:01:37Z

Extracted function as requested by @bnoordhuis. Added tests for extracted function.

I've also made the code bail out with an error if it can't find a readable exports file (before it would pass the last tried candidate on to gyp even though we know it isn't readable). CC @mhdawson

bnoordhuis · 2016-06-17T16:13:16Z

lib/configure.js

+  }
+
+  return found
+}


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.

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?

I'd keep it in a separate function, easier to test in isolation.

richardlau · 2016-06-17T17:23:45Z

Code and tests reworked to use fs.openSync. Verified the tests pass on Windows, Linux (x64) and AIX with Node v0.10.45.

fs.accessSync does not exist in Node 0.10.x.

richardlau · 2016-06-17T17:38:24Z

So with the current version of the changes, if I sabotage my AIX Node installation, chmod u-r /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/include/node/node.exp, I get:

-bash-4.2$ /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/bin/node  "/home/users/riclau/sandbox/github/node-gyp/bin/node-gyp.js" rebuild -C /home/users/riclau/sandbox/github/node-gyp/test/node_modules/hello_world
gyp info it worked if it ends with ok
gyp info using node-gyp@3.3.1
gyp info using node@0.10.45 | aix | ppc64
gyp info chdir /home/users/riclau/sandbox/github/node-gyp/test/node_modules/hello_world
gyp ERR! find exports file Could not find exports file
gyp ERR! configure error
gyp ERR! stack Error: Could not find node.exp file in /tmp/usenode.riclau/node-v0.10.45-aix-ppc64
gyp ERR! stack     at runGyp (/home/users/riclau/sandbox/github/node-gyp/lib/configure.js:245:25)
gyp ERR! stack     at findConfigs (/home/users/riclau/sandbox/github/node-gyp/lib/configure.js:170:23)
gyp ERR! stack     at /home/users/riclau/sandbox/github/node-gyp/lib/configure.js:176:11
gyp ERR! stack     at Object.oncomplete (fs.js:108:15)
gyp ERR! System AIX 1
gyp ERR! command "/tmp/usenode.riclau/node-v0.10.45-aix-ppc64/bin/node" "/home/users/riclau/sandbox/github/node-gyp/bin/node-gyp.js" "rebuild" "-C" "/home/users/riclau/sandbox/github/node-gyp/test/node_modules/hello_world"
gyp ERR! cwd /home/users/riclau/sandbox/github/node-gyp/test/node_modules/hello_world
gyp ERR! node -v v0.10.45
gyp ERR! node-gyp -v v3.3.1
gyp ERR! not ok
-bash-4.2$

With --loglevel=silly, we get these extra diagnostics:

gyp sill find exports file Could not open /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/include/node/node.exp: EACCES, permission denied '/tmp/usenode.riclau/node-v0.10.45-aix-ppc64/include/node/node.exp'
gyp sill find exports file Could not open /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/out/Release/node.exp: ENOENT, no such file or directory '/tmp/usenode.riclau/node-v0.10.45-aix-ppc64/out/Release/node.exp'
gyp sill find exports file Could not open /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/out/Debug/node.exp: ENOENT, no such file or directory '/tmp/usenode.riclau/node-v0.10.45-aix-ppc64/out/Debug/node.exp'
gyp sill find exports file Could not open /tmp/usenode.riclau/node-v0.10.45-aix-ppc64/node.exp: ENOENT, no such file or directory '/tmp/usenode.riclau/node-v0.10.45-aix-ppc64/node.exp'
gyp ERR! find exports file Could not find exports file

PR-URL: #955 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

fs.accessSync does not exist in Node 0.10.x. PR-URL: #955 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis · 2016-06-17T18:01:26Z

Thanks Richard, LGTM. Landed in c4344b3...77383dd.

mhdawson · 2016-06-17T19:05:58Z

A bit late I guess but LGTM from me as well

@rvagg

Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13199 Reviewed-By: @zkat

@rvagg

Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13200 Reviewed-By: @zkat

@rvagg

Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13199 Reviewed-By: @zkat

@rvagg

Headline items: * Fix for Visual Studio 2015 __pfnDliNotifyHook2 failures nodejs/node-gyp#952 * Fix for AIX using fs.statSync nodejs/node-gyp#955 * More verbose msbuild.exe missing error output https://github.com/nodejs/node-gyp/pull/930/files * Added --silent for --loglevel=silent nodejs/node-gyp#937 * Enable V8 deprecation warnings for native addons nodejs/node-gyp#920 Full changelog at https://github.com/nodejs/node-gyp/blob/master/CHANGELOG.md Credit: @rvagg PR-URL: #13199 Reviewed-By: @zkat

bnoordhuis reviewed Jun 14, 2016
View reviewed changes

bnoordhuis mentioned this pull request Jun 14, 2016

Propose v3.3.2 #956

Closed

richardlau force-pushed the test-addons branch from dd873ba to fd74f48 Compare June 14, 2016 18:10

richardlau force-pushed the test-addons branch from fd74f48 to b6a53f8 Compare June 14, 2016 19:36

richardlau mentioned this pull request Jun 14, 2016

doc: Correct added: information for fs.access and fs.accessSync nodejs/node#7299

Closed

2 tasks

richardlau reviewed Jun 14, 2016
View reviewed changes

richardlau force-pushed the test-addons branch 3 times, most recently from dbb775c to b17eea6 Compare June 15, 2016 18:37

bnoordhuis reviewed Jun 17, 2016
View reviewed changes

test: Add simple addon test

dc89fea

richardlau force-pushed the test-addons branch 4 times, most recently from d7a5a69 to efedde8 Compare June 17, 2016 14:55

bnoordhuis reviewed Jun 17, 2016
View reviewed changes

richardlau force-pushed the test-addons branch from efedde8 to 0da4ca9 Compare June 17, 2016 17:15

Replace fs.accessSync call to fs.statSync

e09a7e9

fs.accessSync does not exist in Node 0.10.x.

richardlau force-pushed the test-addons branch from 0da4ca9 to e09a7e9 Compare June 17, 2016 17:26

bnoordhuis pushed a commit that referenced this pull request Jun 17, 2016

test: add simple addon test

0dba4bd

PR-URL: #955 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis pushed a commit that referenced this pull request Jun 17, 2016

Replace fs.accessSync call to fs.statSync

77383dd

fs.accessSync does not exist in Node 0.10.x. PR-URL: #955 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis closed this Jun 17, 2016

rvagg mentioned this pull request Jun 28, 2016

deps: upgrade node-gyp to v3.4.0 (master) npm/npm#13199

Closed

bnoordhuis mentioned this pull request Aug 29, 2017

zos: support platform #1276

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace fs.accessSync calls #955

Replace fs.accessSync calls #955

richardlau commented Jun 13, 2016 •

edited

Loading

richardlau commented Jun 13, 2016

richardlau commented Jun 13, 2016

mhdawson commented Jun 13, 2016

richardlau commented Jun 13, 2016

Flarna commented Jun 13, 2016

mhdawson commented Jun 13, 2016

bnoordhuis Jun 14, 2016

gibfahn commented Jun 14, 2016

gibfahn commented Jun 14, 2016

richardlau commented Jun 14, 2016

richardlau Jun 14, 2016

bnoordhuis Jun 17, 2016

richardlau Jun 17, 2016 •

edited

Loading

richardlau commented Jun 17, 2016

bnoordhuis Jun 17, 2016 •

edited

Loading

richardlau Jun 17, 2016

bnoordhuis Jun 17, 2016

richardlau commented Jun 17, 2016

richardlau commented Jun 17, 2016

bnoordhuis commented Jun 17, 2016

mhdawson commented Jun 17, 2016

		@@ -0,0 +1,3 @@
		var addon = require('bindings')('hello');
		exports.hello = function() { return addon.hello() }

Replace fs.accessSync calls #955

Replace fs.accessSync calls #955

Conversation

richardlau commented Jun 13, 2016 • edited Loading

richardlau commented Jun 13, 2016

richardlau commented Jun 13, 2016

mhdawson commented Jun 13, 2016

richardlau commented Jun 13, 2016

Flarna commented Jun 13, 2016

mhdawson commented Jun 13, 2016

bnoordhuis Jun 14, 2016

Choose a reason for hiding this comment

gibfahn commented Jun 14, 2016

gibfahn commented Jun 14, 2016

richardlau commented Jun 14, 2016

richardlau Jun 14, 2016

Choose a reason for hiding this comment

bnoordhuis Jun 17, 2016

Choose a reason for hiding this comment

richardlau Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

richardlau commented Jun 17, 2016

bnoordhuis Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

richardlau Jun 17, 2016

Choose a reason for hiding this comment

bnoordhuis Jun 17, 2016

Choose a reason for hiding this comment

richardlau commented Jun 17, 2016

richardlau commented Jun 17, 2016

bnoordhuis commented Jun 17, 2016

mhdawson commented Jun 17, 2016

richardlau commented Jun 13, 2016 •

edited

Loading

richardlau Jun 17, 2016 •

edited

Loading

bnoordhuis Jun 17, 2016 •

edited

Loading