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>

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}