You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
324 lines
12 KiB
324 lines
12 KiB
From 00d98eb8a3245fb93a475ecbbbc4c7ec7e6704cd Mon Sep 17 00:00:00 2001 |
|
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@riseup.net> |
|
Date: Tue, 19 May 2020 14:00:00 +0200 |
|
Subject: [PATCH 1/5] Fix performance regression in `require` |
|
|
|
Our check for `-I` paths should not go through all activated gems. |
|
--- |
|
lib/rubygems.rb | 10 ++++++++++ |
|
lib/rubygems/core_ext/kernel_require.rb | 2 +- |
|
lib/rubygems/test_case.rb | 1 + |
|
test/rubygems/test_require.rb | 11 +++++++++++ |
|
4 files changed, 23 insertions(+), 1 deletion(-) |
|
|
|
diff --git a/lib/rubygems.rb b/lib/rubygems.rb |
|
index 843cb49e4a..d1a9a1c7e1 100644 |
|
--- a/lib/rubygems.rb |
|
+++ b/lib/rubygems.rb |
|
@@ -662,10 +662,20 @@ def self.load_path_insert_index |
|
index |
|
end |
|
|
|
+ ## |
|
+ # The number of paths in the `$LOAD_PATH` from activated gems. Used to |
|
+ # prioritize `-I` and `ENV['RUBYLIB`]` entries during `require`. |
|
+ |
|
+ def self.activated_gem_paths |
|
+ @activated_gem_paths ||= 0 |
|
+ end |
|
+ |
|
## |
|
# Add a list of paths to the $LOAD_PATH at the proper place. |
|
|
|
def self.add_to_load_path(*paths) |
|
+ @activated_gem_paths = activated_gem_paths + paths.size |
|
+ |
|
insert_index = load_path_insert_index |
|
|
|
if insert_index |
|
diff --git a/lib/rubygems/core_ext/kernel_require.rb b/lib/rubygems/core_ext/kernel_require.rb |
|
index ed24111bd5..7625ce1bee 100644 |
|
--- a/lib/rubygems/core_ext/kernel_require.rb |
|
+++ b/lib/rubygems/core_ext/kernel_require.rb |
|
@@ -47,7 +47,7 @@ def require(path) |
|
load_path_insert_index = Gem.load_path_insert_index |
|
break unless load_path_insert_index |
|
|
|
- $LOAD_PATH[0...load_path_insert_index].each do |lp| |
|
+ $LOAD_PATH[0...load_path_insert_index - Gem.activated_gem_paths].each do |lp| |
|
safe_lp = lp.dup.tap(&Gem::UNTAINT) |
|
begin |
|
if File.symlink? safe_lp # for backward compatibility |
|
diff --git a/lib/rubygems/test_case.rb b/lib/rubygems/test_case.rb |
|
index a05a2898d1..53dd495aef 100644 |
|
--- a/lib/rubygems/test_case.rb |
|
+++ b/lib/rubygems/test_case.rb |
|
@@ -385,6 +385,7 @@ def setup |
|
Gem::Security.reset |
|
|
|
Gem.loaded_specs.clear |
|
+ Gem.instance_variable_set(:@activated_gem_paths, 0) |
|
Gem.clear_default_specs |
|
Bundler.reset! |
|
|
|
diff --git a/test/rubygems/test_require.rb b/test/rubygems/test_require.rb |
|
index f36892f8cc..9f2fe3439a 100644 |
|
--- a/test/rubygems/test_require.rb |
|
+++ b/test/rubygems/test_require.rb |
|
@@ -382,6 +382,17 @@ def test_default_gem_require_activates_just_once |
|
assert_equal 0, times_called |
|
end |
|
|
|
+ def test_second_gem_require_does_not_resolve_path_manually_before_going_through_standard_require |
|
+ a1 = util_spec "a", "1", nil, "lib/test_gem_require_a.rb" |
|
+ install_gem a1 |
|
+ |
|
+ assert_require "test_gem_require_a" |
|
+ |
|
+ stub(:gem_original_require, ->(path) { assert_equal "test_gem_require_a", path }) do |
|
+ require "test_gem_require_a" |
|
+ end |
|
+ end |
|
+ |
|
def test_realworld_default_gem |
|
testing_ruby_repo = !ENV["GEM_COMMAND"].nil? |
|
skip "this test can't work under ruby-core setup" if testing_ruby_repo || java_platform? |
|
|
|
From ae95885dff6189c5ac59bbdf685cb4ec4751fdef Mon Sep 17 00:00:00 2001 |
|
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@riseup.net> |
|
Date: Tue, 19 May 2020 14:08:19 +0200 |
|
Subject: [PATCH 2/5] Refactor `Gem.load_path_insert_index` |
|
|
|
--- |
|
lib/rubygems.rb | 13 +++---------- |
|
lib/rubygems/core_ext/kernel_require.rb | 5 +---- |
|
2 files changed, 4 insertions(+), 14 deletions(-) |
|
|
|
diff --git a/lib/rubygems.rb b/lib/rubygems.rb |
|
index d1a9a1c7e1..ca80326459 100644 |
|
--- a/lib/rubygems.rb |
|
+++ b/lib/rubygems.rb |
|
@@ -659,7 +659,7 @@ def self.load_path_insert_index |
|
|
|
index = $LOAD_PATH.index RbConfig::CONFIG['sitelibdir'] |
|
|
|
- index |
|
+ index || 0 |
|
end |
|
|
|
## |
|
@@ -676,15 +676,8 @@ def self.activated_gem_paths |
|
def self.add_to_load_path(*paths) |
|
@activated_gem_paths = activated_gem_paths + paths.size |
|
|
|
- insert_index = load_path_insert_index |
|
- |
|
- if insert_index |
|
- # gem directories must come after -I and ENV['RUBYLIB'] |
|
- $LOAD_PATH.insert(insert_index, *paths) |
|
- else |
|
- # we are probably testing in core, -I and RUBYLIB don't apply |
|
- $LOAD_PATH.unshift(*paths) |
|
- end |
|
+ # gem directories must come after -I and ENV['RUBYLIB'] |
|
+ $LOAD_PATH.insert(Gem.load_path_insert_index, *paths) |
|
end |
|
|
|
@yaml_loaded = false |
|
diff --git a/lib/rubygems/core_ext/kernel_require.rb b/lib/rubygems/core_ext/kernel_require.rb |
|
index 7625ce1bee..decf4829f1 100644 |
|
--- a/lib/rubygems/core_ext/kernel_require.rb |
|
+++ b/lib/rubygems/core_ext/kernel_require.rb |
|
@@ -44,10 +44,7 @@ def require(path) |
|
resolved_path = begin |
|
rp = nil |
|
Gem.suffixes.each do |s| |
|
- load_path_insert_index = Gem.load_path_insert_index |
|
- break unless load_path_insert_index |
|
- |
|
- $LOAD_PATH[0...load_path_insert_index - Gem.activated_gem_paths].each do |lp| |
|
+ $LOAD_PATH[0...Gem.load_path_insert_index - Gem.activated_gem_paths].each do |lp| |
|
safe_lp = lp.dup.tap(&Gem::UNTAINT) |
|
begin |
|
if File.symlink? safe_lp # for backward compatibility |
|
|
|
From da1492e9d7b28d068fbfbb0ba1cafcc516681567 Mon Sep 17 00:00:00 2001 |
|
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@riseup.net> |
|
Date: Tue, 19 May 2020 14:32:12 +0200 |
|
Subject: [PATCH 3/5] Extract a local outside the loop |
|
|
|
--- |
|
lib/rubygems/core_ext/kernel_require.rb | 3 ++- |
|
1 file changed, 2 insertions(+), 1 deletion(-) |
|
|
|
diff --git a/lib/rubygems/core_ext/kernel_require.rb b/lib/rubygems/core_ext/kernel_require.rb |
|
index decf4829f1..6a7faaf2d1 100644 |
|
--- a/lib/rubygems/core_ext/kernel_require.rb |
|
+++ b/lib/rubygems/core_ext/kernel_require.rb |
|
@@ -43,8 +43,9 @@ def require(path) |
|
# https://github.com/rubygems/rubygems/pull/1868 |
|
resolved_path = begin |
|
rp = nil |
|
+ load_path_check_index = Gem.load_path_insert_index - Gem.activated_gem_paths |
|
Gem.suffixes.each do |s| |
|
- $LOAD_PATH[0...Gem.load_path_insert_index - Gem.activated_gem_paths].each do |lp| |
|
+ $LOAD_PATH[0...load_path_check_index].each do |lp| |
|
safe_lp = lp.dup.tap(&Gem::UNTAINT) |
|
begin |
|
if File.symlink? safe_lp # for backward compatibility |
|
|
|
From 22ad5717c38feda2375b53628d15ae3db2195684 Mon Sep 17 00:00:00 2001 |
|
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@riseup.net> |
|
Date: Thu, 21 May 2020 15:20:57 +0200 |
|
Subject: [PATCH 4/5] Fix `$LOADED_FEATURES` cache sometimes not respected |
|
|
|
In the cases where the initial manually `-I` path resolution succeeded, |
|
we were passing a full path to the original require effectively skipping |
|
the `$LOADED_FEATURES` cache. With this change, we _only_ do the |
|
resolution when a matching requirable path is found in a default gem. In |
|
that case, we skip activation of the default gem if we detect that the |
|
required file will be picked up for a `-I` path. |
|
--- |
|
lib/rubygems/core_ext/kernel_require.rb | 53 +++++++++++-------------- |
|
test/rubygems/test_require.rb | 29 ++++++++++++++ |
|
2 files changed, 53 insertions(+), 29 deletions(-) |
|
|
|
diff --git a/lib/rubygems/core_ext/kernel_require.rb b/lib/rubygems/core_ext/kernel_require.rb |
|
index 6a7faaf2d1..81e37b98bf 100644 |
|
--- a/lib/rubygems/core_ext/kernel_require.rb |
|
+++ b/lib/rubygems/core_ext/kernel_require.rb |
|
@@ -39,46 +39,41 @@ def require(path) |
|
|
|
path = path.to_path if path.respond_to? :to_path |
|
|
|
- # Ensure -I beats a default gem |
|
- # https://github.com/rubygems/rubygems/pull/1868 |
|
- resolved_path = begin |
|
- rp = nil |
|
- load_path_check_index = Gem.load_path_insert_index - Gem.activated_gem_paths |
|
- Gem.suffixes.each do |s| |
|
- $LOAD_PATH[0...load_path_check_index].each do |lp| |
|
- safe_lp = lp.dup.tap(&Gem::UNTAINT) |
|
- begin |
|
- if File.symlink? safe_lp # for backward compatibility |
|
- next |
|
+ if spec = Gem.find_unresolved_default_spec(path) |
|
+ # Ensure -I beats a default gem |
|
+ # https://github.com/rubygems/rubygems/pull/1868 |
|
+ resolved_path = begin |
|
+ rp = nil |
|
+ load_path_check_index = Gem.load_path_insert_index - Gem.activated_gem_paths |
|
+ Gem.suffixes.each do |s| |
|
+ $LOAD_PATH[0...load_path_check_index].each do |lp| |
|
+ safe_lp = lp.dup.tap(&Gem::UNTAINT) |
|
+ begin |
|
+ if File.symlink? safe_lp # for backward compatibility |
|
+ next |
|
+ end |
|
+ rescue SecurityError |
|
+ RUBYGEMS_ACTIVATION_MONITOR.exit |
|
+ raise |
|
end |
|
- rescue SecurityError |
|
- RUBYGEMS_ACTIVATION_MONITOR.exit |
|
- raise |
|
- end |
|
|
|
- full_path = File.expand_path(File.join(safe_lp, "#{path}#{s}")) |
|
- if File.file?(full_path) |
|
- rp = full_path |
|
- break |
|
+ full_path = File.expand_path(File.join(safe_lp, "#{path}#{s}")) |
|
+ if File.file?(full_path) |
|
+ rp = full_path |
|
+ break |
|
+ end |
|
end |
|
+ break if rp |
|
end |
|
- break if rp |
|
+ rp |
|
end |
|
- rp |
|
- end |
|
|
|
- if resolved_path |
|
- RUBYGEMS_ACTIVATION_MONITOR.exit |
|
- return gem_original_require(resolved_path) |
|
- end |
|
- |
|
- if spec = Gem.find_unresolved_default_spec(path) |
|
begin |
|
Kernel.send(:gem, spec.name, Gem::Requirement.default_prerelease) |
|
rescue Exception |
|
RUBYGEMS_ACTIVATION_MONITOR.exit |
|
raise |
|
- end |
|
+ end unless resolved_path |
|
end |
|
|
|
# If there are no unresolved deps, then we can use just try |
|
diff --git a/test/rubygems/test_require.rb b/test/rubygems/test_require.rb |
|
index 9f2fe3439a..2b11e26dfe 100644 |
|
--- a/test/rubygems/test_require.rb |
|
+++ b/test/rubygems/test_require.rb |
|
@@ -45,6 +45,35 @@ def refute_require(path) |
|
refute require(path), "'#{path}' was not yet required" |
|
end |
|
|
|
+ def test_respect_loaded_features_caching_like_standard_require |
|
+ dir = Dir.mktmpdir("test_require", @tempdir) |
|
+ |
|
+ lp1 = File.join dir, 'foo1' |
|
+ foo1 = File.join lp1, 'foo.rb' |
|
+ |
|
+ FileUtils.mkdir_p lp1 |
|
+ File.open(foo1, 'w') { |f| f.write "class Object; HELLO = 'foo1' end" } |
|
+ |
|
+ lp = $LOAD_PATH.dup |
|
+ |
|
+ $LOAD_PATH.unshift lp1 |
|
+ assert_require 'foo' |
|
+ assert_equal "foo1", ::Object::HELLO |
|
+ |
|
+ lp2 = File.join dir, 'foo2' |
|
+ foo2 = File.join lp2, 'foo.rb' |
|
+ |
|
+ FileUtils.mkdir_p lp2 |
|
+ File.open(foo2, 'w') { |f| f.write "class Object; HELLO = 'foo2' end" } |
|
+ |
|
+ $LOAD_PATH.unshift lp2 |
|
+ refute_require 'foo' |
|
+ assert_equal "foo1", ::Object::HELLO |
|
+ ensure |
|
+ $LOAD_PATH.replace lp |
|
+ Object.send :remove_const, :HELLO if Object.const_defined? :HELLO |
|
+ end |
|
+ |
|
# Providing -I on the commandline should always beat gems |
|
def test_dash_i_beats_gems |
|
a1 = util_spec "a", "1", {"b" => "= 1"}, "lib/test_gem_require_a.rb" |
|
|
|
From db872c7a18d616f4447bdcca3130be6db9e5cb03 Mon Sep 17 00:00:00 2001 |
|
From: =?UTF-8?q?David=20Rodr=C3=ADguez?= <deivid.rodriguez@riseup.net> |
|
Date: Sat, 23 May 2020 20:18:41 +0200 |
|
Subject: [PATCH 5/5] Remove direct reference to PR |
|
|
|
The code is quite different now, so I think the link might be even |
|
confusing. If you want to know more, use git history. |
|
--- |
|
lib/rubygems/core_ext/kernel_require.rb | 1 - |
|
1 file changed, 1 deletion(-) |
|
|
|
diff --git a/lib/rubygems/core_ext/kernel_require.rb b/lib/rubygems/core_ext/kernel_require.rb |
|
index 81e37b98bf..115ae0cb50 100644 |
|
--- a/lib/rubygems/core_ext/kernel_require.rb |
|
+++ b/lib/rubygems/core_ext/kernel_require.rb |
|
@@ -41,7 +41,6 @@ def require(path) |
|
|
|
if spec = Gem.find_unresolved_default_spec(path) |
|
# Ensure -I beats a default gem |
|
- # https://github.com/rubygems/rubygems/pull/1868 |
|
resolved_path = begin |
|
rp = nil |
|
load_path_check_index = Gem.load_path_insert_index - Gem.activated_gem_paths
|
|
|