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

doc: improve child_process.execFile() code example #4504

Closed
wants to merge 1 commit into from

Conversation

ryansobol
Copy link
Contributor

The current code example for child_process.execFile() doesn't work even on Unix-like platforms. Since there's no shell wrapper,

  1. file must be a relative or absolute path to executable.
  2. args must contain all the command line arguments for file.
  3. I/O redirection and file globbing is not possible.

Here's an alternative I whipped up that should work on both Unix and Windows platforms. Suggestions welcome.

@@ -176,8 +176,8 @@ replace the existing process and uses a shell to execute the command.*

### child_process.execFile(file[, args][, options][, callback])

* `file` {String} The filename of the program to run
* `args` {Array} List of string arguments
* `file` {String} An absolute path to an executable file.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not true, don't know what gives @ryansobol that idea:

> child_process.execFile('date', function(_, stdout){console.log(stdout)});null;
null
> Thu 31 Dec 2015 16:43:59 PST

man pages:

The execlp(), execvp(), and execvpe() functions duplicate the actions of the shell in searching for an executable file if the specified filename does not contain a slash (/) character. The file is sought in the colon-separated list of directory pathnames specified in the PATH environment variable.

see http://linux.die.net/man/3/execvp

Copy link
Contributor

Choose a reason for hiding this comment

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

The "if there is a slash it won't be looked up in the PATH" is an interesting gotcha, worth mentioning... but would require confirmation that is how it works on windows, too, so the docs can be correct for both systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed.

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jan 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Jan 1, 2016

The subsystem prefix should probably be doc: instead of docs: in the commit message.

@ryansobol ryansobol changed the title docs: improve child_process.execFile() code example doc: improve child_process.execFile() code example Jan 1, 2016
@ryansobol ryansobol force-pushed the execFile branch 4 times, most recently from 5c7bc12 to df05b3a Compare January 1, 2016 17:26
@ryansobol
Copy link
Contributor Author

@sam-github @mscdex: Thanks for the input. I think all your concerns have been addressed. Please let me know if there's anything else that needs tweaking.

@@ -176,7 +176,7 @@ replace the existing process and uses a shell to execute the command.*

### child_process.execFile(file[, args][, options][, callback])

* `file` {String} The filename of the program to run
* `file` {String} A relative or absolute path to an executable file
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not right. In my comment I showed the execing "date" works fine, and it not a relative or absolute path. The previous docs are correct, what are you trying to fix?

@ryansobol
Copy link
Contributor Author

@sam-github I see how the explanation of searching the directories inside the PATH env variable was not correct. That's been fixed.

I also see how the relative and absolute path description of the file argument is confusing. Personally, I think of file as a path and argue that the date example is indeed a relative path (with special behavior as you've noted). I updated that argument's description, but please let me know if you feel this is still not an improvement. I'll revert it back completely if that's the case.

@ryansobol ryansobol force-pushed the execFile branch 2 times, most recently from 7219a67 to 21e0253 Compare January 9, 2016 15:57
@jasnell
Copy link
Member

jasnell commented Jan 11, 2016

LGTM

[`child_process.exec()`][].

The same options as `child_process.exec()` are supported. If a `file` path
contains no slash delimiters, `child_process.execFile()` searches for a matching
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "slash delimiters", I would say "path separators", "path delimiters", or just "slashes".

From a higher level, this might not be the best place to explain how PATH lookup works, as it's not specific to execFile().

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I suggested mentioning the slash caveat... but I think @cjihrig is right, its not specific to execFile, its just how PATH lookup works, so probably not worth mentioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I've removed the line.

@sam-github
Copy link
Contributor

looks fine to me

@ryansobol
Copy link
Contributor Author

Fixed and rebased

@cjihrig
Copy link
Contributor

cjihrig commented Jan 13, 2016

LGTM. Thanks.

cjihrig pushed a commit that referenced this pull request Jan 13, 2016
PR-URL: #4504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Jan 13, 2016

Landed in d139704

@cjihrig cjihrig closed this Jan 13, 2016
rvagg pushed a commit that referenced this pull request Jan 14, 2016
PR-URL: #4504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@ryansobol ryansobol deleted the execFile branch January 17, 2016 17:25
@MylesBorins
Copy link
Contributor

kevinoid added a commit to kevinoid/node that referenced this pull request Feb 18, 2016
The changes to the file argument of execFile in nodejs#4504 make it appear
that execFile requires an absolute or relative path to the executable
file, when it also supports a filename which will be resolved using
$PATH.  Although the example makes this clear, assuming there isn't a
node binary in $CWD, it's easy to overlook.  This commit clarifies that
point.

It also updates the argument description for execFileSync to match,
since it was overlooked in nodejs#4504 and behaves identically.

Signed-off-by: Kevin Locke <[email protected]>
jasnell pushed a commit that referenced this pull request Feb 20, 2016
The changes to the file argument of execFile in #4504 make it appear
that execFile requires an absolute or relative path to the executable
file, when it also supports a filename which will be resolved using
$PATH.  Although the example makes this clear, assuming there isn't a
node binary in $CWD, it's easy to overlook.  This commit clarifies that
point.

It also updates the argument description for execFileSync to match,
since it was overlooked in #4504 and behaves identically.

PR-URL: #5310
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 21, 2016
The changes to the file argument of execFile in #4504 make it appear
that execFile requires an absolute or relative path to the executable
file, when it also supports a filename which will be resolved using
$PATH.  Although the example makes this clear, assuming there isn't a
node binary in $CWD, it's easy to overlook.  This commit clarifies that
point.

It also updates the argument description for execFileSync to match,
since it was overlooked in #4504 and behaves identically.

PR-URL: #5310
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
PR-URL: #4504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
The changes to the file argument of execFile in #4504 make it appear
that execFile requires an absolute or relative path to the executable
file, when it also supports a filename which will be resolved using
$PATH.  Although the example makes this clear, assuming there isn't a
node binary in $CWD, it's easy to overlook.  This commit clarifies that
point.

It also updates the argument description for execFileSync to match,
since it was overlooked in #4504 and behaves identically.

PR-URL: #5310
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
PR-URL: #4504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
The changes to the file argument of execFile in #4504 make it appear
that execFile requires an absolute or relative path to the executable
file, when it also supports a filename which will be resolved using
$PATH.  Although the example makes this clear, assuming there isn't a
node binary in $CWD, it's easy to overlook.  This commit clarifies that
point.

It also updates the argument description for execFileSync to match,
since it was overlooked in #4504 and behaves identically.

PR-URL: #5310
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
PR-URL: #4504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
The changes to the file argument of execFile in #4504 make it appear
that execFile requires an absolute or relative path to the executable
file, when it also supports a filename which will be resolved using
$PATH.  Although the example makes this clear, assuming there isn't a
node binary in $CWD, it's easy to overlook.  This commit clarifies that
point.

It also updates the argument description for execFileSync to match,
since it was overlooked in #4504 and behaves identically.

PR-URL: #5310
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
PR-URL: nodejs#4504
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants