Project

General

Profile

Actions

Bug #7958

closed

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

Added by drbrain (Eric Hodel) over 11 years ago. Updated over 11 years ago.

Status:
Closed
Target version:
ruby -v:
ruby 2.1.0dev (2013-02-26 trunk 39490) [x86_64-darwin12.2.1]
Backport:
[ruby-core:52890]

Description

=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


Files

fileutils.rb.verbose_install_fix.patch (477 Bytes) fileutils.rb.verbose_install_fix.patch drbrain (Eric Hodel), 02/26/2013 10:24 AM
fileutils.rb.verbose_install_fix.2.patch (23.4 KB) fileutils.rb.verbose_install_fix.2.patch drbrain (Eric Hodel), 02/27/2013 07:14 AM
fileutils.patch (1.81 KB) fileutils.patch trans (Thomas Sawyer), 03/01/2013 05:29 PM

Related issues 2 (0 open2 closed)

Related to Ruby master - Feature #4970: FileUtils refactoredClosed07/04/2011Actions
Related to Backport200 - Backport #7992: Backport r39544 - Fixes FileUtils bug #7958Closednagachika (Tomoyuki Chikanaga)03/01/2013Actions

Updated by ko1 (Koichi Sasada) over 11 years ago

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

Updated by drbrain (Eric Hodel) over 11 years ago

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

It seems this was broken by r34669.

Updated by trans (Thomas Sawyer) over 11 years 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

Updated by drbrain (Eric Hodel) over 11 years 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.

Updated by drbrain (Eric Hodel) over 11 years ago

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

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

Updated by trans (Thomas Sawyer) over 11 years 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.

Actions #8

Updated by drbrain (Eric Hodel) over 11 years 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.

Updated by drbrain (Eric Hodel) over 11 years 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.

Updated by trans (Thomas Sawyer) over 11 years 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.

Updated by drbrain (Eric Hodel) over 11 years 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..

Updated by trans (Thomas Sawyer) over 11 years 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0