From 4df7c3946ab8da8af4c3c0e38a41ab3bd890fc7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Tue, 4 Apr 2023 20:52:20 +0200 Subject: [PATCH] [rubygems/rubygems] Remove one fallback to full indexes on big gemfiles If Gemfile has a lot of dependencies, we have an optimization that uses the full index in that case, assuming it's going to be faster. I think this is an old optimization that predates compact index API times, I believe we no longer need it these days. Also, since a few releases ago we check for circular dependencies when resolving by looping through all versions of each name and removing those that have circular dependencies that would trip up the resolver. This loop becomes actually very slow when full indexes are used because to find dependencies of a gemspec, we need to explicitly fetch the marshaled gemspec (`gemspec.rz` endpoint) for it, so the optimization has the opposite effect of making things very slow. https://github.com/rubygems/rubygems/commit/2f46289bd3 --- lib/bundler/source/rubygems.rb | 13 +++++-------- spec/bundler/install/gems/compact_index_spec.rb | 12 ++++-------- spec/bundler/install/gems/dependency_api_spec.rb | 12 ++++-------- spec/bundler/support/builders.rb | 12 ------------ spec/bundler/support/path.rb | 4 ---- 5 files changed, 13 insertions(+), 40 deletions(-) diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb index 8d0c78bd61..a8d2e26324 100644 --- a/lib/bundler/source/rubygems.rb +++ b/lib/bundler/source/rubygems.rb @@ -7,8 +7,6 @@ module Bundler class Rubygems < Source autoload :Remote, File.expand_path("rubygems/remote", __dir__) - # Use the API when installing less than X gems - API_REQUEST_LIMIT = 500 # Ask for X gems per API request API_REQUEST_SIZE = 50 @@ -401,12 +399,11 @@ module Bundler # gather lists from non-api sites fetch_names(index_fetchers, nil, idx, false) - # because ensuring we have all the gems we need involves downloading - # the gemspecs of those gems, if the non-api sites contain more than - # about 500 gems, we treat all sites as non-api for speed. - allow_api = idx.size < API_REQUEST_LIMIT && dependency_names.size < API_REQUEST_LIMIT - Bundler.ui.debug "Need to query more than #{API_REQUEST_LIMIT} gems." \ - " Downloading full index instead..." unless allow_api + # legacy multi-remote sources need special logic to figure out + # dependency names and that logic can be very costly if one remote + # uses the dependency API but others don't. So use full indexes + # consistently in that particular case. + allow_api = !multiple_remotes? fetch_names(api_fetchers, allow_api && dependency_names, idx, false) end diff --git a/spec/bundler/install/gems/compact_index_spec.rb b/spec/bundler/install/gems/compact_index_spec.rb index 4d5776e2b8..8ab8e61673 100644 --- a/spec/bundler/install/gems/compact_index_spec.rb +++ b/spec/bundler/install/gems/compact_index_spec.rb @@ -406,7 +406,7 @@ The checksum of /versions does not match the checksum provided by the server! So expect(out).to include("Fetching source index from http://localgemserver.test/extra") end - it "does not fetch every spec if the index of gems is large when doing back deps" do + it "does not fetch every spec when doing back deps" do build_repo2 do build_gem "back_deps" do |s| s.add_dependency "foo" @@ -416,9 +416,7 @@ The checksum of /versions does not match the checksum provided by the server! So FileUtils.rm_rf Dir[gem_repo2("gems/foo-*.gem")] end - api_request_limit = low_api_request_limit_for(gem_repo2) - - install_gemfile <<-G, :artifice => "compact_index_extra_missing", :requires => [api_request_limit_hack_file], :env => { "BUNDLER_SPEC_API_REQUEST_LIMIT" => api_request_limit.to_s }.merge(env_for_missing_prerelease_default_gem_activation) + install_gemfile <<-G, :artifice => "compact_index_extra_missing", :env => env_for_missing_prerelease_default_gem_activation source "#{source_uri}" source "#{source_uri}/extra" do gem "back_deps" @@ -428,7 +426,7 @@ The checksum of /versions does not match the checksum provided by the server! So expect(the_bundle).to include_gems "back_deps 1.0" end - it "does not fetch every spec if the index of gems is large when doing back deps & everything is the compact index" do + it "does not fetch every spec when doing back deps & everything is the compact index" do build_repo4 do build_gem "back_deps" do |s| s.add_dependency "foo" @@ -438,9 +436,7 @@ The checksum of /versions does not match the checksum provided by the server! So FileUtils.rm_rf Dir[gem_repo4("gems/foo-*.gem")] end - api_request_limit = low_api_request_limit_for(gem_repo4) - - install_gemfile <<-G, :artifice => "compact_index_extra_api_missing", :requires => [api_request_limit_hack_file], :env => { "BUNDLER_SPEC_API_REQUEST_LIMIT" => api_request_limit.to_s }.merge(env_for_missing_prerelease_default_gem_activation) + install_gemfile <<-G, :artifice => "compact_index_extra_api_missing", :env => env_for_missing_prerelease_default_gem_activation source "#{source_uri}" source "#{source_uri}/extra" do gem "back_deps" diff --git a/spec/bundler/install/gems/dependency_api_spec.rb b/spec/bundler/install/gems/dependency_api_spec.rb index 6cb3d9697d..a54f1db772 100644 --- a/spec/bundler/install/gems/dependency_api_spec.rb +++ b/spec/bundler/install/gems/dependency_api_spec.rb @@ -359,7 +359,7 @@ RSpec.describe "gemcutter's dependency API" do expect(out).to include("Fetching source index from http://localgemserver.test/extra") end - it "does not fetch every spec if the index of gems is large when doing back deps", :bundler => "< 3" do + it "does not fetch every spec when doing back deps", :bundler => "< 3" do build_repo2 do build_gem "back_deps" do |s| s.add_dependency "foo" @@ -369,9 +369,7 @@ RSpec.describe "gemcutter's dependency API" do FileUtils.rm_rf Dir[gem_repo2("gems/foo-*.gem")] end - api_request_limit = low_api_request_limit_for(gem_repo2) - - install_gemfile <<-G, :artifice => "endpoint_extra_missing", :requires => [api_request_limit_hack_file], :env => { "BUNDLER_SPEC_API_REQUEST_LIMIT" => api_request_limit.to_s }.merge(env_for_missing_prerelease_default_gem_activation) + install_gemfile <<-G, :artifice => "endpoint_extra_missing", :env => env_for_missing_prerelease_default_gem_activation source "#{source_uri}" source "#{source_uri}/extra" gem "back_deps" @@ -380,7 +378,7 @@ RSpec.describe "gemcutter's dependency API" do expect(the_bundle).to include_gems "back_deps 1.0" end - it "does not fetch every spec if the index of gems is large when doing back deps using blocks" do + it "does not fetch every spec when doing back deps using blocks" do build_repo2 do build_gem "back_deps" do |s| s.add_dependency "foo" @@ -390,9 +388,7 @@ RSpec.describe "gemcutter's dependency API" do FileUtils.rm_rf Dir[gem_repo2("gems/foo-*.gem")] end - api_request_limit = low_api_request_limit_for(gem_repo2) - - install_gemfile <<-G, :artifice => "endpoint_extra_missing", :requires => [api_request_limit_hack_file], :env => { "BUNDLER_SPEC_API_REQUEST_LIMIT" => api_request_limit.to_s }.merge(env_for_missing_prerelease_default_gem_activation) + install_gemfile <<-G, :artifice => "endpoint_extra_missing", :env => env_for_missing_prerelease_default_gem_activation source "#{source_uri}" source "#{source_uri}/extra" do gem "back_deps" diff --git a/spec/bundler/support/builders.rb b/spec/bundler/support/builders.rb index b546ac7305..7c16470153 100644 --- a/spec/bundler/support/builders.rb +++ b/spec/bundler/support/builders.rb @@ -17,18 +17,6 @@ module Spec Gem::Platform.new(platform) end - # Returns a number smaller than the size of the index. Useful for specs that - # need the API request limit to be reached for some reason. - def low_api_request_limit_for(gem_repo) - all_gems = Dir[gem_repo.join("gems/*.gem")] - - all_gem_names = all_gems.map do |file| - File.basename(file, ".gem").match(/\A(?[^-]+)-.*\z/)[:gem_name] - end.uniq - - (all_gem_names - ["bundler"]).size - end - def build_repo1 rake_path = Dir["#{Path.base_system_gems}/**/rake*.gem"].first diff --git a/spec/bundler/support/path.rb b/spec/bundler/support/path.rb index de03b2746e..2870a8b678 100644 --- a/spec/bundler/support/path.rb +++ b/spec/bundler/support/path.rb @@ -71,10 +71,6 @@ module Spec @spec_dir ||= source_root.join(ruby_core? ? "spec/bundler" : "spec") end - def api_request_limit_hack_file - spec_dir.join("support/api_request_limit_hax.rb") - end - def man_dir @man_dir ||= lib_dir.join("bundler/man") end