Merge pull request #7047 from rubygems/refactor-lockfile-generation

Refactor lockfile generation and deprecate `Definition#lock` with explicit lockfile

bundler/lib/bundler.rb

@@ -200,12 +200,13 @@ def environment
     #
     # @param unlock [Hash, Boolean, nil] Gems that have been requested
     #   to be updated or true if all gems should be updated
+    # @param lockfile [Pathname] Path to Gemfile.lock
     # @return [Bundler::Definition]
-    def definition(unlock = nil)
+    def definition(unlock = nil, lockfile = default_lockfile)
       @definition = nil if unlock
       @definition ||= begin
         configure
-        Definition.build(default_gemfile, default_lockfile, unlock)
+        Definition.build(default_gemfile, lockfile, unlock)
       end
     end
 

bundler/lib/bundler/cli/lock.rb

@@ -33,8 +33,11 @@ def run
         update = { bundler: bundler }
       end
 
+      file = options[:lockfile]
+      file = file ? Pathname.new(file).expand_path : Bundler.default_lockfile
+
       Bundler.settings.temporary(frozen: false) do
-        definition = Bundler.definition(update)
+        definition = Bundler.definition(update, file)
 
         Bundler::CLI::Common.configure_gem_version_promoter(definition, options) if options[:update]
 
@@ -60,10 +63,8 @@ def run
         if print
           puts definition.to_lock
         else
-          file = options[:lockfile]
-          file = file ? File.expand_path(file) : Bundler.default_lockfile
           puts "Writing lockfile to #{file}"
-          definition.lock(file)
+          definition.lock
         end
       end
 

bundler/lib/bundler/definition.rb

@@ -320,38 +320,26 @@ def groups
       dependencies.map(&:groups).flatten.uniq
     end
 
-    def lock(file, preserve_unknown_sections = false)
-      return if Definition.no_lock
-
-      contents = to_lock
-
-      # Convert to \r\n if the existing lock has them
-      # i.e., Windows with `git config core.autocrlf=true`
-      contents.gsub!(/\n/, "\r\n") if @lockfile_contents.match?("\r\n")
-
-      if @locked_bundler_version
-        locked_major = @locked_bundler_version.segments.first
-        current_major = bundler_version_to_lock.segments.first
-
-        updating_major = locked_major < current_major
-      end
+    def lock(file_or_preserve_unknown_sections = false, preserve_unknown_sections_or_unused = false)
+      if [true, false, nil].include?(file_or_preserve_unknown_sections)
+        target_lockfile = lockfile || Bundler.default_lockfile
+        preserve_unknown_sections = file_or_preserve_unknown_sections
+      else
+        target_lockfile = file_or_preserve_unknown_sections
+        preserve_unknown_sections = preserve_unknown_sections_or_unused
 
-      preserve_unknown_sections ||= !updating_major && (Bundler.frozen_bundle? || !(unlocking? || @unlocking_bundler))
+        suggestion = if target_lockfile == lockfile
+          "To fix this warning, remove it from the `Definition#lock` call."
+        else
+          "Instead, instantiate a new definition passing `#{target_lockfile}`, and call `lock` without a file argument on that definition"
+        end
 
-      if file && File.exist?(file) && lockfiles_equal?(@lockfile_contents, contents, preserve_unknown_sections)
-        return if Bundler.frozen_bundle?
-        SharedHelpers.filesystem_access(file) { FileUtils.touch(file) }
-        return
-      end
+        msg = "`Definition#lock` was passed a target file argument. #{suggestion}"
 
-      if Bundler.frozen_bundle?
-        Bundler.ui.error "Cannot write a changed lockfile while frozen."
-        return
+        Bundler::SharedHelpers.major_deprecation 2, msg
       end
 
-      SharedHelpers.filesystem_access(file) do |p|
-        File.open(p, "wb") {|f| f.puts(contents) }
-      end
+      write_lock(target_lockfile, preserve_unknown_sections)
     end
 
     def locked_ruby_version
@@ -518,7 +506,45 @@ def should_add_extra_platforms?
     end
 
     def lockfile_exists?
-      lockfile && File.exist?(lockfile)
+      file_exists?(lockfile)
+    end
+
+    def file_exists?(file)
+      file && File.exist?(file)
+    end
+
+    def write_lock(file, preserve_unknown_sections)
+      return if Definition.no_lock
+
+      contents = to_lock
+
+      # Convert to \r\n if the existing lock has them
+      # i.e., Windows with `git config core.autocrlf=true`
+      contents.gsub!(/\n/, "\r\n") if @lockfile_contents.match?("\r\n")
+
+      if @locked_bundler_version
+        locked_major = @locked_bundler_version.segments.first
+        current_major = bundler_version_to_lock.segments.first
+
+        updating_major = locked_major < current_major
+      end
+
+      preserve_unknown_sections ||= !updating_major && (Bundler.frozen_bundle? || !(unlocking? || @unlocking_bundler))
+
+      if file_exists?(file) && lockfiles_equal?(@lockfile_contents, contents, preserve_unknown_sections)
+        return if Bundler.frozen_bundle?
+        SharedHelpers.filesystem_access(file) { FileUtils.touch(file) }
+        return
+      end
+
+      if Bundler.frozen_bundle?
+        Bundler.ui.error "Cannot write a changed lockfile while frozen."
+        return
+      end
+
+      SharedHelpers.filesystem_access(file) do |p|
+        File.open(p, "wb") {|f| f.puts(contents) }
+      end
     end
 
     def resolver

bundler/lib/bundler/injector.rb

@@ -50,7 +50,7 @@ def inject(gemfile_path, lockfile_path)
         append_to(gemfile_path, build_gem_lines(@options[:conservative_versioning])) if @deps.any?
 
         # since we resolved successfully, write out the lockfile
-        @definition.lock(Bundler.default_lockfile)
+        @definition.lock
 
         # invalidate the cached Bundler.definition
         Bundler.reset_paths!

bundler/lib/bundler/installer.rb

@@ -260,8 +260,8 @@ def resolve_if_needed(options)
       true
     end
 
-    def lock(opts = {})
-      @definition.lock(Bundler.default_lockfile, opts[:preserve_unknown_sections])
+    def lock
+      @definition.lock
     end
   end
 end

bundler/lib/bundler/runtime.rb

@@ -95,7 +95,7 @@ def self.definition_method(meth)
 
     def lock(opts = {})
       return if @definition.no_resolve_needed?
-      @definition.lock(Bundler.default_lockfile, opts[:preserve_unknown_sections])
+      @definition.lock(opts[:preserve_unknown_sections])
     end
 
     alias_method :gems, :specs

bundler/spec/bundler/definition_spec.rb

@@ -5,40 +5,37 @@
 RSpec.describe Bundler::Definition do
   describe "#lock" do
     before do
-      allow(Bundler).to receive(:settings) { Bundler::Settings.new(".") }
-      allow(Bundler::SharedHelpers).to receive(:find_gemfile) { Pathname.new("Gemfile") }
+      allow(Bundler::SharedHelpers).to receive(:find_gemfile) { bundled_app_gemfile }
       allow(Bundler).to receive(:ui) { double("UI", info: "", debug: "") }
     end
-    context "when it's not possible to write to the file" do
-      subject { Bundler::Definition.new(nil, [], Bundler::SourceList.new, []) }
 
+    subject { Bundler::Definition.new(bundled_app_lock, [], Bundler::SourceList.new, {}) }
+
+    context "when it's not possible to write to the file" do
       it "raises an PermissionError with explanation" do
         allow(File).to receive(:open).and_call_original
-        expect(File).to receive(:open).with("Gemfile.lock", "wb").
+        expect(File).to receive(:open).with(bundled_app_lock, "wb").
           and_raise(Errno::EACCES)
-        expect { subject.lock("Gemfile.lock") }.
+        expect { subject.lock }.
           to raise_error(Bundler::PermissionError, /Gemfile\.lock/)
       end
     end
     context "when a temporary resource access issue occurs" do
-      subject { Bundler::Definition.new(nil, [], Bundler::SourceList.new, []) }
-
       it "raises a TemporaryResourceError with explanation" do
         allow(File).to receive(:open).and_call_original
-        expect(File).to receive(:open).with("Gemfile.lock", "wb").
+        expect(File).to receive(:open).with(bundled_app_lock, "wb").
           and_raise(Errno::EAGAIN)
-        expect { subject.lock("Gemfile.lock") }.
+        expect { subject.lock }.
           to raise_error(Bundler::TemporaryResourceError, /temporarily unavailable/)
       end
     end
     context "when Bundler::Definition.no_lock is set to true" do
-      subject { Bundler::Definition.new(nil, [], Bundler::SourceList.new, []) }
       before { Bundler::Definition.no_lock = true }
       after { Bundler::Definition.no_lock = false }
 
       it "does not create a lock file" do
-        subject.lock("Gemfile.lock")
-        expect(File.file?("Gemfile.lock")).to eq false
+        subject.lock
+        expect(bundled_app_lock).not_to be_file
       end
     end
   end