Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Paperclip#run now respects swallow_stderr setting. Fix for #741 #742

Merged
merged 1 commit into from Feb 17, 2012

Conversation

Projects
None yet
4 participants

apolzon commented Feb 15, 2012

Paperclip will now merge in the swallow_stderr option when calling Cocaine::CommandLine.new in Paperclip#run.

Not overriding swallow_stderr if it is passed in to Paperclip#run.

apolzon commented Feb 16, 2012

This is a dupe of #741. Not sure what I did to create two copies of this issue, sorry.

Contributor

sikachu commented Feb 17, 2012

Nah, that's fine. It's ok to open one issue and one PR.

sikachu added a commit that referenced this pull request Feb 17, 2012

Merge pull request #742 from apolzon/respect_swallow_stderr_option_wh…
…en_running_commands

Paperclip#run now respects swallow_stderr setting. Fix for #741

@sikachu sikachu merged commit 6fca437 into thoughtbot:master Feb 17, 2012

This is incorrect when options[:swallow_stderr] is true and local_options[:swallow_stderr] is false: in this case, swallow_stderr is forced to be true when we would expect local_options[:swall_stderr] to take precedence.

I guess that it should read:

local_options = local_options.merge(:swallow_stderr => options[:swallow_stderr]) if local_options[:swallow_stderr].nil? && !options[:swallow_stderr].nil?

@mike-burns mike-burns commented on the diff Feb 24, 2012

lib/paperclip.rb
@@ -99,6 +99,7 @@ def run(cmd, arguments = "", local_options = {})
command_path = options[:command_path] || options[:image_magick_path]
Cocaine::CommandLine.path = ( Cocaine::CommandLine.path ? [Cocaine::CommandLine.path, command_path ].flatten : command_path )
local_options = local_options.merge(:logger => logger) if logging? && (options[:log_command] || local_options[:log_command])
+ local_options = local_options.merge(:swallow_stderr => options[:swallow_stderr]) if !local_options[:swallow_stderr] && !options[:swallow_stderr].nil?
@mike-burns

mike-burns Feb 24, 2012

Owner

This is a very long line with an if modifier, which is basically impossible to read. Can you split this down more?

@mike-burns mike-burns commented on the diff Feb 24, 2012

test/paperclip_test.rb
@@ -22,6 +23,18 @@ class PaperclipTest < Test::Unit::TestCase
Paperclip.run("convert", "stuff")
assert_equal [Cocaine::CommandLine.path].flatten.include?("/opt/my_app/bin"), true
end
+
+ should "respect Paperclip.options[:swallow_stderr]" do
+ Paperclip.options[:swallow_stderr] = false
+ Cocaine::CommandLine.unstub(:new)
+ Cocaine::CommandLine.expects(:new).with("convert", "stuff", {:swallow_stderr => false}).returns(stub(:run))
+ Paperclip.run("convert", "stuff")
+
+ Paperclip.options[:swallow_stderr] = true
+ Cocaine::CommandLine.unstub(:new)
+ Cocaine::CommandLine.expects(:new).with("convert", "stuff", {:swallow_stderr => true}).returns(stub(:run))
+ Paperclip.run("convert", "stuff")
+ end
@mike-burns

mike-burns Feb 24, 2012

Owner

I'm seeing this failure:

% rake
>> bundle install --gemfile=/home/mike/lib/paperclip/gemfiles/rails2.gemfile
Using rake (0.9.2.2) 
Using activesupport (2.3.14) 
Using rack (1.1.2) 
Using actionpack (2.3.14) 
Using actionmailer (2.3.14) 
Using activerecord (2.3.14) 
Using activeresource (2.3.14) 
Using bundler (1.0.21) 
Using appraisal (0.4.0) 
Using ffi (1.0.11) 
Using childprocess (0.2.3) 
Using builder (3.0.0) 
Using diff-lcs (1.1.3) 
Using json (1.6.3) 
Using gherkin (2.7.1) 
Using term-ansicolor (1.0.7) 
Using cucumber (1.1.4) 
Using rspec-core (2.7.1) 
Using rspec-expectations (2.7.0) 
Using rspec-mocks (2.7.0) 
Using rspec (2.7.0) 
Using aruba (0.4.9) 
Using multi_json (1.0.4) 
Using multi_xml (0.4.1) 
Using httparty (0.8.1) 
Using nokogiri (1.5.0) 
Using uuidtools (2.1.2) 
Using aws-sdk (1.2.5) 
Using mime-types (1.17.2) 
Using rack-test (0.6.1) 
Using rubyzip (0.9.5) 
Using selenium-webdriver (2.15.0) 
Using xpath (0.1.4) 
Using capybara (1.1.2) 
Using cocaine (0.2.1) 
Using excon (0.7.12) 
Using fakeweb (1.3.0) 
Using formatador (0.2.1) 
Using net-ssh (2.2.1) 
Using net-scp (1.0.4) 
Using ruby-hmac (0.4.0) 
Using fog (1.1.1) 
Using metaclass (0.0.1) 
Using mocha (0.10.0) 
Using paperclip (2.6.0) from source at /home/mike/lib/paperclip 
Using rails (2.3.14) 
Using shoulda (2.11.3) 
Using sqlite3 (1.3.5) 
Your bundle is complete! Use `bundle show [gemname]` to see where a bundled gem is installed.
>> bundle install --gemfile=/home/mike/lib/paperclip/gemfiles/rails3.gemfile
Using rake (0.9.2.2) 
Using abstract (1.0.0) 
Using activesupport (3.0.11) 
Using builder (2.1.2) 
Using i18n (0.5.0) 
Using activemodel (3.0.11) 
Using erubis (2.6.6) 
Using rack (1.2.4) 
Using rack-mount (0.6.14) 
Using rack-test (0.5.7) 
Using tzinfo (0.3.31) 
Using actionpack (3.0.11) 
Using mime-types (1.17.2) 
Using polyglot (0.3.3) 
Using treetop (1.4.10) 
Using mail (2.2.19) 
Using actionmailer (3.0.11) 
Using arel (2.0.10) 
Using activerecord (3.0.11) 
Using activeresource (3.0.11) 
Using bundler (1.0.21) 
Using appraisal (0.4.0) 
Using ffi (1.0.11) 
Using childprocess (0.2.3) 
Using diff-lcs (1.1.3) 
Using json (1.6.3) 
Using gherkin (2.7.1) 
Using term-ansicolor (1.0.7) 
Using cucumber (1.1.4) 
Using rspec-core (2.7.1) 
Using rspec-expectations (2.7.0) 
Using rspec-mocks (2.7.0) 
Using rspec (2.7.0) 
Using aruba (0.4.9) 
Using multi_json (1.0.4) 
Using multi_xml (0.4.1) 
Using httparty (0.8.1) 
Using nokogiri (1.5.0) 
Using uuidtools (2.1.2) 
Using aws-sdk (1.2.5) 
Using rubyzip (0.9.5) 
Using selenium-webdriver (2.15.0) 
Using xpath (0.1.4) 
Using capybara (1.1.2) 
Using cocaine (0.2.1) 
Using excon (0.7.12) 
Using fakeweb (1.3.0) 
Using formatador (0.2.1) 
Using net-ssh (2.2.1) 
Using net-scp (1.0.4) 
Using ruby-hmac (0.4.0) 
Using fog (1.1.1) 
Using metaclass (0.0.1) 
Using mocha (0.10.0) 
Using paperclip (2.6.0) from source at /home/mike/lib/paperclip 
Using rdoc (3.12) 
Using thor (0.14.6) 
Using railties (3.0.11) 
Using rails (3.0.11) 
Using shoulda (2.11.3) 
Using sqlite3 (1.3.5) 
Your bundle is complete! Use `bundle show [gemname]` to see where a bundled gem is installed.
>> bundle install --gemfile=/home/mike/lib/paperclip/gemfiles/rails3_1.gemfile
Using rake (0.9.2.2) 
Using multi_json (1.0.4) 
Using activesupport (3.1.3) 
Using builder (3.0.0) 
Using i18n (0.6.0) 
Using activemodel (3.1.3) 
Using erubis (2.7.0) 
Using rack (1.3.5) 
Using rack-cache (1.1) 
Using rack-mount (0.8.3) 
Using rack-test (0.6.1) 
Using hike (1.2.1) 
Using tilt (1.3.3) 
Using sprockets (2.0.3) 
Using actionpack (3.1.3) 
Using mime-types (1.17.2) 
Using polyglot (0.3.3) 
Using treetop (1.4.10) 
Using mail (2.3.0) 
Using actionmailer (3.1.3) 
Using arel (2.2.1) 
Using tzinfo (0.3.31) 
Using activerecord (3.1.3) 
Using activeresource (3.1.3) 
Using bundler (1.0.21) 
Using appraisal (0.4.0) 
Using ffi (1.0.11) 
Using childprocess (0.2.3) 
Using diff-lcs (1.1.3) 
Using json (1.6.3) 
Using gherkin (2.7.1) 
Using term-ansicolor (1.0.7) 
Using cucumber (1.1.4) 
Using rspec-core (2.7.1) 
Using rspec-expectations (2.7.0) 
Using rspec-mocks (2.7.0) 
Using rspec (2.7.0) 
Using aruba (0.4.9) 
Using multi_xml (0.4.1) 
Using httparty (0.8.1) 
Using nokogiri (1.5.0) 
Using uuidtools (2.1.2) 
Using aws-sdk (1.2.5) 
Using rubyzip (0.9.5) 
Using selenium-webdriver (2.15.0) 
Using xpath (0.1.4) 
Using capybara (1.1.2) 
Using cocaine (0.2.1) 
Using excon (0.7.12) 
Using fakeweb (1.3.0) 
Using formatador (0.2.1) 
Using net-ssh (2.2.1) 
Using net-scp (1.0.4) 
Using ruby-hmac (0.4.0) 
Using fog (1.1.1) 
Using metaclass (0.0.1) 
Using mocha (0.10.0) 
Using paperclip (2.6.0) from source at /home/mike/lib/paperclip 
Using rack-ssl (1.3.2) 
Using rdoc (3.12) 
Using thor (0.14.6) 
Using railties (3.1.3) 
Using rails (3.1.3) 
Using shoulda (2.11.3) 
Using sqlite3 (1.3.5) 
Your bundle is complete! Use `bundle show [gemname]` to see where a bundled gem is installed.
>> bundle install --gemfile=/home/mike/lib/paperclip/gemfiles/rails3_2.gemfile
Using rake (0.9.2.2) 
Using i18n (0.6.0) 
Using multi_json (1.1.0) 
Using activesupport (3.2.1) 
Using builder (3.0.0) 
Using activemodel (3.2.1) 
Using erubis (2.7.0) 
Using journey (1.0.3) 
Using rack (1.4.1) 
Using rack-cache (1.1) 
Using rack-test (0.6.1) 
Using hike (1.2.1) 
Using tilt (1.3.3) 
Using sprockets (2.1.2) 
Using actionpack (3.2.1) 
Using mime-types (1.17.2) 
Using polyglot (0.3.3) 
Using treetop (1.4.10) 
Using mail (2.4.1) 
Using actionmailer (3.2.1) 
Using arel (3.0.2) 
Using tzinfo (0.3.31) 
Using activerecord (3.2.1) 
Using activeresource (3.2.1) 
Using bundler (1.0.21) 
Using appraisal (0.4.1) 
Using ffi (1.0.11) 
Using childprocess (0.3.1) 
Using diff-lcs (1.1.3) 
Using json (1.6.5) 
Using gherkin (2.9.0) 
Using term-ansicolor (1.0.7) 
Using cucumber (1.1.9) 
Using rspec-core (2.8.0) 
Using rspec-expectations (2.8.0) 
Using rspec-mocks (2.8.0) 
Using rspec (2.8.0) 
Using aruba (0.4.11) 
Using multi_xml (0.4.1) 
Using httparty (0.8.1) 
Using nokogiri (1.5.0) 
Using uuidtools (2.1.2) 
Using aws-sdk (1.3.5) 
Using json_pure (1.6.5) 
Using rubyzip (0.9.6.1) 
Using selenium-webdriver (2.13.0) 
Using xpath (0.1.4) 
Using capybara (1.1.2) 
Using cocaine (0.2.1) 
Using excon (0.6.6) 
Using fakeweb (1.3.0) 
Using formatador (0.2.1) 
Using net-ssh (2.3.0) 
Using net-scp (1.0.4) 
Using ruby-hmac (0.4.0) 
Using fog (0.9.0) 
Using metaclass (0.0.1) 
Using mocha (0.10.4) 
Using paperclip (2.6.0) from source at /home/mike/lib/paperclip 
Using rack-ssl (1.3.2) 
Using rdoc (3.12) 
Using thor (0.14.6) 
Using railties (3.2.1) 
Using rails (3.2.1) 
Using shoulda (2.11.3) 
Using sqlite3 (1.3.5) 
Your bundle is complete! Use `bundle show [gemname]` to see where a bundled gem is installed.
>> BUNDLE_GEMFILE=/home/mike/lib/paperclip/gemfiles/rails2.gemfile bundle exec /usr/bin/rake test cucumber
/usr/bin/ruby1.8 -I"lib:lib:profile" -I"/usr/lib/ruby/gems/1.8/gems/rake-0.9.2.2/lib" "/usr/lib/ruby/gems/1.8/gems/rake-0.9.2.2/lib/rake/rake_test_loader.rb" "test/**/*_test.rb" 
Testing against version 2.3.14
debugger disabled
Loaded suite /usr/lib/ruby/gems/1.8/gems/rake-0.9.2.2/lib/rake/rake_test_loader
Started
.......................................................................................................................................................................................................................................................................................[DEPRECATION] validates_attachment_thumbnail is deprecated. This validation is on by default and will be removed from future versions. If you wish to turn it off, supply :whiny => false in your definition.
........Permission denied - /home/mike/lib/paperclip/public/system/avatars/1/original/5k.png - skipping file
.Permission denied - /home/mike/lib/paperclip/public/system/avatars/1/original/5k.png - skipping file
.........................................................................................F...................identify: improper image header `/tmp/stream20120224-13438-103paau-0.png' @ error/png.c/ReadPNGImage/2957.
.identify: improper image header `/tmp/stream20120224-13438-s39bkb-0.png' @ error/png.c/ReadPNGImage/2957.
...................................................................................................sh: convert: not found
..............convert: unrecognized option `-this-aint-no-option' @ error/convert.c/ConvertImageCommand/2777.
.sh: convert: not found
....convert: unrecognized option `-this-aint-no-option' @ error/convert.c/ConvertImageCommand/2777.
..................................................................................
Finished in 42.885945 seconds.

  1) Failure:
test: Calling Paperclip.run should respect Paperclip.options[:swallow_stderr]. (PaperclipTest)
    [/home/mike/lib/paperclip/test/paperclip_test.rb:8:in `__bind_1330096671_529352'
     /usr/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:400:in `call'
     /usr/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:400:in `run_current_setup_blocks'
     /usr/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:399:in `each'
     /usr/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:399:in `run_current_setup_blocks'
     /usr/lib/ruby/gems/1.8/gems/shoulda-2.11.3/lib/shoulda/context.rb:381:in `test: Calling Paperclip.run should respect Paperclip.options[:swallow_stderr]. ']:
not all expectations were satisfied
unsatisfied expectations:
- expected exactly once, not yet invoked: Cocaine::CommandLine.new('convert', 'stuff', {})
satisfied expectations:
- allowed any number of times, not yet invoked: #<Mock:Rails>.env(any_parameters)
- allowed any number of times, not yet invoked: #<Mock:Rails>.root(any_parameters)
- allowed any number of times, not yet invoked: #<Mock:0x7f73ad9d1b88>.run(any_parameters)
- expected exactly once, invoked once: Cocaine::CommandLine.new('convert', 'stuff', {:swallow_stderr => false})
- allowed any number of times, invoked once: #<Mock:0x7f73ad9cede8>.run(any_parameters)
- expected exactly once, invoked once: Cocaine::CommandLine.new('convert', 'stuff', {:swallow_stderr => true})
- allowed any number of times, invoked once: #<Mock:0x7f73ad9c9938>.run(any_parameters)

598 tests, 1042 assertions, 1 failures, 0 errors
rake aborted!
Command failed with status (1): [/usr/bin/ruby1.8 -I"lib:lib:profile" -I"/u...]

Tasks: TOP => test
(See full trace by running task with --trace)
Owner

mike-burns commented Feb 24, 2012

I reverted this because the test fails.

Note to those with commit access: don't pull ugly code.

Contributor

sikachu commented Feb 24, 2012

Ok then.

apolzon commented Feb 24, 2012

Sorry guys. Was trying to get it working changing the minimal amount of code. I can fix it up and re-push. Not sure what caused those test failures; I ran the tests locally and all passed (and saw it pull gemfiles for all tested rails versions).

As a side-note, I've worked around this temporarily by using AS3.1's quietly{} Kernel method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment