Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move all requires to top level and use require instead of require_relative #1026

Closed
wants to merge 1 commit into from

Conversation

@rrosenblum
Copy link

rrosenblum commented May 30, 2017

This fixes #1024.

rrosenblum@C02NV0KSG3QN:~/work/test$ ls -la
total 32
drwxr-xr-x   8 rrosenblum  staff   272 Mar 18 14:12 .
drwxr-xr-x  44 rrosenblum  staff  1496 Mar 17 18:58 ..
drwxr-xr-x   3 rrosenblum  staff   102 Mar 17 18:59 .bundle
-rw-r--r--   1 rrosenblum  staff     6 Mar 17 18:58 .ruby-version
-rw-r--r--   1 rrosenblum  staff    52 Mar 18 14:12 Gemfile
-rw-r--r--   1 rrosenblum  staff   225 Mar 18 14:12 Gemfile.lock
drwxr-xr-x   3 rrosenblum  staff   102 Mar 17 18:59 vendor
lrwxr-xr-x   1 rrosenblum  staff     6 Mar 17 18:59 vendor2 -> vendor
rrosenblum@C02NV0KSG3QN:~/work/test$ cat Gemfile
source 'https://rubygems.org'

gem 'prawn', '2.2.2'
rrosenblum@C02NV0KSG3QN:~/work/test$ bundle config
Settings are listed in order of priority. The top value will be used.
jobs
Set for the current user (/Users/rrosenblum/.bundle/config): "3"

path
Set for your local app (/Users/rrosenblum/work/test/.bundle/config): "vendor2/bundle"
Set for the current user (/Users/rrosenblum/.bundle/config): "vendor/bundle"

disable_shared_gems
Set for your local app (/Users/rrosenblum/work/test/.bundle/config): "true"

rrosenblum@C02NV0KSG3QN:~/work/test$ bundle exec irb
irb(main):001:0> require 'prawn'
/Users/rrosenblum/work/test/vendor2/bundle/ruby/2.3.0/gems/pdf-core-0.7.0/lib/pdf/core/text.rb:12: warning: already initialized constant PDF::Core::Text::VALID_OPTIONS
/Users/rrosenblum/work/test/vendor/bundle/ruby/2.3.0/gems/pdf-core-0.7.0/lib/pdf/core/text.rb:12: warning: previous definition of VALID_OPTIONS was here
/Users/rrosenblum/work/test/vendor2/bundle/ruby/2.3.0/gems/pdf-core-0.7.0/lib/pdf/core/text.rb:13: warning: already initialized constant PDF::Core::Text::MODES
/Users/rrosenblum/work/test/vendor/bundle/ruby/2.3.0/gems/pdf-core-0.7.0/lib/pdf/core/text.rb:13: warning: previous definition of MODES was here
=> true
irb(main):002:0>
@yvbeek
yvbeek approved these changes Jun 22, 2017
@yvbeek

This comment has been minimized.

Copy link

yvbeek commented Jun 22, 2017

@pointlessone Could you please review this pull request?
It would be great to get rid of the annoying warnings (#1024)

@pointlessone

This comment has been minimized.

Copy link
Member

pointlessone commented Jun 22, 2017

I'm on the fence about require vs require_relative. I really don't like moving requires up like this.

@yvbeek

This comment has been minimized.

Copy link

yvbeek commented Jun 22, 2017

@pointlessone Is there an other solution? This is affecting everyone that is using Capistrano or another set up that uses symbolic links.

@fidothe

This comment has been minimized.

Copy link
Contributor

fidothe commented Jun 23, 2017

Having just dug into this, there's a very simple 'fix': simply replace the call to require 'pdf/core/text' in lib/prawn/text.rb with require 'pdf/core', or remove the call completely.

The question I have is: why are things inside pdf-core (i.e. not just pdf-core) being required? Normally I would assume that such a use is because the thing in question is a stand-alone chunk of functionality (à la active_support/blah in Rails).

If this is the case, then another fix (an actual fix) would be to not require or require_relative pdf/core/text from lib/pdf/core.rb in pdf-core.

The main use for require_relative is that, because it skips the $LOAD_PATH lookups, it's much, much, faster than require, particularly if you have a lot of gems installed and aren't using Bundler. That seems to argue for keeping require_relative in pdf-core, but ensuring that things you want to require standalone from pdf-core are not required by default.

If someone would like, I can prepare a PR tomorrow that removes or alters the requires to pdf/core in Prawn to only require pdf/core itself.

@pointlessone

This comment has been minimized.

Copy link
Member

pointlessone commented Jun 23, 2017

@fidothe This is actually a reasonable proposal. PDF/Core is supposed to be stand-alone. The problem is that it was extracted from Prawn and the extraction wasn't quite finished. You may still find that PDF/Core actually depends on Prawn. It's also is not used by any other piece of code other than Prawn itself, as far as I know.

Now, while it's true that require_relative is a bit faster it wasn't the main reason to use. It was mostly convenience. You may notice that some parts of Prawn are relativly deeply nested. It resulted in a bit of a repetition. That is it.

Regarding the whole problem. I'm convinced it's an environment (read "bundler") issue. There's some piece of code that adds both symlinked and real paths to $:. Moreover, it requires Prawn (and most likely all other gems, too) both before and after adding resolved (or symlinked — I don't know the order) paths. Basically, deps are loaded twice and that can lead to all sorts of problems much worse than warnings.

