nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.64k stars 29.62k forks source link

Windows: node.cmd shortcut instead of full-blown node.exe #871

Closed am11 closed 9 years ago

am11 commented 9 years ago

In io.js dist, node.exe is provided as a separate executable which is confusing and causes problems if your script is relying on process.execPath and your intention is to run iojs via node alias (npm task etc.).

For Linux, io.js provides a (4bytes) symlink called node which points to iojs in bin/ dir. On Windows we can also create a symlink, and resolve to real executable (like this) but since Windows symbolic links are not sharable FS objects, and they actually reflect the real path if you try to move, copy or package it to a zip. Meaning you cannot copy/paste Windows symlink itself (like shortcut (.lnk) files), it will copy the original file instead.

Having said that, here is a proposed solution:

Inspired by nvmw, which downloads only iojs.exe and then makes a copy of alias-node.cmd to node.cmd in iojs version installation directory like this: nvmw.bat#L170, where alias-node.cmd has the canned content:

:: Created by nvmw, please don't edit manually. 
@IF EXIST "%~dp0\iojs.exe" ( 
   "%~dp0\iojs.exe" %* 
) ELSE ( 
   iojs %* 
) 

Please add this node.cmd in io.js distro in place of node.exe.

Related: https://github.com/appveyor/ci/issues/139.

piscisaureus commented 9 years ago

I agree that this would be a nice, however the issue is that child_process.spawn() can't run .cmd files. This breaks for programs that try to spawn('node').

Using a batch file would also make it impossible to obtain the pid of "node", which makes IPC stdio streams nonfunctional.

Also note that on windows node.exe and iojs.exe are hard linked together. If a symlink was used it would work as expected (except https://github.com/libuv/libuv/issues/206), but symlinks can't be created on windows 2003, and it requires administrator privileges and elevation on other windows versions.

am11 commented 9 years ago

@piscisaureus, thank you for the clarification.

Aside, I am sorry but I am not sure how else to test spawn('node') issue you described. Please take a look at this outcome:

:: this is cmd on Windows 8.1
$ nvmw install iojs-v1.2.0
...

$ nvmw use iojs-v1.2.0
Now using iojs v1.2.0

$ iojs -v
v1.2.0

$ iojs

> var child_process = require('child_process');
> child_process.spawn('c:/users/adeel/.nvmw/iojs/v1.2.0/node.cmd', ['-p', 'process.execPath']).stdout.on('data', function(e){console.log('data: ' + e)})
...
...
> data: c:\Users\Adeel\.nvmw\iojs\v1.2.0\iojs.exe
(^C again to quit)
>
$ node -v
v1.2.0

$ node

> var child_process = require('child_process');
> child_process.spawn('c:/users/adeel/.nvmw/iojs/v1.2.0/node.cmd', ['-p', 'process.execPath']).stdout.on('data', function(e){console.log('data: ' + e)})
...
...
> data: c:\Users\Adeel\.nvmw\iojs\v1.2.0\iojs.exe

Apparently, spawn has no issue executing that .cmd file.

piscisaureus commented 9 years ago

Apparently, spawn has no issue executing that .cmd file.

:astonished: I just tried your test case and, I can confirm it works. But it shouldn't work! And the inability to spawn .cmd/.bat files has been on my long-term wishlist for many years.

According to msdn, "to run a batch file, you must start the command interpreter; set lpApplicationName to cmd.exe and set lpCommandLine to the following arguments: /c plus the name of the batch file."

There is no way iojs is prepending 'cmd /c' when spawn() is used.

Am I dreaming? Is it the drugs? Can someone confirm that this works on windows 7 too?

piscisaureus commented 9 years ago

I think I figured out what happened.

Windows CreateProcess() has been able to run .bat and .cmd files since windows 2000. However, when I tested it, I provided a full path to the batch file to CreateProcess, but didn't specify a PATH environment variable to be passed to the child process. Subsequently CreateProcess would be unable to locate cmd.exe so it failed.

A relatively recent security fix (in response to CVE-2014-0315) hard-coded the full path to cmd.exe into CreateProcess, which makes it work.

There are still significant issues with running .bat files.

var spawnSync = require('child_process').spawnSync;

spawnSync('node.bat', ["-p", "'hello'"], { stdio: 'inherit' });
// works
// prints hello as expected.

spawnSync('c:\\users\\bert belder\\node.bat', ["-p", "'hello'"], { stdio: 'inherit' });
// works
// prints hello as expected

spawnSync('c:\\users\\bert belder\\node.bat', ['-p', '"hello"'], { stdio: 'inherit' });
// unexpected error:
// 'c:\users\bert' is not recognized as an internal or external command,
// operable program or batch file.

spawnSync('node.bat', ["-p", "'^'"], { stdio: 'inherit' });
// arguments are mangled
// prints nothing, while '^' was expected

spawnSync('node.bat', ["-p", '"^^"'], { stdio: 'inherit' });
// arguments are mangled
// prints nothing, while '^' was expected

spawnSync('node.bat', ["-p", '"^^^^"'], { stdio: 'inherit' });
// arguments are mangled
// prints '^', while '^^^^' was expected

spawnSync('node.bat', ["-p", '2', '>', '3'], { stdio: 'inherit' });
// arguments are mangled
// expected to print 'false'
// what actually happens is a file named '3' is created.
am11 commented 9 years ago

I do not know why but I am not getting this error: "arguments are mangled" even after moving c:/users/adeel/.nvmw/iojs/v1.2.0/node.cmd to c:/users/adeel/.nvmw/iojs/v1.2.0/node.bat and running the ditto commands.

The last line created file named 3 in the cwd (with type 3 outputting 2).

Perhaps, I have something in the path which is remedying it from happening. Here is the path output:

PATH=c:\Users\Adeel\.nvmw\;c:\Users\Adeel\.nvmw\iojs\v1.2.0;C:\ProgramData\Oracle\Java\javapath;C:\Program Files (x86)\P
ython\;C:\Program Files (x86)\Intel\iCLS Client\;C:\Program Files\Intel\iCLS Client\;C:\windows\system32;C:\windows;C:\w
indows\System32\Wbem;C:\windows\System32\WindowsPowerShell\v1.0\;C:\Program Files\Intel\Intel(R) Management Engine Compo
nents\DAL;C:\Program Files\Intel\Intel(R) Management Engine Components\IPT;C:\Program Files (x86)\Intel\Intel(R) Managem
ent Engine Components\DAL;C:\Program Files (x86)\Intel\Intel(R) Management Engine Components\IPT;C:\Program Files (x86)\
ATI Technologies\ATI.ACE\Core-Static;C:\Program Files\Lenovo\Bluetooth Software\;C:\Program Files\Lenovo\Bluetooth Softw
are\syswow64;C:\Program Files (x86)\Windows Kits\8.1\Windows Performance Toolkit\;C:\Program Files\Microsoft SQL Server\
110\Tools\Binn\;C:\Users\Adeel\AppData\Local\GitHub\PortableGit_054f2e797ebafd44a30203088cd3d58663c627ef\cmd;C:\Program
Files (x86)\Bitvise SSH Client;C:\Program Files\Microsoft SQL Server\120\Tools\Binn\;C:\Program Files (x86)\WinMerge;C:\
Program Files\Microsoft\Web Platform Installer\;C:\wamp\bin\php\php5.5.12;C:\Program Files\Microsoft SQL Server\120\DTS\
Binn\;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\110\Tools\Binn\;C:\Program Files (x86)\Microsoft SQL Server\
120\Tools\Binn\;C:\Program Files (x86)\Microsoft SQL Server\120\Tools\Binn\ManagementStudio\;C:\Program Files (x86)\Micr
osoft SQL Server\120\DTS\Binn\;C:\Program Files (x86)\Microsoft SDKs\TypeScript\1.0\;C:\HashiCorp\Vagrant\bin;C:\Program
 Files (x86)\Microsoft SDKs\TypeScript\1.1\;C:\Program Files (x86)\nodejs\;c:\users\adeel\.nvmw;C:\path\to\php;c:\path\t
o\php\pear;C:\Users\Adeel\AppData\Roaming\npm
piscisaureus commented 9 years ago

I do not know why but I am not getting this error: "arguments are mangled"

  • Try moving the batch file to a path with spaces in it.
  • I didn't mean to say that you'd see a message "arguments are mangled". What I meant is that the arguments don't come out on the other side as you put them in.
am11 commented 9 years ago

My bad. I thought that was the actual output. :smile:

sam-github commented 9 years ago

Instead of installing node as a symlink to iojs.exe, why don't we simply install the binary twice, with two different names?

It avoids all the downsides of symlinks, and preserves all the upsides of having iojs available under both names. On unix, symlinks (and hard links) work well for this kind of thing, I'm wondering if we went too far down the "do it like unix on windows" path.

piscisaureus commented 9 years ago

Instead of installing node as a symlink to iojs.exe, why don't we simply install the binary twice, with two different names

Right now the two files are hard linked. It saves some space - probably so little that it's unnoticeable. Other than that, it should behave as if there were two copies of the same file.

sam-github commented 9 years ago

I'm a bit lost then as to what the problem is, and pretty nervous about replacing with a node.cmd. Does the OP not like that there are two copies of the same file?

@am11 what problem are you trying to solve?

am11 commented 9 years ago

@sam-github, as noted earlier, the issue is described here: https://github.com/appveyor/ci/issues/139.

The AppVeyorCI for Windows is using node.exe provided by io.js distribution in addition to iojs.exe, as you guys are recommending.

As you can see there, it took some hit and trial to narrow down the issue to this:

Since node.cmd for Windows is a no-go, we will probably have to somehow inject this node.cmd into their iojs install folder and delete the node.exe (if it is even a permitted action) to successfully run io.js job on AppVeyor.

sam-github commented 9 years ago

Sorry, not really understanding the problem.

Naturally, we cannot replace node with iojs for AppVeyor CI, as it will fail the other two node jobs (v0.10 and v0.12).

Not following at all. You clearly already have two packages that contain a node.exe (v0.10 and v0.12), why is a third package (v1.x) a problem?

Threading through the link, it all seems to stem from the assumption that io.js and node are so different, they can be distinguished based on executable name:

https://github.com/appveyor/ci/issues/139#issuecomment-74379328

Why do that? Use version. 1.x is io.js, 0.10 and 0.12 are node... personally, I'd call them both "node". By the time joyent/node gets to 1.x in a couple years, iojs and it will have merged, and if not, then deal with the runtime distinction in a couple years, don't cause yourself this pain now.

Or wait a while, and use node -p process.release.iojs or whatever gets hacked into there if node-gyp decides not to use the version number.

sam-github commented 9 years ago

--> https://github.com/rvagg/node-gyp/blob/55936498fca8f4ae27d55d5ac12c3d714a93742c/lib/build.js#L18

am11 commented 9 years ago

@sam-github, we need to distinguish the runtimes to name our binaries for many-runtimes, various-platforms matrix, and publish on a separate repo: https://github.com/sass/node-sass-binaries/. On install, we reconstruct the name and download the matching binary from that repo.

I started with this PR: https://github.com/rvagg/pangyp/pull/1 and I do agree that our weird runtime detection is not the ideal way, but that is not causing of this issue; because even if we hardcode return {name:'iojs'} there, it throws the same exception Module did not self-register with node.exe (v1.2.0, in iojs directory). This is the only case which breaks.

am11 commented 9 years ago

Generally, this is what I have narrowed it down to:

If you built the binary like this:

# this is PS
iojs node_modules/pangyp/bin/node-gyp configure  # optional
iojs node_modules/pangyp/bin/node-gyp build

It will execute perfectly with iojs prefix, but not with the same versioned node.exe (that is bundled with io.js) in io.js bin folder.

Also related: https://github.com/sass/node-sass/issues/708#issuecomment-76151398

am11 commented 9 years ago

This turned out to be the same issue as https://github.com/iojs/io.js/issues/751.