gyp: Export LD_LIBRARY_PATH quoted in case $(builddir) contains spaces #1277

melloc · 2017-08-25T23:53:01Z

Checklist

npm install && npm test passes
tests are included
commit message follows commit guidelines

Description of change

This change will make sure that the path to the build directory gets properly quoted and escaped when running actions.

Tar version 3 performs better and is more well tested than its predecessor. npm will be using this in the near future, so there is no benefit in shipping a node-gyp that uses the slower and less reliable fstream-based tar. This drops support for node 0.x, and thus should be considered a breaking semver-major change. PR-URL: nodejs#1212 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

* dropping support for node < 4 * signal the CI not to test node < 4

If you're providing a path to a header tarball to install, you probably want it to always be re-installed. PR-URL: nodejs#1220 Fixes: nodejs#1216 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

* test: build simple addon in path with non-ascii characters * test: add test-charmap.py PR-URL: nodejs#1203 Reviewed-By: Refael Ackermann <refack@gmail.com>

Enable linking to the platform specific installation instructions PR-URL: nodejs#1225 Reviewed-By: Refael Ackermann <refack@gmail.com>

Lifted verbatim from https://github.com/nodejs/node/blob/master/CONTRIBUTING.md then `s/Node.js/node-gyp/`. PR-URL: nodejs#1229 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Give users reporting bugs a clearer idea of the info that will be helpful when reporting issues. PR-URL: nodejs#1228 Refs: https://github.com/nodejs/node/tree/master/.github Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

When actions are run, they're preceded by an update to the LD_LIBRARY_PATH environment variable. If $(builddir) includes any special shell characters, then the action will fail to run and the build will fail.

refack · 2017-08-27T19:17:46Z

