diff options
author | The Bundler Bot <bot@bundler.io> | 2017-09-13 23:17:24 +0000 |
---|---|---|
committer | Samuel Giddins <segiddins@segiddins.me> | 2017-09-18 13:43:33 -0500 |
commit | b26899bb88fccd8fd68a2a21ee2ccc1b68681b23 (patch) | |
tree | 62f2cfb56de43463d80ec777147d6fcd0bcb5856 | |
parent | acbe31104bade2729d2616dfac9254ddd63c7c88 (diff) | |
download | bundler-b26899bb88fccd8fd68a2a21ee2ccc1b68681b23.tar.gz |
Auto merge of #6022 - bundler:seg-fix-fetcher-regression, r=colby-swandale
Avoid making unnecessary network requests fetching specs
The problem was installing `source "https://rubygems.org"; gem "rack"` could cause bundler to make requests for _hundreds_ of specs, drastically slowing down installation. This was a regression in 1.16 caused by the new source pinning logic.
My diagnosis was the "double checking" step of creating the definition's index was accidentally saying Bundler needed to download specs for _every gem installed_, along with their dependencies _for every version in existence_.
My fix narrows down the set of names that need to be double-checked for to (a) only those that could possibly be needed for resolution (b) and only those specs that could possibly come from many sources (i.e. are "unpinned")
I chose this fix because it takes that `rack` gemfile back down to __2__ requests: `/versions` and `/info/rack`, as it should be, while maintaining proper searching for back-deps.
(cherry picked from commit c1668a569244145790c627a08aa0f0f4720031ad)
-rwxr-xr-x | bin/bundle1 | 8 | ||||
-rw-r--r-- | lib/bundler/definition.rb | 46 | ||||
-rw-r--r-- | lib/bundler/fetcher/dependency.rb | 2 | ||||
-rw-r--r-- | lib/bundler/index.rb | 19 | ||||
-rw-r--r-- | lib/bundler/resolver.rb | 15 | ||||
-rw-r--r-- | lib/bundler/source.rb | 4 | ||||
-rw-r--r-- | lib/bundler/source/rubygems.rb | 25 | ||||
-rw-r--r-- | lib/bundler/spec_set.rb | 7 | ||||
-rw-r--r-- | spec/install/gems/compact_index_spec.rb | 19 | ||||
-rw-r--r-- | spec/install/gems/dependency_api_spec.rb | 4 | ||||
-rw-r--r-- | spec/support/artifice/endpoint.rb | 26 |
11 files changed, 122 insertions, 53 deletions
diff --git a/bin/bundle1 b/bin/bundle1 new file mode 100755 index 0000000000..b5df992269 --- /dev/null +++ b/bin/bundle1 @@ -0,0 +1,8 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +module Bundler + VERSION = "1.98".freeze +end + +load File.expand_path("../bundle", __FILE__) diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb index 1385ff2a94..486ef85a64 100644 --- a/lib/bundler/definition.rb +++ b/lib/bundler/definition.rb @@ -265,9 +265,8 @@ module Bundler dependency_names = @dependencies.map(&:name) sources.all_sources.each do |source| - source.dependency_names = dependency_names.dup + source.dependency_names = dependency_names - pinned_spec_names(source) idx.add_source source.specs - dependency_names -= pinned_spec_names(source.specs) dependency_names.concat(source.unmet_deps).uniq! end @@ -281,21 +280,25 @@ module Bundler # of Foo specifically depends on a version of Bar that is only found in source B. This ensures that for # each spec we found, we add all possible versions from all sources to the index. def double_check_for_index(idx, dependency_names) + pinned_names = pinned_spec_names loop do idxcount = idx.size + + names = :names # do this so we only have to traverse to get dependency_names from the index once + unmet_dependency_names = lambda do + return names unless names == :names + new_names = sources.all_sources.map(&:dependency_names_to_double_check) + return names = nil if new_names.compact! + names = new_names.flatten(1).concat(dependency_names) + names.uniq! + names -= pinned_names + names + end + sources.all_sources.each do |source| - names = :names # do this so we only have to traverse to get dependency_names from the index once - unmet_dependency_names = proc do - break names unless names == :names - names = if idx.size > Source::Rubygems::API_REQUEST_LIMIT - new_names = idx.dependency_names_if_available - new_names && dependency_names.+(new_names).uniq - else - dependency_names.+(idx.dependency_names).uniq - end - end source.double_check_for(unmet_dependency_names, :override_dupes) end + break if idxcount == idx.size end end @@ -916,18 +919,15 @@ module Bundler source_requirements end - def pinned_spec_names(specs) - names = [] - specs.each do |s| - # TODO: when two sources without blocks is an error, we can change - # this check to !s.source.is_a?(Source::LocalRubygems). For now, - # we need to ask every RubyGems for every gem name. - if s.source.is_a?(Source::Git) || s.source.is_a?(Source::Path) - names << s.name - end + def pinned_spec_names(skip = nil) + pinned_names = [] + default = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? && sources.default_source + @dependencies.each do |dep| + next unless dep_source = dep.source || default + next if dep_source == skip + pinned_names << dep.name end - names.uniq! - names + pinned_names end def requested_groups diff --git a/lib/bundler/fetcher/dependency.rb b/lib/bundler/fetcher/dependency.rb index 741b81acac..1430d1ebeb 100644 --- a/lib/bundler/fetcher/dependency.rb +++ b/lib/bundler/fetcher/dependency.rb @@ -7,7 +7,7 @@ module Bundler class Fetcher class Dependency < Base def available? - fetch_uri.scheme != "file" && downloader.fetch(dependency_api_uri) + @available ||= fetch_uri.scheme != "file" && downloader.fetch(dependency_api_uri) rescue NetworkDownError => e raise HTTPError, e.message rescue AuthenticationRequiredError diff --git a/lib/bundler/index.rb b/lib/bundler/index.rb index 8d496f1e2b..9166a92738 100644 --- a/lib/bundler/index.rb +++ b/lib/bundler/index.rb @@ -115,6 +115,12 @@ module Bundler self end + def spec_names + names = specs.keys + sources.map(&:spec_names) + names.uniq! + names + end + # returns a list of the dependencies def unmet_dependency_names dependency_names.select do |name| @@ -133,19 +139,6 @@ module Bundler names.uniq end - def dependency_names_if_available - reduce([]) do |names, spec| - case spec - when EndpointSpecification, Gem::Specification, LazySpecification, StubSpecification - names.concat(spec.dependencies) - when RemoteSpecification # from the full index - return nil - else - raise "unhandled spec type in #dependency_names_if_available (#{spec.inspect})" - end - end.tap {|n| n && n.map!(&:name) } - end - def use(other, override_dupes = false) return unless other other.each do |s| diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index 348b64ff4e..c25c853f11 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -38,6 +38,7 @@ module Bundler @platforms = platforms @gem_version_promoter = gem_version_promoter @allow_bundler_dependency_conflicts = Bundler.feature_flag.allow_bundler_dependency_conflicts? + @lockfile_uses_separate_rubygems_sources = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? end def start(requirements) @@ -145,18 +146,16 @@ module Bundler def index_for(dependency) source = @source_requirements[dependency.name] - if Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? + if source + source.specs + elsif @lockfile_uses_separate_rubygems_sources Index.build do |idx| - if source - idx.add_source source.specs - elsif dependency.all_sources + if dependency.all_sources dependency.all_sources.each {|s| idx.add_source(s.specs) if s } else idx.add_source @source_requirements[:default].specs end end - elsif source - source.specs else @index end @@ -191,7 +190,7 @@ module Bundler def relevant_sources_for_vertex(vertex) if vertex.root? [@source_requirements[vertex.name]] - elsif Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? + elsif @lockfile_uses_separate_rubygems_sources vertex.recursive_predecessors.map do |v| @source_requirements[v.name] end << @source_requirements[:default] @@ -339,7 +338,7 @@ module Bundler [conflict.requirement.source] elsif conflict.requirement.all_sources conflict.requirement.all_sources - elsif Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? + elsif @lockfile_uses_separate_rubygems_sources # every conflict should have an explicit group of sources when we # enforce strict pinning raise "no source set for #{conflict}" diff --git a/lib/bundler/source.rb b/lib/bundler/source.rb index 956cf39d56..5a1f05098b 100644 --- a/lib/bundler/source.rb +++ b/lib/bundler/source.rb @@ -38,6 +38,10 @@ module Bundler # dependencies, looking for gems we don't have info on yet. def double_check_for(*); end + def dependency_names_to_double_check + specs.dependency_names + end + def include?(other) other == self end diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb index fa60bb0c84..6f4157364f 100644 --- a/lib/bundler/source/rubygems.rb +++ b/lib/bundler/source/rubygems.rb @@ -259,13 +259,36 @@ module Bundler return unless api_fetchers.any? unmet_dependency_names = unmet_dependency_names.call - return if !unmet_dependency_names.nil? && unmet_dependency_names.empty? + unless unmet_dependency_names.nil? + if api_fetchers.size <= 1 + # can't do this when there are multiple fetchers because then we might not fetch from _all_ + # of them + unmet_dependency_names -= remote_specs.spec_names # avoid re-fetching things we've already gotten + end + return if unmet_dependency_names.empty? + end Bundler.ui.debug "Double checking for #{unmet_dependency_names || "all specs (due to the size of the request)"} in #{self}" fetch_names(api_fetchers, unmet_dependency_names, index, override_dupes) end + def dependency_names_to_double_check + names = [] + remote_specs.each do |spec| + case spec + when EndpointSpecification, Gem::Specification, StubSpecification, LazySpecification + names.concat(spec.runtime_dependencies) + when RemoteSpecification # from the full index + return nil + else + raise "unhandled spec type (#{spec.inspect})" + end + end + names.map!(&:name) if names + names + end + protected def credless_remotes diff --git a/lib/bundler/spec_set.rb b/lib/bundler/spec_set.rb index 5239a96213..7cd3021997 100644 --- a/lib/bundler/spec_set.rb +++ b/lib/bundler/spec_set.rb @@ -110,9 +110,10 @@ module Bundler def merge(set) arr = sorted.dup - set.each do |s| - next if arr.any? {|s2| s2.name == s.name && s2.version == s.version && s2.platform == s.platform } - arr << s + set.each do |set_spec| + full_name = set_spec.full_name + next if arr.any? {|spec| spec.full_name == full_name } + arr << set_spec end SpecSet.new(arr) end diff --git a/spec/install/gems/compact_index_spec.rb b/spec/install/gems/compact_index_spec.rb index 273366c32f..f633004a3d 100644 --- a/spec/install/gems/compact_index_spec.rb +++ b/spec/install/gems/compact_index_spec.rb @@ -253,6 +253,22 @@ The checksum of /versions does not match the checksum provided by the server! So end end + it "does not double check for gems that are only installed locally" do + system_gems %w[rack-1.0.0 thin-1.0 net_a-1.0] + bundle! "config --local path.system true" + ENV["BUNDLER_SPEC_ALL_REQUESTS"] = strip_whitespace(<<-EOS).strip + #{source_uri}/versions + #{source_uri}/info/rack + EOS + + install_gemfile! <<-G, :artifice => "compact_index", :verbose => true + source "#{source_uri}" + gem "rack" + G + + expect(last_command.stdboth).not_to include "Double checking" + end + it "fetches again when more dependencies are found in subsequent sources", :bundler => "< 2" do build_repo2 do build_gem "back_deps" do |s| @@ -279,14 +295,13 @@ The checksum of /versions does not match the checksum provided by the server! So FileUtils.rm_rf Dir[gem_repo2("gems/foo-*.gem")] end - gemfile <<-G + install_gemfile! <<-G, :artifice => "compact_index_extra", :verbose => true source "#{source_uri}" source "#{source_uri}/extra" do gem "back_deps" end G - bundle! :install, :artifice => "compact_index_extra" expect(the_bundle).to include_gems "back_deps 1.0", "foo 1.0" end diff --git a/spec/install/gems/dependency_api_spec.rb b/spec/install/gems/dependency_api_spec.rb index 22c623ab40..2ffe4b62d7 100644 --- a/spec/install/gems/dependency_api_spec.rb +++ b/spec/install/gems/dependency_api_spec.rb @@ -320,7 +320,7 @@ RSpec.describe "gemcutter's dependency API" do gem 'somegem', '1.0.0' G - bundle :install, :artifice => "endpoint_extra_api" + bundle! :install, :artifice => "endpoint_extra_api" expect(the_bundle).to include_gems "somegem 1.0.0" expect(the_bundle).to include_gems "activesupport 1.2.3" @@ -368,7 +368,7 @@ RSpec.describe "gemcutter's dependency API" do bundle :install, :artifice => "endpoint_extra" - expect(out).to include("Fetching gem metadata from http://localgemserver.test/..") + expect(out).to include("Fetching gem metadata from http://localgemserver.test/.") expect(out).to include("Fetching source index from http://localgemserver.test/extra") end diff --git a/spec/support/artifice/endpoint.rb b/spec/support/artifice/endpoint.rb index 33e2a9b411..94f2b5cbc9 100644 --- a/spec/support/artifice/endpoint.rb +++ b/spec/support/artifice/endpoint.rb @@ -8,11 +8,37 @@ $LOAD_PATH.unshift(*Dir[Spec::Path.base_system_gems.join("gems/{artifice,rack,ti require "artifice" require "sinatra/base" +ALL_REQUESTS = [] # rubocop:disable Style/MutableConstant +ALL_REQUESTS_MUTEX = Mutex.new + +at_exit do + if expected = ENV["BUNDLER_SPEC_ALL_REQUESTS"] + expected = expected.split("\n").sort + actual = ALL_REQUESTS.sort + + unless expected == actual + raise "Unexpected requests!\nExpected:\n\t#{expected.join("\n\t")}\n\nActual:\n\t#{actual.join("\n\t")}" + end + end +end + class Endpoint < Sinatra::Base + def self.all_requests + @all_requests ||= [] + end + GEM_REPO = Pathname.new(ENV["BUNDLER_SPEC_GEM_REPO"] || Spec::Path.gem_repo1) set :raise_errors, true set :show_exceptions, false + def call!(*) + super.tap do + ALL_REQUESTS_MUTEX.synchronize do + ALL_REQUESTS << @request.url + end + end + end + helpers do def dependencies_for(gem_names, gem_repo = GEM_REPO) return [] if gem_names.nil? || gem_names.empty? |