Make sure rubygems/package can be directly required reliably #3670

bronzdoc · 2020-05-31T06:08:44Z

Description:

closes #3650

I will abide by the code of conduct.

deivid-rodriguez · 2020-05-31T08:23:15Z

Maybe we can add a little test of requiring rubygems/package on a ruby started with the --disable-gems flag.

bronzdoc · 2020-05-31T15:27:24Z

I just tested this and module Gem; end Is not fixing anything in this case. I still see the same error reported.

If I put it before every require I hit:

Traceback (most recent call last):
        5: from -e:1:in `<main>'
        4: from -e:1:in `require'
        3: from /Users/bronzdoc/projects/rubygems/lib/rubygems/package.rb:49:in `<top (required)>'
        2: from /Users/bronzdoc/projects/rubygems/lib/rubygems/package.rb:49:in `require'
        1: from /Users/bronzdoc/projects/rubygems/lib/rubygems/specification.rb:39:in `<top (required)>'
/Users/bronzdoc/projects/rubygems/lib/rubygems/specification.rb:158:in `<class:Specification>': uninitialized constant Gem::VERSION (NameError)

But doing require "rubygems" fixes it as expected...

So maybe we should require "rubygems" do you see any problem with this @deivid-rodriguez ?

deivid-rodriguez · 2020-05-31T15:32:19Z

That's weird because specification.rb requires rubygems/version at the top, so I think your $LOAD_PATH setup might be incorrect.

Either way, we should change our internal requires to use require_relative.

bronzdoc · 2020-05-31T15:37:48Z

does this works for you ruby -v --disable-gems -I "lib" -e 'require "rubygems/package"'
With the change in this PR?

deivid-rodriguez · 2020-05-31T15:46:36Z

Oh, right! I always get confused with this. In the case of rubygems, Gem::VERSION is not defined in rubygems/version.rb but in rubygems.rb. So, yeah, in this case, it sounds like we should require "rubygems" indeed.

deivid-rodriguez · 2020-05-31T15:47:44Z

@eregon Thoughts?

eregon · 2020-05-31T17:59:42Z

Interesting.
ruby -v --disable-gems -e 'require "rubygems/package"' works for me with the module Gem; end in deprecate.rb with Ruby 2.6.6 and default RubyGems 3.0.3.
And it indeed fails with RubyGems master + module Gem; end:

$ ruby -v --disable-gems -I "lib" -e 'require "rubygems/package"'
ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-linux]
Traceback (most recent call last):
	5: from -e:1:in `<main>'
	4: from -e:1:in `require'
	3: from /home/eregon/code/rubygems/lib/rubygems/package.rb:46:in `<top (required)>'
	2: from /home/eregon/code/rubygems/lib/rubygems/package.rb:46:in `require'
	1: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:39:in `<top (required)>'
/home/eregon/code/rubygems/lib/rubygems/specification.rb:158:in `<class:Specification>': uninitialized constant Gem::VERSION (NameError)

So the only reason require "rubygems/package"' works with current RubyGems master is that RubyGems is loaded by default.

I agree we should just require the full RubyGems here, but that introduces a cycle if we add it in deprecate.rb because rubygems.rb requires deprecate.rb:
https://github.com/rubygems/rubygems/blob/d841d769027382fe80ec1f49711311355a2a575b/lib/rubygems.rb#L19
And so it fails too by adding require 'rubygems' in deprecate.rb:

$ ruby -v --disable-gems -I "lib" -e 'require "rubygems/package"'
ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-linux]
/home/eregon/code/rubygems/lib/rubygems.rb:19: warning: /home/eregon/code/rubygems/lib/rubygems.rb:19: warning: loading in progress, circular require considered harmful - /home/eregon/code/rubygems/lib/rubygems/deprecate.rb
	from -e:1:in  `<main>'
	from -e:1:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/package.rb:45:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/package.rb:45:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/security.rb:8:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/security.rb:8:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/exceptions.rb:3:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/exceptions.rb:3:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/deprecate.rb:24:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/deprecate.rb:24:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems.rb:19:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems.rb:19:in  `require'

/home/eregon/code/rubygems/lib/rubygems/requirement.rb:3: warning: /home/eregon/code/rubygems/lib/rubygems/requirement.rb:3: warning: loading in progress, circular require considered harmful - /home/eregon/code/rubygems/lib/rubygems/deprecate.rb
	from -e:1:in  `<main>'
	from -e:1:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/package.rb:45:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/package.rb:45:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/security.rb:8:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/security.rb:8:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/exceptions.rb:3:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/exceptions.rb:3:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/deprecate.rb:24:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/deprecate.rb:24:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems.rb:115:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems.rb:1334:in  `<module:Gem>'
	from /home/eregon/code/rubygems/lib/rubygems.rb:1334:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/specification.rb:10:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/specification.rb:10:in  `require'
	from /home/eregon/code/rubygems/lib/rubygems/requirement.rb:3:in  `<top (required)>'
	from /home/eregon/code/rubygems/lib/rubygems/requirement.rb:3:in  `require'

Traceback (most recent call last):
	20: from -e:1:in `<main>'
	19: from -e:1:in `require'
	18: from /home/eregon/code/rubygems/lib/rubygems/package.rb:45:in `<top (required)>'
	17: from /home/eregon/code/rubygems/lib/rubygems/package.rb:45:in `require'
	16: from /home/eregon/code/rubygems/lib/rubygems/security.rb:8:in `<top (required)>'
	15: from /home/eregon/code/rubygems/lib/rubygems/security.rb:8:in `require'
	14: from /home/eregon/code/rubygems/lib/rubygems/exceptions.rb:3:in `<top (required)>'
	13: from /home/eregon/code/rubygems/lib/rubygems/exceptions.rb:3:in `require'
	12: from /home/eregon/code/rubygems/lib/rubygems/deprecate.rb:24:in `<top (required)>'
	11: from /home/eregon/code/rubygems/lib/rubygems/deprecate.rb:24:in `require'
	10: from /home/eregon/code/rubygems/lib/rubygems.rb:115:in `<top (required)>'
	 9: from /home/eregon/code/rubygems/lib/rubygems.rb:1334:in `<module:Gem>'
	 8: from /home/eregon/code/rubygems/lib/rubygems.rb:1334:in `require'
	 7: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:10:in `<top (required)>'
	 6: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:10:in `require'
	 5: from /home/eregon/code/rubygems/lib/rubygems/requirement.rb:12:in `<top (required)>'
	 4: from /home/eregon/code/rubygems/lib/rubygems/requirement.rb:37:in `<class:Requirement>'
	 3: from /home/eregon/code/rubygems/lib/rubygems/version.rb:206:in `new'
	 2: from /home/eregon/code/rubygems/lib/rubygems/version.rb:206:in `new'
	 1: from /home/eregon/code/rubygems/lib/rubygems/version.rb:214:in `initialize'
/home/eregon/code/rubygems/lib/rubygems/version.rb:174:in `correct?': uninitialized constant Gem::Deprecate (NameError)

EDIT: that's over-complicated, because package.rb is not loaded by rubygems.rb, see comment below.
So I think we need to move package.rb to package_impl.rb (or some other "private" name), replace all references inside RubyGems requiring package.rb to require package_impl.rb instead (to avoid cyclic require), and then package.rb would then just be require 'rubygems'.
As a bonus, that would also work on TruffleRuby where Gem is an autoload.

eregon · 2020-05-31T18:28:15Z

I see, just require 'rubygems' in package.rb actually solves it for require "rubygems/package", because that's not required by rubygems.rb.
I think that's a good and simple solution 👍

I initially tried to solve the problem for deprecate.rb, because many files do require 'rubygems/deprecate' and I'm not sure how many are "valid" entry points.

Note that require "rubygems/specification" doesn't work though:

$ ruby --disable-gems -I "lib" -e 'require "rubygems/specification"'
Traceback (most recent call last):
	4: from -e:1:in `<main>'
	3: from -e:1:in `require'
	2: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:9:in `<top (required)>'
	1: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:9:in `require'
/home/eregon/code/rubygems/lib/rubygems/version.rb:152:in `<top (required)>': uninitialized constant Gem (NameError)

Not sure if that's a proper entry point, but it's used in 636 + 47 gems,
and also in Bundler itself:
https://github.com/rubygems/rubygems/blob/d841d769027382fe80ec1f49711311355a2a575b/bundler/lib/bundler/rubygems_ext.rb#L5

rubygems/specification.rb is required by rubygems.rb so we cannot just require "rubygems" there:

$ ruby --disable-gems -I "lib" -e 'require "rubygems/specification"'
Traceback (most recent call last):
	4: from -e:1:in `<main>'
	3: from -e:1:in `require'
	2: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:9:in `<top (required)>'
	1: from /home/eregon/code/rubygems/lib/rubygems/specification.rb:9:in `require'
/home/eregon/code/rubygems/lib/rubygems.rb:1358:in `<top (required)>': uninitialized constant Gem::Specification (NameError)