Hello @melloc, thank you for the contribution.
This LGTM (apart from a few style nits), and I'm running node-gyp's CI on this: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/34/
But what I would ask you to do, if you are so inclined, is it to submit this upstream to: https://gyp.gsrc.io/docs/Hacking.md
(If you rather not deal with the hassle, please let me know, and I'll submit it on your behalf.)

refack

2 nits

refack · 2017-08-27T19:25:07Z

gyp/pylib/gyp/generator/make.py

@@ -916,11 +916,12 @@ def WriteActions(self, actions, extra_sources, extra_outputs,
      # libraries, but until everything is made cross-compile safe, also use
      # target libraries.
      # TODO(piman): when everything is cross-compile safe, remove lib.target
-      self.WriteLn('cmd_%s = LD_LIBRARY_PATH=$(builddir)/lib.host:'
-                   '$(builddir)/lib.target:$$LD_LIBRARY_PATH; '
+      subst_builddir = '$(subst \',\'\\\'\',$(builddir))'


nit: I'd rather you use the " string delimiter and the r prefix so you get:

subst_builddir = r"$(subst ', '\'', $(builddir))"

Also IMHO the arg to self.WriteLn should be constructed before the call

action_cmd = "..." \ "..." % (...) self.WriteLn(action_cmd)

refack · 2017-08-27T19:30:08Z

Seems like the new test fails on Windows:

module.js:487
    throw err;
    ^

Error: Cannot find module 'name's (weird)'
    at Function.Module._resolveFilename (module.js:485:15)
    at Function.Module._load (module.js:437:25)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at [eval]:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:569:30)
    at evalScript (bootstrap_node.js:432:27)
child_process.js:591
    throw err;
    ^

Error: Command failed: node -e require('name\'s (weird)')
module.js:487
    throw err;
    ^

Error: Cannot find module 'name's (weird)'
    at Function.Module._resolveFilename (module.js:485:15)
    at Function.Module._load (module.js:437:25)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at [eval]:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:569:30)
    at evalScript (bootstrap_node.js:432:27)


    at checkExecSyncError (child_process.js:568:13)
    at execFileSync (child_process.js:588:13)
    at runSpacedName (c:\workspace\nodegyp-test-commit\nodes\win2012r2\test\test-actions.js:15:10)
    at c:\workspace\nodegyp-test-commit\nodes\win2012r2\test\test-actions.js:29:19
    at ChildProcess.exithandler (child_process.js:244:7)
    at emitTwo (events.js:125:13)
    at ChildProcess.emit (events.js:213:7)
    at maybeClose (internal/child_process.js:887:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:208:5)

refack · 2017-08-27T19:40:13Z

test/test-actions.js

+var nodeGyp = path.resolve(__dirname, '..', 'bin', 'node-gyp.js')
+
+function runSpacedName() {
+  var testCode = "require('name\\'s (weird)')"


You can't put a \ in a Windows path. But you can put an unescaped '.
You could try using require("${addonPath}") but YMMV

refack · 2017-08-27T19:42:29Z

test/test-actions.js

+    var lastLine = logLines[logLines.length-1]
+    t.strictEqual(err, null)
+    t.strictEqual(lastLine, 'gyp info ok', 'should end in ok')
+    t.strictEqual(runSpacedName().trim(), 'world')


You should lower the .trim() into runSpacedName

refack · 2017-08-27T19:44:03Z

test/test-actions.js

+  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/)


const lastLine = stderr.toString().trim().split(os.EOL).pop();

refack · 2017-08-27T19:48:05Z

test/test-actions.js

+    var lastLine = logLines[logLines.length-1]
+    t.strictEqual(err, null)
+    t.strictEqual(lastLine, 'gyp info ok', 'should end in ok')
+    t.strictEqual(runSpacedName().trim(), 'world')


You should put a comment explaining where "world" comes from.

melloc · 2017-08-30T01:24:50Z

@refack Thanks for the feedback. I suspect the Windows tests are failing because the shell script is failing and not creating hello.js, although I'm not sure why this isn't causing node-gyp to exit non-zero. I'll modify it to run a batch script on Windows, and the shell script on everything else, and see if I can find a Windows environment to test it in.

Once I have that I'll look into what it would take to get it submitted upstream, too.

rvagg · 2019-06-21T02:57:42Z

not sure if this is still needed, if so and it belongs here please open a fresh PR for it, otherwise maybe https://github.com/refack/GYP is the right place for it

isaacs and others added 8 commits June 6, 2017 06:56

bump to v4.0.0

75cfae2

* dropping support for node < 4 * signal the CI not to test node < 4

gyp: update xml string encoding conversion

d6139b5

* test: build simple addon in path with non-ascii characters * test: add test-charmap.py PR-URL: nodejs#1203 Reviewed-By: Refael Ackermann <refack@gmail.com>

doc: headerify the Install instructions

35e1dbb

Enable linking to the platform specific installation instructions PR-URL: nodejs#1225 Reviewed-By: Refael Ackermann <refack@gmail.com>

doc: update proposed DCO and CoC

2e40fad

Lifted verbatim from https://github.com/nodejs/node/blob/master/CONTRIBUTING.md then `s/Node.js/node-gyp/`. PR-URL: nodejs#1229 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

gyp: Actions should quote LD_LIBRARY_PATH value

013f41f

When actions are run, they're preceded by an update to the LD_LIBRARY_PATH environment variable. If $(builddir) includes any special shell characters, then the action will fail to run and the build will fail.

refack approved these changes Aug 27, 2017

View reviewed changes

refack suggested changes Aug 27, 2017

View reviewed changes

refack reviewed Aug 27, 2017

View reviewed changes

melloc mentioned this pull request Sep 8, 2017

Still getting lots of DtraceProviderBindings errors on OS X trentm/node-bunyan#216

Closed

rvagg force-pushed the master branch from 1456ef2 to 7a71d68 Compare April 24, 2019 00:37

ghost mentioned this pull request May 23, 2019

Build failure when builddir contains spaces. #65

Closed

rvagg force-pushed the master branch from ed3ff7e to 611bc3c Compare June 21, 2019 02:25

rvagg closed this Jun 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gyp: Export LD_LIBRARY_PATH quoted in case $(builddir) contains spaces #1277

gyp: Export LD_LIBRARY_PATH quoted in case $(builddir) contains spaces #1277

melloc commented Aug 25, 2017 •

edited

Loading

refack commented Aug 27, 2017

refack left a comment

refack Aug 27, 2017

refack commented Aug 27, 2017

refack Aug 27, 2017

refack Aug 27, 2017 •

edited

Loading

refack Aug 27, 2017 •

edited

Loading

refack Aug 27, 2017

melloc commented Aug 30, 2017 •

edited

Loading

rvagg commented Jun 21, 2019

gyp: Export LD_LIBRARY_PATH quoted in case $(builddir) contains spaces #1277

gyp: Export LD_LIBRARY_PATH quoted in case $(builddir) contains spaces #1277

Conversation

melloc commented Aug 25, 2017 • edited Loading

Checklist

Description of change

refack commented Aug 27, 2017

refack left a comment

Choose a reason for hiding this comment

refack Aug 27, 2017

Choose a reason for hiding this comment

refack commented Aug 27, 2017

refack Aug 27, 2017

Choose a reason for hiding this comment

refack Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 27, 2017

Choose a reason for hiding this comment

melloc commented Aug 30, 2017 • edited Loading

rvagg commented Jun 21, 2019

melloc commented Aug 25, 2017 •

edited

Loading

refack Aug 27, 2017 •

edited

Loading

refack Aug 27, 2017 •

edited

Loading

melloc commented Aug 30, 2017 •

edited

Loading