Bug #7958: include FileUtils::Verbose gives NoMethodError when installing files with a different mode

• Status: Closed
• Assignee: drbrain (Eric Hodel)
• Target version: 2.1.0
• ruby -v: ruby 2.1.0dev (2013-02-26 trunk 39490) [x86_64-darwin12.2.1]

[drbrain (Eric Hodel)]

=begin
Seems like (({fu_stream_blksize})) isn't included when (({FileUtils::Verbose})) is. Changing to plain FileUtils works, though.

$ cat test.rb
require 'fileutils'
require 'tmpdir'

include FileUtils::Verbose

Dir.mktmpdir 'test' do |dir|
install FILE, dir, mode: 0600
install FILE, dir, mode: 0640
end

$ ~/.rubies/trunk/bin/ruby -v test.rb
ruby 2.1.0dev (2013-02-26 trunk 39490) [x86_64-darwin12.2.1]
install -c -m 0600 test.rb /var/folders/87/twjsm89x01161gp5d9qwlx2m0000gn/T/test20130225-53176-197q6me
install -c -m 0640 test.rb /var/folders/87/twjsm89x01161gp5d9qwlx2m0000gn/T/test20130225-53176-197q6me
/Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:898:in compare_stream': undefined method fu_stream_blksize' for main:Object (NoMethodError)
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:882:in block (2 levels) in compare_file' from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:881:in open'
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:881:in block in compare_file' from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:880:in open'
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:880:in compare_file' from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:926:in block in install'
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:1620:in block in fu_each_src_dest' from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:1633:in fu_each_src_dest0'
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:1618:in fu_each_src_dest' from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:925:in install'
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/fileutils.rb:137:in install' from test.rb:8:in block in '
from /Users/drbrain/.rubies/trunk/lib/ruby/2.1.0/tmpdir.rb:88:in mktmpdir' from test.rb:6:in '
=end

[ko1 (Koichi Sasada)]

• Assignee set to nobu (Nobuyoshi Nakada)
• Target version set to 2.1.0

[drbrain (Eric Hodel)]

• File fileutils.rb.verbose_install_fix.patch fileutils.rb.verbose_install_fix.patch added

This patch fixes it, but I'm unsure how to test it.

It seems this was broken by r34669.

[trans (Thomas Sawyer)]

=begin
It's b/c of Module Inclusion Problem. StreamUtils_ is included into FileUtils and FileUtils is included in FileUtils::Verbose. So one would expect it to work, but b/c the later include is done first, StreamUtils_ only actually makes it as far as FileUtils.

The simplest solution is probably to move lines 90-125 from the top of the file to the bottom. But that's unfortunate b/c it would make the code less readable.

Eric's patch can work but it might need to add extend StreamUtils_ as well, in order to fully work.

Taking a moment to think about this more in depth, it becomes clear this will be a problem if anyone ever wants to extend FileUtils with some other module (for whatever reason). So perhaps a more robust solution would be to work around the module include problem altogether with an included callback, e.g.

module FileUtils
  def self.include(mod)
    Verbose.send(:include, mod)
    NoWrite.send(:include, mod)
    DryRun.send(:include, mod)

    # re-extend self
    extend self
    Verbose.send(:extend, Verbose)
    NoWrite.send(:extend, NoWrite)
    DryRun.send(:extend, DryRun)
  end
end

See any problems with that approach?
=end

[drbrain (Eric Hodel)]

• File fileutils.rb.verbose_install_fix.2.patch fileutils.rb.verbose_install_fix.2.patch added

It appears that #4970 is the culprit.

I think r34669 should be reverted. Trading one set of metaprogramming for another that requires extra patches seems dangerous.

I don't see how you could extend FileUtils with another module usefully. The existing modules are wrappers around adding default options.

[tenderlovemaking (Aaron Patterson)]

@drbrain (Eric Hodel) it looks good to me!

[drbrain (Eric Hodel)]

• Assignee changed from nobu (Nobuyoshi Nakada) to drbrain (Eric Hodel)

As FileUtils has no maintainer, I will assign this to myself.

[trans (Thomas Sawyer)]

There are reasons for the changes and they are an improvement to code, reverting just because there is an issue that had to be ironed out is anathema to progress of the language.

[drbrain (Eric Hodel)]

• % Done changed from 0 to 100
• Status changed from Open to Closed

This issue was solved with changeset r39544.
Eric, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• lib/fileutils.rb: Revert r34669 which altered the way
  metaprogramming in FileUtils occurred. [ruby-trunk - Bug #7958]

• test/fileutils/visibility_tests.rb: Refactored tests of FileUtils
  options modules to expose bug found in #7958

• test/fileutils/test_dryrun.rb: ditto.

• test/fileutils/test_nowrite.rb: ditto.

• test/fileutils/test_verbose.rb: ditto.

[drbrain (Eric Hodel)]

There's no reason r34669 cannot be fixed and re-applied at a future date.

I cannot determine what user-facing feature r34669 added to FileUtils as define_command was private, so reverting it seemed to be the safest course of action to ensure that FileUtils will work in a future 2.0.0 patchlevel.

[trans (Thomas Sawyer)]

• File fileutils.patch fileutils.patch added

Here is a patch with the fix the r34669 code.

Sorry, I do not know what you mean by "I cannot determine what user-facing feature r34669 added to FileUtils as define_command was private".

If it helps to clarify, the define_command "dsl" method was added and extend self used instead of module_function to facilitate clean extension of FileUtils. The old code made doing so properly cumbersome.

The code passes all the tests, so I do not see why it would need to be reverted. In so far as the tests are lacking, we are never going to know until people put the code to use. Which would be preferable in the Ruby 2.0.0 preview releases rather than later on.

[drbrain (Eric Hodel)]

Your patch is a band-aid, overriding Module#extend is not a good practice, so I can't apply it. The ruby standard libraries are a place to work with ruby, not around it.

This issue is closed.

Please create a new issue with a complete patch that can be evaluated. The goal of this issue is to fix RubyGems installation as quickly as possible as it affects many more people..

[trans (Thomas Sawyer)]

It is not a band-aid. You are not even aware of the code enough to know it is not #extend, but #include. And it's fits precisely to the design of FileUtils. FileUtils is included into FileUtils::Verbose, FileUtils::NoWrite and FileUtils::DryRun, therefore whenever a module is included into FileUtils, FileUtils must also be re-included into those sub-modules b/c of Ruby's well know Inclusion Problem. So it's not working around Ruby at all, but with the very facts of it's design.

I took the time and effort to understand how FileUtils works b/c I have written extensions for FileUtils and have learned the difficulties of doing so because of the old structure of the code. Which is why I took the time and effort to improve the API for the benefit of everyone in the future. You have swept in and summarily issued an edict to revert that work over a single very easily fixable issue, without the first clue of what's really going on with the code as a whole.

It's was clear from the beginning that you had no real interest in what's best for this code, or an appropriate dialog on how to properly address it. Your premature closure of the issue a determent to Ruby. (And for what I suspect to be purely personal and petty reasons, which is exceptionally shameful.) There is no point in a new issue for which I have already provided the solution. You will just ignore it or find another excuse to dismiss it as well, as you have demonstrated by never engaging in an earnest conversation about it to begin with.