diff options
author | Bundlerbot <bot@bundler.io> | 2019-04-02 07:11:40 +0000 |
---|---|---|
committer | Bundlerbot <bot@bundler.io> | 2019-04-02 07:11:40 +0000 |
commit | d6f343cc5b69185c0bb45140d9e6b1fc4121688b (patch) | |
tree | b42cb47eb21d1e61e65af3614a7a334b1814952d | |
parent | cb062dff33331a067afa4ccd5d445de2100da3a4 (diff) | |
parent | 8c4b82e3a4ef42ab6fee9b324200a9e5ff20c948 (diff) | |
download | bundler-d6f343cc5b69185c0bb45140d9e6b1fc4121688b.tar.gz |
Merge #7007
7007: Remove lockfile incompatibility created by the `lockfile_uses_separate_rubygems_sources` setting r=deivid-rodriguez a=deivid-rodriguez
This is more of a question PR, I created this patch to try it out and try to understand, not necessarily get it merged.
### What was the end-user problem that led to this PR?
The problem was that once we enable the `lockfile_uses_separate_rubygems_sources` setting, all lockfiles in the world will become incompatible with the previous version. Actually, not necessarily incompatible, but bundler will reorder the sections when the setting is enabled, that will generate churn lock file diffs, and _maybe_ some confusion / merge conflicts, and so on.
### What was your diagnosis of the problem?
My diagnosis was that maybe this is not necessary. I read over the issues where this setting was added and what I understood is that previously if a Gemfile specified multiple rubygems sources, they would all get merged together and that's dangerous because it's not deterministic from which source each gem will be picked up, and that could be maliciously exploited. So now each source gets its own separate section. However, how does that affect the ordering of the sections? I don't think it should affect it?
### What is your fix for the problem, implemented in this PR?
My fix is to change the `lock_sources` method so that both code branches (`lockfile_uses_separate_rubygems_sources == true`, and `lockfile_uses_separate_rubygems_sources == false`) result in the same ordering of the source sections.
### Why did you choose this fix out of the possible options?
I chose this fix because I _think_ it keeps the setting doing the same thing, but also keeps lock file compatibility.
Co-authored-by: David RodrÃguez <deivid.rodriguez@riseup.net>
-rw-r--r-- | lib/bundler/source_list.rb | 6 | ||||
-rw-r--r-- | spec/bundler/definition_spec.rb | 49 | ||||
-rw-r--r-- | spec/bundler/source_list_spec.rb | 14 | ||||
-rw-r--r-- | spec/install/gemfile/gemspec_spec.rb | 103 | ||||
-rw-r--r-- | spec/lock/lockfile_spec.rb | 86 | ||||
-rw-r--r-- | spec/plugins/source/example_spec.rb | 16 | ||||
-rw-r--r-- | spec/update/git_spec.rb | 10 |
7 files changed, 72 insertions, 212 deletions
diff --git a/lib/bundler/source_list.rb b/lib/bundler/source_list.rb index 61b7a4e1aa..173875ce22 100644 --- a/lib/bundler/source_list.rb +++ b/lib/bundler/source_list.rb @@ -77,12 +77,10 @@ module Bundler end def lock_sources + lock_sources = (path_sources + git_sources + plugin_sources).sort_by(&:to_s) if Bundler.feature_flag.lockfile_uses_separate_rubygems_sources? - [[default_source], @rubygems_sources, git_sources, path_sources, plugin_sources].map do |sources| - sources.sort_by(&:to_s) - end.flatten(1) + lock_sources + rubygems_sources.sort_by(&:to_s) else - lock_sources = (path_sources + git_sources + plugin_sources).sort_by(&:to_s) lock_sources << combine_rubygems_sources end end diff --git a/spec/bundler/definition_spec.rb b/spec/bundler/definition_spec.rb index ceb7b4bf05..163ec507b0 100644 --- a/spec/bundler/definition_spec.rb +++ b/spec/bundler/definition_spec.rb @@ -88,17 +88,17 @@ RSpec.describe Bundler::Definition do expect(out).to match(/re-resolving dependencies/) lockfile_should_be <<-G - GEM - remote: file://localhost#{gem_repo1}/ - specs: - rack (1.0.0) - PATH remote: #{lib_path("foo")} specs: foo (1.0) rack (= 1.0) + GEM + remote: file://localhost#{gem_repo1}/ + specs: + rack (1.0.0) + PLATFORMS #{lockfile_platforms} @@ -110,7 +110,7 @@ RSpec.describe Bundler::Definition do G end - it "for a path gem with deps and no changes", :bundler => "< 2" do + it "for a path gem with deps and no changes" do build_lib "foo", "1.0", :path => lib_path("foo") do |s| s.add_dependency "rack", "1.0" s.add_development_dependency "net-ssh", "1.0" @@ -137,43 +137,6 @@ RSpec.describe Bundler::Definition do rack (1.0.0) PLATFORMS - ruby - - DEPENDENCIES - foo! - - BUNDLED WITH - #{Bundler::VERSION} - G - end - - it "for a path gem with deps and no changes", :bundler => "2" do - build_lib "foo", "1.0", :path => lib_path("foo") do |s| - s.add_dependency "rack", "1.0" - s.add_development_dependency "net-ssh", "1.0" - end - - install_gemfile <<-G - source "file://localhost#{gem_repo1}" - gem "foo", :path => "#{lib_path("foo")}" - G - - bundle :check, :env => { "DEBUG" => 1 } - - expect(out).to match(/using resolution from the lockfile/) - lockfile_should_be <<-G - GEM - remote: file://localhost#{gem_repo1}/ - specs: - rack (1.0.0) - - PATH - remote: #{lib_path("foo")} - specs: - foo (1.0) - rack (= 1.0) - - PLATFORMS #{lockfile_platforms} DEPENDENCIES diff --git a/spec/bundler/source_list_spec.rb b/spec/bundler/source_list_spec.rb index 729311421a..971f1042dc 100644 --- a/spec/bundler/source_list_spec.rb +++ b/spec/bundler/source_list_spec.rb @@ -393,19 +393,19 @@ RSpec.describe Bundler::SourceList do it "returns all sources, without combining rubygems sources", :bundler => "2" do expect(source_list.lock_sources).to eq [ - Bundler::Source::Rubygems.new, - Bundler::Source::Rubygems.new("remotes" => ["https://duplicate-rubygems.org"]), - Bundler::Source::Rubygems.new("remotes" => ["https://first-rubygems.org"]), - Bundler::Source::Rubygems.new("remotes" => ["https://second-rubygems.org"]), - Bundler::Source::Rubygems.new("remotes" => ["https://third-rubygems.org"]), Bundler::Source::Git.new("uri" => "git://first-git.org/path.git"), Bundler::Source::Git.new("uri" => "git://second-git.org/path.git"), Bundler::Source::Git.new("uri" => "git://third-git.org/path.git"), + ASourcePlugin.new("uri" => "https://second-plugin.org/random"), + ASourcePlugin.new("uri" => "https://third-bar.org/foo"), Bundler::Source::Path.new("path" => "/first/path/to/gem"), Bundler::Source::Path.new("path" => "/second/path/to/gem"), Bundler::Source::Path.new("path" => "/third/path/to/gem"), - ASourcePlugin.new("uri" => "https://second-plugin.org/random"), - ASourcePlugin.new("uri" => "https://third-bar.org/foo"), + Bundler::Source::Rubygems.new, + Bundler::Source::Rubygems.new("remotes" => ["https://duplicate-rubygems.org"]), + Bundler::Source::Rubygems.new("remotes" => ["https://first-rubygems.org"]), + Bundler::Source::Rubygems.new("remotes" => ["https://second-rubygems.org"]), + Bundler::Source::Rubygems.new("remotes" => ["https://third-rubygems.org"]), ] end end diff --git a/spec/install/gemfile/gemspec_spec.rb b/spec/install/gemfile/gemspec_spec.rb index ff0f7a8445..0bec2e9d96 100644 --- a/spec/install/gemfile/gemspec_spec.rb +++ b/spec/install/gemfile/gemspec_spec.rb @@ -447,7 +447,7 @@ RSpec.describe "bundle install from an existing gemspec" do end end - context "on ruby", :bundler => "< 2" do + context "on ruby" do before do simulate_platform("ruby") bundle :install @@ -547,107 +547,6 @@ RSpec.describe "bundle install from an existing gemspec" do end end end - - context "on ruby", :bundler => "2" do - before do - simulate_platform("ruby") - bundle :install - end - - context "as a runtime dependency" do - it "keeps java dependencies in the lockfile" do - expect(the_bundle).to include_gems "foo 1.0", "platform_specific 1.0 RUBY" - expect(lockfile).to eq normalize_uri_file(strip_whitespace(<<-L)) - GEM - remote: file://localhost#{gem_repo2}/ - specs: - platform_specific (1.0) - platform_specific (1.0-java) - - PATH - remote: . - specs: - foo (1.0) - platform_specific - - PLATFORMS - java - ruby - - DEPENDENCIES - foo! - - BUNDLED WITH - #{Bundler::VERSION} - L - end - end - - context "as a development dependency" do - let(:platform_specific_type) { :development } - - it "keeps java dependencies in the lockfile" do - expect(the_bundle).to include_gems "foo 1.0", "platform_specific 1.0 RUBY" - expect(lockfile).to eq normalize_uri_file(strip_whitespace(<<-L)) - GEM - remote: file://localhost#{gem_repo2}/ - specs: - platform_specific (1.0) - platform_specific (1.0-java) - - PATH - remote: . - specs: - foo (1.0) - - PLATFORMS - java - ruby - - DEPENDENCIES - foo! - platform_specific - - BUNDLED WITH - #{Bundler::VERSION} - L - end - end - - context "with an indirect platform-specific development dependency" do - let(:platform_specific_type) { :development } - let(:dependency) { "indirect_platform_specific" } - - it "keeps java dependencies in the lockfile" do - expect(the_bundle).to include_gems "foo 1.0", "indirect_platform_specific 1.0", "platform_specific 1.0 RUBY" - expect(lockfile).to eq normalize_uri_file(strip_whitespace(<<-L)) - GEM - remote: file://localhost#{gem_repo2}/ - specs: - indirect_platform_specific (1.0) - platform_specific - platform_specific (1.0) - platform_specific (1.0-java) - - PATH - remote: . - specs: - foo (1.0) - - PLATFORMS - java - ruby - - DEPENDENCIES - foo! - indirect_platform_specific - - BUNDLED WITH - #{Bundler::VERSION} - L - end - end - end end end diff --git a/spec/lock/lockfile_spec.rb b/spec/lock/lockfile_spec.rb index 0809258859..f98ff11dd7 100644 --- a/spec/lock/lockfile_spec.rb +++ b/spec/lock/lockfile_spec.rb @@ -466,15 +466,15 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - GIT remote: #{lib_path("foo-1.0")} revision: #{git.ref_for("master")} specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -535,15 +535,15 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - GIT remote: #{lib_path("foo-1.0")} revision: #{git.ref_for("master")} specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -564,9 +564,6 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - GIT remote: #{lib_path("foo-1.0")} revision: #{git.ref_for("omg")} @@ -574,6 +571,9 @@ RSpec.describe "the lockfile format", :bundler => "2" do specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -594,9 +594,6 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - GIT remote: #{lib_path("foo-1.0")} revision: #{git.ref_for("omg")} @@ -604,6 +601,9 @@ RSpec.describe "the lockfile format", :bundler => "2" do specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -623,14 +623,14 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - PATH remote: #{lib_path("foo-1.0")} specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -653,14 +653,14 @@ RSpec.describe "the lockfile format", :bundler => "2" do bundle! :install, :local => true lockfile_should_be <<-G - GEM - specs: - PATH remote: #{lib_path("foo-1.0")} specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -685,11 +685,6 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - remote: file://localhost#{gem_repo1}/ - specs: - rack (1.0.0) - GIT remote: #{lib_path("bar-1.0")} revision: #{bar.ref_for("master")} @@ -701,6 +696,11 @@ RSpec.describe "the lockfile format", :bundler => "2" do specs: foo (1.0) + GEM + remote: file://localhost#{gem_repo1}/ + specs: + rack (1.0.0) + PLATFORMS #{lockfile_platforms} @@ -876,14 +876,14 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - PATH remote: foo specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -905,14 +905,14 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - PATH remote: ../foo specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -934,14 +934,14 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - PATH remote: foo specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -961,14 +961,14 @@ RSpec.describe "the lockfile format", :bundler => "2" do G lockfile_should_be <<-G - GEM - specs: - PATH remote: ../foo specs: foo (1.0) + GEM + specs: + PLATFORMS #{lockfile_platforms} @@ -1248,10 +1248,6 @@ RSpec.describe "the lockfile format", :bundler => "2" do # Create a Gemfile.lock that has duplicate GIT sections lockfile <<-L - GEM - remote: file://localhost#{gem_repo1}/ - specs: - GIT remote: #{lib_path("omg")} revision: #{revision} @@ -1266,6 +1262,10 @@ RSpec.describe "the lockfile format", :bundler => "2" do specs: omg (1.0) + GEM + remote: file://localhost#{gem_repo1}/ + specs: + PLATFORMS #{lockfile_platforms} @@ -1282,10 +1282,6 @@ RSpec.describe "the lockfile format", :bundler => "2" do # Confirm that duplicate specs do not appear lockfile_should_be(<<-L) - GEM - remote: file://localhost#{gem_repo1}/ - specs: - GIT remote: #{lib_path("omg")} revision: #{revision} @@ -1293,6 +1289,10 @@ RSpec.describe "the lockfile format", :bundler => "2" do specs: omg (1.0) + GEM + remote: file://localhost#{gem_repo1}/ + specs: + PLATFORMS #{lockfile_platforms} diff --git a/spec/plugins/source/example_spec.rb b/spec/plugins/source/example_spec.rb index fd30892f63..4b410c3035 100644 --- a/spec/plugins/source/example_spec.rb +++ b/spec/plugins/source/example_spec.rb @@ -96,16 +96,16 @@ RSpec.describe "real source plugins" do bundle "install" lockfile_should_be <<-G - GEM - remote: file://localhost#{gem_repo2}/ - specs: - PLUGIN SOURCE remote: #{lib_path("a-path-gem-1.0")} type: mpath specs: a-path-gem (1.0) + GEM + remote: file://localhost#{gem_repo2}/ + specs: + PLATFORMS #{lockfile_platforms} @@ -391,10 +391,6 @@ RSpec.describe "real source plugins" do bundle "install" lockfile_should_be <<-G - GEM - remote: file://localhost#{gem_repo2}/ - specs: - PLUGIN SOURCE remote: file://#{lib_path("ma-gitp-gem-1.0")} type: gitp @@ -402,6 +398,10 @@ RSpec.describe "real source plugins" do specs: ma-gitp-gem (1.0) + GEM + remote: file://localhost#{gem_repo2}/ + specs: + PLATFORMS #{lockfile_platforms} diff --git a/spec/update/git_spec.rb b/spec/update/git_spec.rb index 7cb9f09377..1d5bce2758 100644 --- a/spec/update/git_spec.rb +++ b/spec/update/git_spec.rb @@ -348,17 +348,17 @@ RSpec.describe "bundle update" do bundle "update --source bar" lockfile_should_be <<-G - GEM - remote: file://localhost#{gem_repo2}/ - specs: - rack (1.0.0) - GIT remote: #{@git.path} revision: #{ref} specs: foo (2.0) + GEM + remote: file://localhost#{gem_repo2}/ + specs: + rack (1.0.0) + PLATFORMS #{lockfile_platforms} |