Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

fix ImageMagick command in Windows #2332

Closed
wants to merge 13 commits into from
Closed

Conversation

aboutqx
Copy link
Contributor

@aboutqx aboutqx commented Oct 31, 2016

Add OS detect to choose right command.

It seems that paperclip use identify as exe in windows,

Command :: SET PATH=C:\file-5.03-bin\bin;%PATH% & identify -format '%wx%h,%[exif:orientation]' "C:/Users/AppData/Local/Temp/5415c7fe534a63366b48d388f204c32820161028-7156-f2nblm.jpg[0]" 2>NUL.,

while only magick identify command works in windows,and there in no identify.exe in windows in the latest version ImageMagic.
And only Paperclip.options[:command_path] is used, the Paperclip.options[:image_magick_path] seems ignored.

About imageMagick in windows can be found there.

@tute
Copy link
Contributor

tute commented Oct 31, 2016

Thank you, @aboutqx! I like this change.

I know master is now failing in CI for Ruby 2.2, I'm taking care of that. That makes it hard to do what I'll ask, but do you know you can set up paperclip for Windows in Travis CI with a regression test for this? Otherwise, I can't make sure we can reliably maintain this code.

Also, do you think we can abstract out this change to a single place in the codebase, and then call that configuration instead of checking three times whether we are on windows or not?

Thanks so much!

@tute
Copy link
Contributor

tute commented Oct 31, 2016

the Paperclip.options[:image_magick_path] seems ignored.

Is this a bug? If so, we should address it. Thanks!

@@ -98,7 +98,8 @@ def self.options
:swallow_stderr => true,
:content_type_mappings => {},
:use_exif_orientation => true,
:read_timeout => nil
:read_timeout => nil,
:is_windows => Gem.win_platform?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the new Ruby 1.9 hash syntax.
Put a comma after the last item of a multiline hash.

@@ -98,7 +98,8 @@ def self.options
:swallow_stderr => true,
:content_type_mappings => {},
:use_exif_orientation => true,
:read_timeout => nil
:read_timeout => nil,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the new Ruby 1.9 hash syntax.

@aboutqx
Copy link
Contributor Author

aboutqx commented Nov 1, 2016

Glad for your reply.I've make it configurable by adding it to options,but I'm not familiar with Travis CI.

I've made some google,it seems that Windows is not supported in Travis CI right now,and I have no ideas.
Do you have nay solution?I'm fine with it unmerged.
Btw,the Paperclip.options[:image_magick_path] is never used,not sure if it's needed.

@tute
Copy link
Contributor

tute commented Nov 3, 2016

It seems that Windows is not supported in Travis CI right now

huh, we can't test it then. We could tag those tests with windows_platform or alike, ignored by CI, so that people on Windows platforms can run them. But I don't have available window boxes to run them myself and verify the tests and code indeed work.

The Paperclip.options[:image_magick_path] is never used,not sure if it's needed.

Right, thanks! Just dropped the option.

@aboutqx
Copy link
Contributor Author

aboutqx commented Nov 4, 2016

Maybe another way is make the option is_windows default false ,then we'll need no tests on windows,and add a tip about changing it in windows while referencing the doc of ImageMagick.

Right now the error information didn't cover it,just return not idetentified,which is not clear.

@mike-burns
Copy link
Member

Merged as 0d93e0f. Thank you!

@mike-burns mike-burns closed this May 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants