diff options
author | The Bundler Bot <bot@bundler.io> | 2017-09-13 23:17:24 +0000 |
---|---|---|
committer | The Bundler Bot <bot@bundler.io> | 2017-09-13 23:17:24 +0000 |
commit | c1668a569244145790c627a08aa0f0f4720031ad (patch) | |
tree | 9ec78bb076d11ce6803d9da7cbff8e15f1c71519 | |
parent | 0d38f6604c425228cb6ca87c7121e67b23966b6f (diff) | |
parent | f8f3b8ae57a1f7e2adcac3750465347a9259600a (diff) | |
download | bundler-c1668a569244145790c627a08aa0f0f4720031ad.tar.gz |
Auto merge of #6022 - bundler:seg-fix-fetcher-regression, r=colby-swandale
Avoid making unnecessary network requests fetching specs
### What was the end-user problem that led to this PR?
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.
### What was your diagnosis of the problem?
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_.
### What is your fix for the problem, implemented in this PR?
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")
### Why did you choose this fix out of the possible options?
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.
-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 ecb41f83c2..4b9a781c82 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 7962ff40f8..5adc8ecbd8 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -39,6 +39,7 @@ module Bundler @gem_version_promoter = gem_version_promoter @allow_bundler_dependency_conflicts = Bundler.feature_flag.allow_bundler_dependency_conflicts? @use_gvp = Bundler.feature_flag.use_gem_version_promoter_for_major_updates? || !@gem_version_promoter.major? + @lockfile_uses_separate_rubygems_sources = Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? end def start(requirements) @@ -149,18 +150,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 @@ -195,7 +194,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] @@ -343,7 +342,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 0d52130263..9afecff8e6 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? |