Rewrite exec using execsync-ng (which uses node-ffi)

[spion]

Hi,

Shelljs has been awesome so far, except for exec. I know how it works, with the write-to-a-temporary file loop, and I know that its a hack necessary due to the lack of child_process.execSync.

Unfortunately, exec has been very buggy for me, randomly not working on various developer and server machines. Usually it gets stuck in an endless loop. I never seem to be able to satisfyingly reproduce the issue or find the cause.

So I decided to give that up and write shelljs-ffi based on execsync-ng which in turn is based on node-ffi. execsync-ng is a fork of execSync that also works on windows.

You can check out these modules at https://github.com/doxout/shelljs and https://github.com/doxout/execSync or try them out: npm install shelljs-ffi execsync-ng

I know that a native module dependency such as node-ffi is probably unacceptable for shelljs. However I decided to ask anyway... Is it possible that this fork may be accepted into shelljs?

[arturadib]

Hey Gorgi, nice work there.

I like where you're going with this, but off the top of my head here's a couple of concerns: First is cross-platform reliability. Having a native dependency might add some issues at install time (the build might break) or during runtime (the platform might not have the necessary libraries). I don't think either is a show-stopper, I just don't have enough experience with lib-ffi to know it's reliable enough at the moment. @TooTallNate ?

Also, I don't have enough experience with native libs on Windows -- for example, is your dependency on msvcrt pretty much guaranteed to be satisfied on any Windows box? Perhaps we can come up with a fallback to the current hack in case we detect msvcrt is not available?

Another point is that if I'm not mistaken your current implementation of execSync lumps all of the stdout into one blob that's spit out at the end of the process... Ideally it'd progressively print out the contents at runtime, like the current implementation. Do you think you can fix that? We don't have tests for that case, so right now everything passes... (New tests welcome! :))

Let me know your thoughts. As I said I like where you're going, and if this solution is proven to be more reliable across platforms than the current one I'm all in!

PS: Let me bring a couple of other friends here to chime in: @TooTallNate and @sindresorhus

[TooTallNate]

I'd say do a hybrid approach, where node-ffi is listed in shelljs' "optionalDependencies" hash. This way, if it fails to install for whatever reason, there can still be the old-style "hack" fallback.

As far as the msvcrt question: I believe that's Windows' C runtime, so it should pretty much be on every windows system unless there's something really screwy about it :p

[sindresorhus]

^ Sounds like a good approach.

[arturadib]

Thanks @TooTallNate & @sindresorhus.

@spion - could you issue a PR where node-ffi is listed in optionalDependencies and fallback to the current hack if it's not available? Also if there's a way to test whether msvcrt is installed we can also maybe fallback to the hack too.

Thanks in advance!

[spion]

Yes I can, but I'm not sure how to test the result - how would I write tests that check if execSync works both with and without the execsync-ng dependency? Perhaps by controlling whether execsync-ng is used or not through a global variable?

As for the situation when msvcrt is missing, I researched a bit and came to the following conclusions

1. a version of msvcrt is distributed with the VC++ compiler so if node-ffi manages to build, the compiler is present, therefore the msvcrt library is also present.

2. if node-ffi fails to load a library, or it fails to load one of the specified functions, it will throw an exception. Therefore, the code

   var execSyncNG;
   try {
       execSyncNG = require('execsync-ng');
   } catch (e) {
       execSyncNG = null;
   }

   should result with execSyncNG being null in both cases (execsync-ng failed to install or node-ffi failed to load msvcrt)

[aurium]

Node now supports child_process.execSync https://nodejs.org/api/child_process.html#child_process_child_process_execsync_command_options

[nfischer]

Closing this, since we no longer support v0.10, and can utilize the synchronous functions.