Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Commit

Permalink
Merge #7246
Browse files Browse the repository at this point in the history
7246: No need to make gem refresh a noop r=indirect a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was that rubygems/rubygems#2711 fails because the test in bundler that checks that `Gem.refresh` reveals no system gems fails.

### What was your diagnosis of the problem?

My diagnosis was that we don't really need to make `Gem.refresh` a noop at all, because we already setup a `post_reset` hook to automatically go back to the set of specs bundler knows about, everytime `Gem::Specification.reset` is called. See https://github.com/bundler/bundler/blob/847db56c6b5d2156d27990d73a9949507693985a/lib/bundler/rubygems_integration.rb#L551-L554.

And `Gem::Specification.reset` is what `Gem.refresh` does: https://github.com/rubygems/rubygems/blob/564bb0a5bb310ccceaf6cd6391b3e67e0712edd5/lib/rubygems.rb#L861-L867.

### What is your fix for the problem, implemented in this PR?

My fix is to remove the redefinition of `Gem.refresh`. This makes sense to me not only because it's unnecessary, but because the monkeypatch could still be very easily workarounded by calling `Gem::Specification.reset` directly.

After undoing the `Gem.refresh` redefinition, I changed the test to still make sure that the (not overriden) `Gem.refresh` still does not change the set of specs bundler knows about.

This is a nice simplification, but doesn't really fix the spec for rubygems/rubygems#2711, because after the first call to `Bundler.rubygems.find_name("rack")`, the `@stubs_by_name` array in rubygems gets populated and thus is no longer empty.

We can test the same thing without using `@stubs_by_name` under the hood, so I changed the test to do that, so that the rubygems PR is not affected.

### Why did you choose this fix out of the possible options?

I chose this fix because it simplifies the integration between bundler & rubygems, and it unblocks rubygems/rubygems#2711.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
  • Loading branch information
bundlerbot and deivid-rodriguez committed Jul 11, 2019
2 parents 847db56 + 49c519e commit 2eec310
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 12 deletions.
8 changes: 0 additions & 8 deletions lib/bundler/rubygems_integration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -441,13 +441,6 @@ def replace_bin_path(specs_by_name)
end
end

# Because Bundler has a static view of what specs are available,
# we don't #refresh, so stub it out.
def replace_refresh
gem_class = (class << Gem; self; end)
redefine_method(gem_class, :refresh) {}
end

# Replace or hook into RubyGems to provide a bundlerized view
# of the world.
def replace_entrypoints(specs)
Expand All @@ -468,7 +461,6 @@ def replace_entrypoints(specs)
replace_gem(specs, specs_by_name)
stub_rubygems(specs)
replace_bin_path(specs_by_name)
replace_refresh

Gem.clear_paths
end
Expand Down
8 changes: 4 additions & 4 deletions spec/runtime/setup_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ def clean_load_path(lp)
end
end

it "stubs out Gem.refresh so it does not reveal system gems" do
it "does not reveal system gems even when Gem.refresh is called" do
system_gems "rack-1.0.0"

install_gemfile <<-G
Expand All @@ -854,12 +854,12 @@ def clean_load_path(lp)
G

run <<-R
puts Bundler.rubygems.find_name("rack").inspect
puts Bundler.rubygems.all_specs.map(&:name)
Gem.refresh
puts Bundler.rubygems.find_name("rack").inspect
puts Bundler.rubygems.all_specs.map(&:name)
R

expect(out).to eq("[]\n[]")
expect(out).to eq("activesupport\nbundler\nactivesupport\nbundler")
end

describe "when a vendored gem specification uses the :path option" do
Expand Down

0 comments on commit 2eec310

Please sign in to comment.