Bug #7958

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

Added by Eric Hodel about 1 year ago. Updated about 1 year ago.

[ruby-core:52890]
Status:Closed
Priority:Normal
Assignee:Eric Hodel
Category:lib
Target version:2.1.0
ruby -v:ruby 2.1.0dev (2013-02-26 trunk 39490) [x86_64-darwin12.2.1] Backport:

Description

=begin
Seems like (({fustreamblksize})) 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) [x8664-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 methodfu
streamblksize' 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
eachsrcdest0'
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

fileutils.rb.verbose_install_fix.patch Magnifier (477 Bytes) Eric Hodel, 02/26/2013 10:24 AM

fileutils.rb.verbose_install_fix.2.patch Magnifier (23.4 KB) Eric Hodel, 02/27/2013 07:14 AM

fileutils.patch Magnifier (1.81 KB) Thomas Sawyer, 03/01/2013 05:29 PM


Related issues

Related to ruby-trunk - Feature #4970: FileUtils refactored Closed 07/04/2011
Related to Backport200 - Backport #7992: Backport r39544 - Fixes FileUtils bug #7958 Closed 03/01/2013

Associated revisions

Revision 39544
Added by Eric Hodel about 1 year ago

  • 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.

History

#1 Updated by Koichi Sasada about 1 year ago

  • Assignee set to Nobuyoshi Nakada
  • Target version set to 2.1.0

#2 Updated by Eric Hodel about 1 year ago

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

It seems this was broken by r34669.

#3 Updated by Thomas Sawyer about 1 year ago

=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

#4 Updated by Eric Hodel about 1 year ago

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.

#5 Updated by Aaron Patterson about 1 year ago

@drbrain it looks good to me!

#6 Updated by Eric Hodel about 1 year ago

  • Assignee changed from Nobuyoshi Nakada to Eric Hodel

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

#7 Updated by Thomas Sawyer about 1 year ago

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.

#8 Updated by Eric Hodel about 1 year ago

  • % 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.

#9 Updated by Eric Hodel about 1 year ago

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.

#10 Updated by Thomas Sawyer about 1 year ago

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.

#11 Updated by Eric Hodel about 1 year ago

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..

#12 Updated by Thomas Sawyer about 1 year ago

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.

Also available in: Atom PDF