Use absolute_path of caller_locations to infer engine root

[pointlessone]

According to documentation path only returns file names. On MRI it's not the case but it's likely a bug in MRI. Rubinius tries to implement it according to documentation and Rails get broken on rbx (relevant discussion).

Affected versions of Rails are >= 4.1.0. Would you like me to submit a PR for 4.1 branch, too?

[SamSaffron]

something is astray here, this commit breaks discourse, in particular seeing:

/home/sam/.rbenv/versions/2.1.2.discourse/lib/ruby/gems/2.1.0/gems/railties-4.2.0.rc1/lib/rails/engine.rb:360:in `dirname': no implicit conversion of nil into String (TypeError)
    from /home/sam/.rbenv/versions/2.1.2.discourse/lib/ruby/gems/2.1.0/gems/railties-4.2.0.rc1/lib/rails/engine.rb:360:in `inherited'
    from /home/sam/Source/discourse/plugins/poll/plugin.rb:15:in `<module:PollPlugin>'
    from /home/sam/Source/discourse/plugins/poll/plugin.rb:14:in `block in activate!'

after, which is a simple Engine.

after_initialize do
  # Rails Engine for accepting votes.
  module PollPlugin
    class Engine < ::Rails::Engine
      engine_name "poll_plugin"
      isolate_namespace PollPlugin
    end

[SamSaffron]

also .. why should Engine be exploding just cause it can not get a backtrace, seems rather extreme.

[SamSaffron]

This demos the issue:

class A
  def initialize
    instance_eval 'def b; c; end', File.expand_path(__FILE__)
  end

  def c
    puts caller_locations[0].path.nil?
    # false
    puts caller_locations[0].absolute_path.nil?
    # true
   end
end

A.new.b

absolute_path can be nil in some conditions ... so the code above should be:

  caller_locations.map{|location| location.absolute_path || location.path}

[pointlessone]

Wouldn't path be nil too in cases when absolute_path is nil?

[SamSaffron]

Nope, run my example

On Monday, December 1, 2014, Alexander Mankuta notifications@github.com
wrote:

Wouldn't path be nil too in cases when absolute_path is nil?

—
Reply to this email directly or view it on GitHub
#17782 (comment).

[pointlessone]

This seem to be a Ruby bug.

According to documentation:

Returns the full file path of this frame.

Same as path, but includes the absolute path.

[SamSaffron]

I think the documentation here needs to be corrected instance_eval can send in whatever name it wants even something that is not a file on the filesystem. absolute_path is attempting to get an actual path.

@ko1 is this a bug ? (see code sample above)

cc @zzak

[SamSaffron]

After discussing this with @ko1 I opened https://bugs.ruby-lang.org/issues/10561 feel free to leave your ideas / comments there @cheba

[brixen]

@SamSaffron is there an ETA for 4.1.9?

[brixen]

@SamSaffron also, might it make sense to do a bit of sanity checking here?

rails/railties/lib/rails/engine.rb

Line 360 in f2b1723

File.dirname(call_stack.detect { |p| p !~ %r[railties[\w.-]*/lib/rails|rack[\w.-]*/lib/rack] })

If the resulting path is ".", as was the case here rubinius/rubinius#3223, the behavior of this code is problematic. Considering the issues raised in https://bugs.ruby-lang.org/issues/10561, this Rails code shouldn't unconditionally trust what's coming out of #caller_locations.

[rafaelfranca]

@brixen next week, maybe in the Tuesday.

[SamSaffron]

@brixen @rafaelfranca agree here, we need to be a bit more protective. also in light of 10561 its possible for a frame to be nil (for injected c frames)

we are probably unlikely to hit this in the real world but worth extra protection.

[brixen]

@rafaelfranca are there still plans for a 4.1.9? People are having some issues updating to 4.2 and this bug is a pain for a bunch of applications on 4.1.

[rafaelfranca]

@brixen sure. I was planing to start the release ritual yesterday but life got in the way. I'll try to do it today but Tuesday will be the deadline.

[brixen]

@rafaelfranca ❤️

[rafaelfranca]

@brixen just released the first release candidate including this bug fix. I believe next week we will have the final version

[brixen]

@rafaelfranca many thanks.