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

process.argv[0] contains full node.exe path on Windows, docs say it must be just "node" #7434

Closed
Dmitry-Me opened this issue Jun 27, 2016 · 16 comments
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.

Comments

@Dmitry-Me
Copy link

Dmitry-Me commented Jun 27, 2016

  • Version: 4.4.6.0
  • Platform: Windows 7 Enterprise SP1
  • Subsystem: no idea

Documentation https://nodejs.org/api/process.html#process_process_argv says process.argv[0] must be node but I run this code on Windows

console.log('argv[0]: ' + process.argv[0]);

and it outputs this

argv[0]: C:\Program Files (x86)\nodejs\node.exe

which kind of isn't the same as node. It was node on some earlier versions - perhaps a couple years ago.

Here's how I run my code: I started cmd.exe, cdd into C:\Program Files (x86)\nodejs, so it's now the current path and I run

node fullPathToJsFile
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 27, 2016

./node -p 'process.argv[0]'
/Users/Jeremiah/Documents/node/out/Release/node

The docs are arguably not entirely correct. It will be the node binary, wherever that is. (Including the path.)

@Fishrock123 Fishrock123 added question Issues that look for answers. doc Issues and PRs related to the documentations. and removed question Issues that look for answers. labels Jun 27, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 27, 2016

More specifically, how you invoked the node binary. node ... will have node but ./ will append the path to the local node.

@Dmitry-Me
Copy link
Author

I started cmd, cdd into C:\Program Files (x86)\nodejs, so it's now the current path. I run

 node fullPathToJsFile

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Jun 27, 2016
@addaleax addaleax added good first issue Issues that are suitable for first-time contributors. docs-requested labels Jun 27, 2016
@addaleax
Copy link
Member

There’s this line which will always set process.argv[0] to process.execPath, so usually it won’t matter how node was invoked.

@tarungarg546
Copy link
Contributor

I am up for taking this up.
What needs to done exactly modifying docs?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 27, 2016

The process.argv docs should be updated to say that the first element will be process.execPath. It would probably be good to link there too.

@Dmitry-Me
Copy link
Author

@cjihrig Why did the behavior change since perhaps two years ago? I remember clearly that a while ago it was node, not full path.

@addaleax
Copy link
Member

#1194 was the PR that changed this for Windows.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 27, 2016

I honestly don't recall.

tarungarg546 added a commit to tarungarg546/node that referenced this issue Jun 27, 2016
@tarungarg546
Copy link
Contributor

#7449

@Dmitry-Me
Copy link
Author

Dmitry-Me commented Jun 27, 2016

@addaleax @cjihrig Okay, may it be that changes from #1194 should be further adapted so that old behavior is preserved? Backward compatibility, you know...

tarungarg546 added a commit to tarungarg546/node that referenced this issue Jun 27, 2016
The current documentation states that if run something like node app.js then
in our process.argv array first elements is node,
but actually its process.execPath not node
as documentation currently suggests

This commit fixes this documentation bug.

Fixes : nodejs#7434
PR-URL: nodejs#7449
@cjihrig
Copy link
Contributor

cjihrig commented Jun 28, 2016

At this point, the change has been in a wild long enough that backward compatibility would be hard to achieve.

@addaleax addaleax removed the good first issue Issues that are suitable for first-time contributors. label Jun 29, 2016
@Dmitry-Me
Copy link
Author

Reviewed by four contributors. That's impressive.

@addaleax
Copy link
Member

addaleax commented Jul 4, 2016

@Dmitry-Me I’m not a hundred percent sure what you’re trying to say. Documenting the current behaviour is definitely the right thing to do, independently of whether a change to the actual logic should be made or not.

The reviews concerned the correctness of the docs change and I wouldn’t take them to make any statement on what the behaviour should be.

I won’t keep you from reopening this issue and making convincing points as to why the behaviour should be changed back, but keep a few things in mind:

  • The original change in src: don't error at startup when cwd doesn't exist #1194 brought parity between Windows and the rest of the world; something that is quite valuable for a cross-platform runtime like Node.
  • That change happened over a year ago, and people may have come to rely on the current behaviour.
  • Checking process.argv[0] against a specific value to test for a Node-like environment is not very reliable, or even a very common way to check it.

@Dmitry-Me
Copy link
Author

@addaleax I'm suprised to see so many contributors signing off a commit. It usually takes up to two contributors (elsewhere). I won't make any conclusions.

@addaleax
Copy link
Member

addaleax commented Jul 4, 2016

@Dmitry-Me I think it’s just that there is a tendency for small changes that are easy to review to gain more reviewers, that’s all. :)

Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
The current documentation states that if run something like
`node app.js` then in our process.argv array first elements is `node`,
but actually it's `process.execPath` not `node`
as documentation currently suggests.

Fixes: #7434
PR-URL: #7449
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ppannuto added a commit to ppannuto/node that referenced this issue Aug 1, 2016
For historical and other reasons, node overwrites `argv[0]` on startup.
See
 - 2c6f79c,
 - nodejs#7434
 - nodejs#7449
 - nodejs#7696

For cases where it may be useful, save the original value of `argv[0]`
in `process.argv0`
addaleax pushed a commit that referenced this issue Aug 8, 2016
For historical and other reasons, node overwrites `argv[0]` on startup.
See
 - 2c6f79c,
 - #7434
 - #7449
 - #7696

For cases where it may be useful, save the original value of `argv[0]`
in `process.argv0`

PR-URL: #7696
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
cjihrig pushed a commit that referenced this issue Aug 10, 2016
For historical and other reasons, node overwrites `argv[0]` on startup.
See
 - 2c6f79c,
 - #7434
 - #7449
 - #7696

For cases where it may be useful, save the original value of `argv[0]`
in `process.argv0`

PR-URL: #7696
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. labels Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants