Skip to content

Add NODE_DEBUG info to child_process - #721

Closed
remixz wants to merge 1 commit into
nodejs:v1.xfrom
remixz:gh-720-node-debug-child-process
Closed

remixz wants to merge 1 commit into
nodejs:v1.xfrom
remixz:gh-720-node-debug-child-process

Conversation

@remixz

@remixz remixz commented Feb 5, 2015

Copy link
Copy Markdown
Contributor

Fixes GH-720. Also adds the arguments passed to the Error object.

Example:

var spawn = require('child_process').spawn;
var path = require('path');

var child = spawn('fdjslkdf', ['--foo', 'bar'], {
  cwd: path.join(process.cwd(), './bat')
});

child.on('error', function (err) {
  throw err;
});
$ NODE_DEBUG=child_process ./iojs tmp.js
CHILD_PROCESS 72525: command: fdjslkdf
CHILD_PROCESS 72525: arguments: --foo,bar
CHILD_PROCESS 72525: options: {"cwd":"/Users/zach/Documents/github/remixz/io.js/bat"}
/Users/zach/Documents/github/remixz/io.js/tmp.js:9
  throw err;
        ^
Error: spawn fdjslkdf ENOENT
    at exports._errnoException (util.js:738:11)
    at Process.ChildProcess._handle.onexit (child_process.js:1029:32)
    at child_process.js:1120:20
    at process._tickCallback (node.js:337:11)
    at Function.Module.runMain (module.js:489:11)
    at startup (node.js:111:16)
    at node.js:799:3

Comment thread lib/child_process.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing a semicolon

@remixz
remixz force-pushed the gh-720-node-debug-child-process branch from dfe56bd to a967948 Compare February 5, 2015 03:38
@remixz

remixz commented Feb 5, 2015

Copy link
Copy Markdown
Contributor Author

@cjihrig All done! Rebased into a967948.

Comment thread lib/child_process.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you move the slice(1) from below to this line so that it's only called when an error occurs.

@cjihrig

cjihrig commented Feb 5, 2015

Copy link
Copy Markdown
Contributor

@trevnorris what do you think?

Fixes nodejsGH-720. Also adds the arguments passed to the Error object.
@remixz
remixz force-pushed the gh-720-node-debug-child-process branch from a967948 to f7711fe Compare February 5, 2015 03:54
@trevnorris

Copy link
Copy Markdown
Contributor

I'm cool with this. Except for the debug() call, it only affects error cases. And this isn't a hot code path anyways. LGTM.

@cjihrig want to clean up the commit and land it, or shall I?

@cjihrig

cjihrig commented Feb 5, 2015

Copy link
Copy Markdown
Contributor

I'll land it.

cjihrig pushed a commit that referenced this pull request Feb 5, 2015
This commit adds debug() calls to spawn() and spawnSync(), and
attaches additional information to Error objects.

Fixes: #720
PR-URL: #721
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@cjihrig

cjihrig commented Feb 5, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 9a8f186. I tweaked the commit message and moved the debug('spawnSync' up a few lines to avoid printing the environment data too.

@cjihrig cjihrig closed this Feb 5, 2015
@rvagg

rvagg commented Feb 9, 2015

Copy link
Copy Markdown
Member

@cjihrig @trevnorris should this have landed with a test too? It doesn't look trivial enough to me to not include a new test to help prevent regressions.

Reopening but please feel free to close if you have another perspective.

@rvagg rvagg reopened this Feb 9, 2015
@trevnorris

Copy link
Copy Markdown
Contributor

@rvagg The change isn't going to break existing functionality, but we could add a test to make sure the correct values are being attached to the error object.

@cjihrig

cjihrig commented Feb 9, 2015

Copy link
Copy Markdown
Contributor

I'll add the test for the error objects.

@cjihrig

cjihrig commented Feb 14, 2015

Copy link
Copy Markdown
Contributor

Test landed in d53b636

@cjihrig cjihrig closed this Feb 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants