thoughtbot / cocaine

A small library for doing (command) lines.
https://robots.thoughtbot.com
Other
785 stars 55 forks source link

Don't use ClimateControl when it's not necessary #73

Closed jyurek closed 9 years ago

jyurek commented 9 years ago

The ProcessRunner and PosixRunner are perfectly capable of taking an environment hash. This means there's no reason to use ClimateControl to modify the environment beforehand. In order to find custom binaries, we can modify the PATH on the command line -- yes, for both Unix and Windows.

Unix commandlines use the normal PATH=/new/path:$PATH cmd syntax. Windows uses SET PATH=C:\new\path;%PATH% & cmd instead.

The difference between then and now is that I found out that you can actually separate commands in Windows with & (see http://support.microsoft.com/kb/279253).

As a result of this the supplemental_environment isn't modified when the path is modified. We keep the path in memory as a Cocaine::OS.path_separator-separated string.

In addition, this commit cleans up the OS detection slightly, moving it to a new class. In the future, we can do things like telling instead of asking.

The result of all this is that the ProcessRunner and PosixRunner should be thread-safe, since we're not mucking around with the ENV when we run stuff anymore. This will make the high-throughput users of Paperclip happy, since they shouldn't see clobbering issues like they did before. I tested this by running the code supplied by @maxim, https://github.com/thoughtbot/paperclip/issues/1709#issuecomment-64812263, with the unmodified cocaine 0.5.4 gem and with this commit in place. It failed in a few seconds before and ran for as long as I wanted after.

This also might fix a bug? Previously, on Java on Windows, commands would be called prefixed with env, which doesn't exist on Windows. No one complained, though, so you can guess how popular Java on Windows is (or I'm completely screwing up). Now we look for Windows first, then use env with Java, and then Unix like normal.

maxim commented 9 years ago

:clap: thank you.

balexand commented 9 years ago

Looks good. Thanks! :+1: