Rewrite exec using execsync-ng (which uses node-ffi) about shelljs HOT 7 CLOSED

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

from shelljs.

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

from shelljs.

^ Sounds like a good approach.

from shelljs.

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!

from shelljs.

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)

from shelljs.

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

from shelljs.

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

from shelljs.