Prawn doesn't do much crazy metaprogramming/init-run stuff. What happens is mostly just consts/methods overwrite. But in Ruby there are definitely much worse things that can go awry with double load.

Anyway, what I'm trying to say is while we can fix the warnings in Prawn it won't fix the root cause.

@yvbeek

This comment has been minimized.

Copy link

yvbeek commented Jun 23, 2017

@fidothe that pull request sounds awesome! Thank you.

@pointlessone I would inform the bundler team what's wrong, but unfortunately this is a bit beyond my knowledge of how ruby includes work.

@fidothe

This comment has been minimized.

Copy link
Contributor

fidothe commented Jun 24, 2017

I'm working up a minimal-test-case to check this out. I don't think (at this point) that there's a bug in Bundler. What I think is happening is that the first thing that requires into pdf-core is getting the symlinked dir for File.expand_path('.') (which is effectively what require_relative uses), and requires from the actual files in pdf-core get the original dir (the target of the symlink), which gives two separate absolute paths, hence the double require.

@gettalong

This comment has been minimized.

Copy link
Member

gettalong commented Jun 24, 2017

@fidothe I agree with @pointlessone that this is primarily not Prawn's problem but probably some invalid behaviour on part of bundler or require or require_relative; see this example:

/tmp  cat start.rb
require 'hallo'
p $".last

/tmp  ll -d hallo test
lrwxrwxrwx 1 thomas thomas    4 Jun 24 14:43 hallo -> test/
drwxrwxr-x 2 thomas thomas 4096 Jun 24 15:03 test/

/tmp  cat test/hallo.rb
puts "before"
p $".last
require_relative 'test.rb'
puts "after"
p $".last

/tmp  cat test/test.rb
puts "test.rb"
p $".last

/tmp  ruby -Itest start.rb
before
"/home/thomas/.rvm/gems/ruby-2.4.0@global/gems/did_you_mean-1.1.0/lib/did_you_mean.rb"
test.rb
"/home/thomas/.rvm/gems/ruby-2.4.0@global/gems/did_you_mean-1.1.0/lib/did_you_mean.rb"
after
"/tmp/test/test.rb"
"/tmp/test/hallo.rb"

/tmp  ruby -Ihallo start.rb
before
"/home/thomas/.rvm/gems/ruby-2.4.0@global/gems/did_you_mean-1.1.0/lib/did_you_mean.rb"
test.rb
"/home/thomas/.rvm/gems/ruby-2.4.0@global/gems/did_you_mean-1.1.0/lib/did_you_mean.rb"
after
"/tmp/test/test.rb"
"/tmp/hallo/hallo.rb"

The last two lines of the output should be the same (in my opinion) but differ.

@fidothe

This comment has been minimized.

Copy link
Contributor

fidothe commented Jun 28, 2017

This is definitely the result of require_relative and require behaving differently with respect to symlinks. I've got a simple example here: https://github.com/fidothe/symlink-require. I've opened a bug on Ruby to see if this is intended behaviour or not. (https://bugs.ruby-lang.org/issues/13695).

@rrosenblum

This comment has been minimized.

Copy link
Author

rrosenblum commented Jun 28, 2017

I am really interested to see what the response from Ruby is on this. Thanks for creating a bug report for Ruby.

@pointlessone

This comment has been minimized.

Copy link
Member

pointlessone commented Jun 29, 2017

Shyouhey (Ruby Core Team) acknowledges this is a bug in Ruby:

I believe the reported behaviour is a bug to be fixed.

More info on Ruby issue tracker: issue 10222

@yvbeek

This comment has been minimized.

Copy link

yvbeek commented Jun 29, 2017

Oh wow, that is a three year old bug.

It would be great though if @fidothe's change could fix the warnings. At the moment Prawn is the only gem that I'm using that is causing these warnings.

I'm all for placing a crown, but an emergency filling for now sounds great too :)

@fidothe

This comment has been minimized.

Copy link
Contributor

fidothe commented Jul 6, 2017

We merged a fix for #1024 that avoids the underlying problem. I'm going to close this PR as a result.

@fidothe fidothe closed this Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.