Fixed so PATH modification doesnt error on windows

[gordcorp]

Fix for #25. The tests wouldnt pass on windows, but fixes the error and everything seems to work. Hopefully it works on *nix...

[jnunemaker]

It seems like you are trying to change ENV['PATH'] and then set it back to the original, is that correct? If so, you need to use begin/ensure/end or ENV['PATH'] is going to be left dirty. Also, setting ENV['PATH'] like this changes it for the current process, but I don't think it will for the system call. The point of doing PATH in the system call is so that it changes the ghostscript path that imagemagick uses when the imagemagick command is run.

[gordcorp]

Yep that's what I'm trying to do. The previous method gave an error on windows. Could also change from backticks to system but I thought that would be riskier and wanted something simple, particularly given the tests dont work for me.
The backtick system call is in a subshell so will inherit the environment from ruby. I've confirmed it does for me on windows when I added a echo %PATH%
I've added in the begin/ensure/end.

[jnunemaker]

The backtick system call is in a subshell so will inherit the environment from ruby. I've confirmed it does for me on windows when I added a echo %PATH%

Interesting. I guess if this works then it should be fine. I wish we had a good test case for this actually working the whole way through. We'd probably have to vendor ghostscript and imagemagick and somehow do something that ensured we knew we were using the vendored ghostscript version. Sounds like a pain. :)

@jonmagic any thoughts?

[jonmagic]

@jonmagic any thoughts?

I'd be keen on trying out http://ruby-doc.org/core-2.3.1/Kernel.html#method-i-system first to see if we can modify the ENV for that single command without having to change PATH back in ensure. I used backticks for this back when that was the only way I knew how to do system calls 😁

@BJgordon would you be up for testing out:

command_env = {}
if @ghostscript_path && @ghostscript_path != DefaultGhostScriptPath
  command_env["PATH"] = "#{File.dirname(@ghostscript_path)}#{File::PATH_SEPARATOR}#{ENV['PATH']}"
end

system(command_env, command.join(" "))

I'm curious if Windows will respect it if we run it that way.

[gordcorp]

@jonmagic Yep that code's fine for me on windows.
And ahh I just realised I dont think the original code would have worked as intended: command.unshift("PATH=#{File.dirname(@ghostscript_path)} sets PATH as a shell variable, not in the environment, so even on *nix imagemagick wouldnt see the new PATH. If I'm right, no one has missed this functionality, and would be easiest just to delete the line?

[jnunemaker]

If I'm right, no one has missed this functionality, and would be easiest just to delete the line?

I don't know if you are right, but it definitely makes a difference on SpeakerDeck. We use it currently to alter the version of ghostscript used by imagemagick and have confirmed that it works as we intended.

[jonmagic]

Ok, I think Open3.capture2e is what we want to use here. I think we want to replace these five lines with the following:

command_env = {}
if @ghostscript_path && @ghostscript_path != DefaultGhostScriptPath
  command_env["PATH"] = "#{File.dirname(@ghostscript_path)}#{File::PATH_SEPARATOR}#{ENV['PATH']}"
end

result, status = Open3.capture2e(command_env, command.join(" "))

status.success? || raise(UnprocessablePage, result)

We'll also want to add require "open3" to the top of this file. If we can test this out on Windows and on Speaker Deck to make sure it works for both cases that would be rad.

[gordcorp]

Thanks, it works for me on Windows.

[jonmagic]

@BJgordon if you are up for updating this PR with the suggested changes I can test it with Speaker Deck and then merge if it all works as expected.

[gordcorp]

@jonmagic sure thing will do

[gordcorp]

Done.

[jonmagic]

Tested. Merging and I will cut a new version of Grim as soon as I get a couple of other improvements in.