ioquatix / script-runner

This package will run various script files inside of Atom. It currently supports JavaScript, CoffeeScript, Ruby, and Python. You can add more.
http://atom.io/packages/script-runner
Other
62 stars 23 forks source link

Windows: reliance on $SHELL variable and bash-like shell breaks windows compatibility #79

Open ccoenen opened 7 years ago

ccoenen commented 7 years ago

After installing script-runner on windows in Atom 1.19.0, I can't run any .js or .rb files. The extra UI pops up, and it shows a cursor, but it will never receive any content.

The inspector revealed these errors:

grafik

The call finally failing is spawn inside of child_process.js. This is called with (faulty?) options. There's an undefined file property. I'll investigate this next. This stems directly from getLoginEnvironmentFromShell which tries to

ChildProcess.spawn(process.env.SHELL, [.....]

but on my system, there is no process.env.SHELL (there is process.env, but no SHELL property is defined). This may have to be fixed over in https://github.com/ioquatix/shell-environment. Also, simply trusting in $SHELL -ilc seems to be a little odd - AFAIK zsh does not have -l. And cmd.exe or powershell.exe will have none of those.

It seems like script runner continues to rely on these assumptions as well:

https://github.com/ioquatix/script-runner/blob/add5bd2bcb281c988bcbc8ff09b1b46337c344f7/lib/script-runner.js#L91-L92

There may be more problems but I can't get further into it right now.

ioquatix commented 7 years ago

Thanks for the detailed report.

I see there are a few issues, but they should be resolved in the shell-environment package. The point of that package is to isolate cross-platform function for detecting the right env for running scripts.

I think the key thing is that windows obviously doesn't have a SHELL variable - what does it have? Anything?

A quick hack which should work would be to detect windows and simply return process.env from within shell-environment.

What do you think?

ioquatix commented 7 years ago

Windows support has been discussed in https://github.com/ioquatix/shell-environment/issues/2

I've added a patch to shell-environment to just return process.env if there is no process.env.SHELL. https://github.com/ioquatix/shell-environment/commit/cf2442f2a75724db2cbaba5bc2a3e3d5115fce78

Ideally, I think we figure out a better solution, but this should allow script-runner to work without crashing.

ioquatix commented 7 years ago

@ccoenen do you think you can test and report back?

ccoenen commented 7 years ago

I'll gladly add a pull request. I'll get to that, once we resolved the shell-environment issues. (i.e. once it is useful on windows)

ioquatix commented 7 years ago

I tested this on Windows 10 and it appears to work. I make a simples script:

#!python.exe

print "Hello World"

And it executed fine.

Just FYI.

ccoenen commented 7 years ago

Thanks for the heads-up!

I have tried this with the following languages (all of which are on my %PATH%). Apparently, one needs the #! at the top? This is somewhat odd, and in my case even problematic.

Without them I only get the blank panel with blinking cursor (no error message). Sadly the developer tools inside atom are not very forthcoming - if I run something that does not work while they are open, the whole thing just crashes :-(

ccoenen commented 7 years ago

Is there any possibility to hardcode these? I see very little advantage in requiring these comments. It would be fine, if a comment like that could override a default (for choosing python 3 over python 2 perhaps), but a default should be there nonetheless.

ioquatix commented 7 years ago

So, the nominal behaviour is to replicate the same #! format used on unix. The same script can be run anywhere.

If you don't provide a shebang line (#!) it will use the hard coded defaults: https://github.com/ioquatix/script-runner/blob/master/lib/script-runner.js#L38-L48

It seems like python on unix needs to be python.exe on Windows - is that correct?

ioquatix commented 7 years ago

Does #!python work on Windows?

ccoenen commented 7 years ago

as long as it's on the %PATH%, yes that's the case. It needs the .exe suffix, though.

Both Python2 and Python3 use python.exe in case you were wondering. I'd guess whichever is first in %PATH% wins.

ioquatix commented 7 years ago

Okay, so I guess the simplest solution would be to detect if on windows, and then add .exe suffix, unless you have a better idea?

ccoenen commented 7 years ago

yes, from what I can see adding .exe to each one of these should be good. In my case, even bash.exe might work, because it's loaded via my git install at C:\Program Files\Git\usr\bin\bash.exe

ccoenen commented 7 years ago

Now, all we need is a shortcut and we're good :D

ccoenen commented 6 years ago

gentle bumping

ccoenen commented 6 years ago

not so gentle bumping