danini-the-panini / mittsu

3D Graphics Library for Ruby.
https://github.com/danini-the-panini/mittsu
MIT License
508 stars 33 forks source link

On Ruby 2.7.2 require 'mittsu' (0.3.2) raises a NameError #94

Closed javier-sy closed 3 years ago

javier-sy commented 3 years ago

Hello jellyman,

First of all, thanks for your work on mittsu project! It's a very helpful gem for me to display 3d geometrical transformations of some music material.

I've found the next error that raises on require 'mittsu' (tested: mittsu 0.3.2, ruby 2.7.2) when there is a not used refinement on Object (and on Math per extension).

Example with a refinement on to_s method:

module RefineObject
  refine Object do
    def to_s
      super
      # do some other things....
    end
  end
end

require 'mittsu'

Raises this:

12: from /Volumes/Base/Composición/Proyectos/music/10 On Composition-Production/2020-10-05 Espiral [musa]/musa/graphic-support.rb:19:in `<top (required)>'
    11: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:156:in `require'
    10: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:168:in `rescue in require'
     9: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:168:in `require'
     8: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu.rb:3:in `<top (required)>'
     7: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
     6: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
     5: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:22:in `<top (required)>'
     4: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:23:in `<module:Mittsu>'
     3: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `<module:Math>'
     2: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `each'
     1: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `block in <module:Math>'
/Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `public_class_method': undefined method `to_s' for class `#<Class:Mittsu::Math>' (NameError)

I think it is due to some Ruby internal trickery to implement refinements.

I have found a bugfix that resolves the problem (rake tests passed):

On lib/mittsu/math.rb replace BuiltInMath.methods by BuiltInMath.instance_methods (also it seems using instance_methods is more well fitted to your intention of replicating standard Math module methods)

BuiltInMath = Math

module Mittsu
  module Math
    extend BuiltInMath
    include BuiltInMath
    ### BuiltInMath.methods.each { |m| public_class_method m }
    BuiltInMath.instance_methods.each { |m| public_class_method m }

#....

If you prefer I can make a mod and send you a PR. Please, let me know what do you think.

Regards. Javier

danini-the-panini commented 3 years ago

@javier-sy I'm struggling to reproduce this issue. I'm testing on macOS Catalina. Could you provide a minimal reproducing example in a repo?

danini-the-panini commented 3 years ago

@javier-sy okay I've added 2.7 to the test matrix and it seems to only happen on linux

danini-the-panini commented 3 years ago

Nevermind, seems the 2.7 error is due to something else :/

danini-the-panini commented 3 years ago

@javier-sy okay so I'm back to square one, all the tests are passing it seems: #95

javier-sy commented 3 years ago

Hello @jellymann , the original error raised for my with a refinement on dup method instead of to_s method. After that I tried some other methods (to_s) that also failed but now I can't reproduce the problem with to_s. I can reproduce the problem with dup or clone methods:

test.rb:

module RefineObject
  refine Object do
    def clone
      super
      # do some other things....
    end
  end
end

require 'mittsu'

(Sorry, probably I missed some test save that confussed me)

Can you reproduce the error with clone or dup?

Also, it is important to have the refinement defined before require 'mittsu'. If require 'mittsu' is before the refinement the error doesn't happen.

danini-the-panini commented 3 years ago

@javier-sy weird. I tried with clone and I still didn't get any errors

javier-sy commented 3 years ago

@jellymann , ultraweird... now it doesn't happen to me neither.

A few minutes ago: Captura de pantalla 2020-11-13 a las 12 25 01

Just now:

Captura de pantalla 2020-11-13 a las 12 33 12

I don't understand what's happening.

javier-sy commented 3 years ago

Sorry!!! I understand what happened on the second ruby test.rb. I left a require 'mittsu' on the first line on test.rb. Sorry, I'm on my work time and trying to guess the problem at the same time I'm on videoconference with my work team and I have lost my focus.

javier-sy commented 3 years ago

@jellymann, I've checked out branch ds-fix-27 and I've changed test_mittsu.rb to refine clone instead of to_s.

When I call rake test there are no errors.

On the same test folder I've put a file manual-test.rb:

module RefineObject
  refine Object do
    def clone
      super
      # do some other things....
    end
  end
end

require 'mittsu'

and run it with:

ruby manual-test.rb

And it fails with:

Traceback (most recent call last):
    2: from manual-test.rb:10:in `<main>'
    1: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:92:in `require'
/Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:92:in `require': cannot load such file -- mittsu (LoadError)
    12: from manual-test.rb:10:in `<main>'
    11: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:156:in `require'
    10: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:168:in `rescue in require'
     9: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:168:in `require'
     8: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu.rb:3:in `<top (required)>'
     7: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
     6: from /Users/javier/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72:in `require'
     5: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:22:in `<top (required)>'
     4: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:23:in `<module:Mittsu>'
     3: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `<module:Math>'
     2: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `each'
     1: from /Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `block in <module:Math>'
/Users/javier/.rvm/gems/ruby-2.7.2/gems/mittsu-0.3.2/lib/mittsu/math.rb:26:in `public_class_method': undefined method `clone' for class `#<Class:Mittsu::Math>' (NameError)

It seems there is a difference between executing with rake and executing only with ruby. In my tests, if the require 'mittsu' is before (not after) the refinement the error doesn't happen.

Probably rake loads all mittsu code before starting tests and for this reason the refinement doesn't affect mittsu code that executes on loading (as is BuiltInMath.methods.each { |m| public_class_method m } in module definitions inside math.rb).

Could you test directly with ruby manual-test.rb instead of with rake?

danini-the-panini commented 3 years ago

Ah I see, I've managed to reproduce the issue now

danini-the-panini commented 3 years ago

@javier-sy I figured including Ruby Maths module into Mittsu's Math module is probably more trouble than it's worth, so I've just taken out that whole piece of logic and painstakingly prepended all the Math calls with :: -> #95

javier-sy commented 3 years ago

@jellymann It sounds a good approach. Thank you!