module Gem; VERSION = "3.2.0.pre1".freeze; end in version.rb (just to try) gets further but then:

$ ruby --disable-gems -I "lib" -e 'require "rubygems/specification"'
Traceback (most recent call last):
	2: from -e:1:in `<main>'
	1: from -e:1:in `require'
/home/eregon/code/rubygems/lib/rubygems/specification.rb:2673:in `<top (required)>': undefined method `clear_paths' for Gem:Module (NoMethodError)

And clear_paths is defined in rubygems.rb.

So that one seems hard and I'd tend to say require "rubygems/specification" is not a supported entry point (without first a require 'rubygems' in user code, or relying on RubyGems being loaded by default, and then that require is useless), because it's already required by require "rubygems" and so it is cyclic.

A possible fix could be to split rubygems.rb in smaller 2 parts, and require the first part in specification.rb, and the second part requires specification.rb + code depending on it, and rubygems.rb requires both parts. But (!) Gem.clear_paths is called in specification.rb and it does Gem::Specification.reset so this seems highly cyclic and problematic, and we'd likely need to move or remove that call to Gem.clear_paths (it's suspicious anyway, would probably be better in rubygems.rb). Anyway I give up for now on that one, it's not a good idea to require "rubygems/specification" in user code.

eregon · 2020-05-31T18:37:51Z

I updated the above, I only searched for require "rubygems/specification" but there are a lot more require 'rubygems/specification' and it's even used in Bundler.

deivid-rodriguez · 2020-05-31T19:04:13Z

Let's fix this one for now then by require'ing rubygems on top of rubygems/package. We can deal with the other potential issues @eregon mentioned when they bite someone.

eregon · 2020-05-31T19:05:24Z

Agreed 👍

bronzdoc · 2020-06-02T03:25:10Z

test/rubygems/test_project_sanity.rb

deivid-rodriguez

Found a little issue with the require location. Other than that is great!

test/rubygems/test_project_sanity.rb

deivid-rodriguez

Awesome!

* Partially reverts 8962ca0 * See #2014 and ruby/rubygems#3650 and ruby/rubygems#3670

bundlerbot added the RubyGems label May 31, 2020

Make sure rubygems/package can be directly required reliably

73c199b

bronzdoc force-pushed the reliable-rubygems-package branch from aaa31b0 to 73c199b Compare June 2, 2020 03:08

deivid-rodriguez reviewed Jun 2, 2020

View reviewed changes

test/rubygems/test_project_sanity.rb Outdated Show resolved Hide resolved

Use ruby_with_rubygems_in_load_path helper

5e6d82b

bronzdoc force-pushed the reliable-rubygems-package branch from a8abe79 to 5e6d82b Compare June 3, 2020 04:55

deivid-rodriguez suggested changes Jun 3, 2020

View reviewed changes

test/rubygems/test_project_sanity.rb Outdated Show resolved Hide resolved

Move require to the top

e6cabc3

eregon approved these changes Jun 3, 2020

View reviewed changes

deivid-rodriguez approved these changes Jun 3, 2020

View reviewed changes

bronzdoc merged commit a00c3d6 into master Jun 3, 2020

bronzdoc deleted the reliable-rubygems-package branch June 3, 2020 18:29

graalvmbot pushed a commit to truffleruby/truffleruby that referenced this pull request Jun 23, 2020

Adopt the better fix for require 'rubygems/package' from upstream

964ed03

* Partially reverts 8962ca0 * See #2014 and ruby/rubygems#3650 and ruby/rubygems#3670

Irakaj93 mentioned this pull request Dec 26, 2020

Error occurs everytime i try to create a new rails app #4197

Closed

Uh oh!

Uh oh!

Make sure rubygems/package can be directly required reliably #3670

Make sure rubygems/package can be directly required reliably #3670

Uh oh!

Conversation

bronzdoc commented May 31, 2020

Description:

Uh oh!

deivid-rodriguez commented May 31, 2020

Uh oh!

bronzdoc commented May 31, 2020

Uh oh!

deivid-rodriguez commented May 31, 2020

Uh oh!

bronzdoc commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented May 31, 2020

Uh oh!

deivid-rodriguez commented May 31, 2020

Uh oh!

eregon commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon commented May 31, 2020

Uh oh!

deivid-rodriguez commented May 31, 2020

Uh oh!

eregon commented May 31, 2020

Uh oh!

bronzdoc commented Jun 2, 2020

Uh oh!

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bronzdoc commented May 31, 2020 •

edited

Loading

eregon commented May 31, 2020 •

edited

Loading

eregon commented May 31, 2020 •

edited

Loading