From 085eda71478479b4c80dcbc6d69c83d28e3daa2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Mon, 9 Oct 2023 21:07:15 +0200 Subject: [PATCH 1/7] Remove unused parameter --- bundler/lib/bundler/installer.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundler/lib/bundler/installer.rb b/bundler/lib/bundler/installer.rb index e837f732cf93..5245da2e0e65 100644 --- a/bundler/lib/bundler/installer.rb +++ b/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(Bundler.default_lockfile) end end end From 6a0c03c77fac3260a751ae52d365c670880224c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Thu, 2 Mar 2023 20:18:35 +0100 Subject: [PATCH 2/7] Refactor lockfile generation --- bundler/lib/bundler.rb | 5 +- bundler/lib/bundler/cli/lock.rb | 9 +-- bundler/lib/bundler/definition.rb | 82 ++++++++++++++++--------- bundler/lib/bundler/injector.rb | 2 +- bundler/lib/bundler/installer.rb | 2 +- bundler/lib/bundler/runtime.rb | 2 +- bundler/spec/bundler/definition_spec.rb | 12 ++-- 7 files changed, 70 insertions(+), 44 deletions(-) diff --git a/bundler/lib/bundler.rb b/bundler/lib/bundler.rb index 7bb6690e5241..59a1107bb71d 100644 --- a/bundler/lib/bundler.rb +++ b/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 diff --git a/bundler/lib/bundler/cli/lock.rb b/bundler/lib/bundler/cli/lock.rb index 7247121df551..dac3d2a09a91 100644 --- a/bundler/lib/bundler/cli/lock.rb +++ b/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 diff --git a/bundler/lib/bundler/definition.rb b/bundler/lib/bundler/definition.rb index 0b0e63f77e5e..32a708992542 100644 --- a/bundler/lib/bundler/definition.rb +++ b/bundler/lib/bundler/definition.rb @@ -320,38 +320,24 @@ 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 - - preserve_unknown_sections ||= !updating_major && (Bundler.frozen_bundle? || !(unlocking? || @unlocking_bundler)) + 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 - 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 + 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 Bundler.frozen_bundle? - Bundler.ui.error "Cannot write a changed lockfile while frozen." - return + warn "Passing a file to `Definition#lock` is deprecated. #{suggestion}" 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 +504,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 diff --git a/bundler/lib/bundler/injector.rb b/bundler/lib/bundler/injector.rb index 2cf7754ecb53..cf561c2ee49d 100644 --- a/bundler/lib/bundler/injector.rb +++ b/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! diff --git a/bundler/lib/bundler/installer.rb b/bundler/lib/bundler/installer.rb index 5245da2e0e65..018324f8402e 100644 --- a/bundler/lib/bundler/installer.rb +++ b/bundler/lib/bundler/installer.rb @@ -261,7 +261,7 @@ def resolve_if_needed(options) end def lock - @definition.lock(Bundler.default_lockfile) + @definition.lock end end end diff --git a/bundler/lib/bundler/runtime.rb b/bundler/lib/bundler/runtime.rb index 993f1082c338..ec772cfe7b2e 100644 --- a/bundler/lib/bundler/runtime.rb +++ b/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 diff --git a/bundler/spec/bundler/definition_spec.rb b/bundler/spec/bundler/definition_spec.rb index a19cf789ee9b..85f13d287c2c 100644 --- a/bundler/spec/bundler/definition_spec.rb +++ b/bundler/spec/bundler/definition_spec.rb @@ -10,34 +10,34 @@ 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("Gemfile.lock", [], Bundler::SourceList.new, []) } it "raises an PermissionError with explanation" do allow(File).to receive(:open).and_call_original expect(File).to receive(:open).with("Gemfile.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, []) } + subject { Bundler::Definition.new("Gemfile.lock", [], 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"). 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, []) } + subject { Bundler::Definition.new("Gemfile.lock", [], 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") + subject.lock expect(File.file?("Gemfile.lock")).to eq false end end From 331c415af030899cf8b38d6ce135651e57fc3009 Mon Sep 17 00:00:00 2001 From: David Rodriguez Date: Fri, 26 Jan 2024 09:11:47 +0100 Subject: [PATCH 3/7] Move `subject` to top level context --- bundler/spec/bundler/definition_spec.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bundler/spec/bundler/definition_spec.rb b/bundler/spec/bundler/definition_spec.rb index 85f13d287c2c..5e66e012c084 100644 --- a/bundler/spec/bundler/definition_spec.rb +++ b/bundler/spec/bundler/definition_spec.rb @@ -9,9 +9,10 @@ allow(Bundler::SharedHelpers).to receive(:find_gemfile) { Pathname.new("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("Gemfile.lock", [], Bundler::SourceList.new, []) } + subject { Bundler::Definition.new("Gemfile.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"). @@ -21,8 +22,6 @@ end end context "when a temporary resource access issue occurs" do - subject { Bundler::Definition.new("Gemfile.lock", [], 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"). @@ -32,7 +31,6 @@ end end context "when Bundler::Definition.no_lock is set to true" do - subject { Bundler::Definition.new("Gemfile.lock", [], Bundler::SourceList.new, []) } before { Bundler::Definition.no_lock = true } after { Bundler::Definition.no_lock = false } From 54948e428d0dc830341d53e92dc657c451046a74 Mon Sep 17 00:00:00 2001 From: David Rodriguez Date: Fri, 26 Jan 2024 09:21:24 +0100 Subject: [PATCH 4/7] Fix incorrect 4th parameter to Definition.new --- bundler/spec/bundler/definition_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundler/spec/bundler/definition_spec.rb b/bundler/spec/bundler/definition_spec.rb index 5e66e012c084..c4a0f0cdb8d4 100644 --- a/bundler/spec/bundler/definition_spec.rb +++ b/bundler/spec/bundler/definition_spec.rb @@ -10,7 +10,7 @@ allow(Bundler).to receive(:ui) { double("UI", info: "", debug: "") } end - subject { Bundler::Definition.new("Gemfile.lock", [], Bundler::SourceList.new, []) } + subject { Bundler::Definition.new("Gemfile.lock", [], Bundler::SourceList.new, {}) } context "when it's not possible to write to the file" do it "raises an PermissionError with explanation" do From 660def5b685362bdabace9a8b07e6db69ed9467c Mon Sep 17 00:00:00 2001 From: David Rodriguez Date: Fri, 26 Jan 2024 09:26:31 +0100 Subject: [PATCH 5/7] Run definition specs in an isolated location And consistently pass Pathname's to `Definition.new` like production code does. --- bundler/spec/bundler/definition_spec.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bundler/spec/bundler/definition_spec.rb b/bundler/spec/bundler/definition_spec.rb index c4a0f0cdb8d4..7bc48fce05c8 100644 --- a/bundler/spec/bundler/definition_spec.rb +++ b/bundler/spec/bundler/definition_spec.rb @@ -5,17 +5,16 @@ 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 - subject { Bundler::Definition.new("Gemfile.lock", [], 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 }. to raise_error(Bundler::PermissionError, /Gemfile\.lock/) @@ -24,7 +23,7 @@ context "when a temporary resource access issue occurs" do 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 }. to raise_error(Bundler::TemporaryResourceError, /temporarily unavailable/) From d1963bf1a6db7698f3a295dc2b4a7b04d2d318c7 Mon Sep 17 00:00:00 2001 From: David Rodriguez Date: Fri, 26 Jan 2024 10:41:00 +0100 Subject: [PATCH 6/7] Use deprecation helper for deprecation warning --- bundler/lib/bundler/definition.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bundler/lib/bundler/definition.rb b/bundler/lib/bundler/definition.rb index 32a708992542..c8faf77b3bfd 100644 --- a/bundler/lib/bundler/definition.rb +++ b/bundler/lib/bundler/definition.rb @@ -334,7 +334,9 @@ def lock(file_or_preserve_unknown_sections = false, preserve_unknown_sections_or "Instead, instantiate a new definition passing `#{target_lockfile}`, and call `lock` without a file argument on that definition" end - warn "Passing a file to `Definition#lock` is deprecated. #{suggestion}" + msg = "`Definition#lock` was passed a target file argument. #{suggestion}" + + Bundler::SharedHelpers.major_deprecation 2, msg end write_lock(target_lockfile, preserve_unknown_sections) From 7f2f2b898c57539d1ad19654b1795c2fc4261988 Mon Sep 17 00:00:00 2001 From: David Rodriguez Date: Fri, 26 Jan 2024 20:37:22 +0100 Subject: [PATCH 7/7] Improve assertion Co-authored-by: Martin Emde --- bundler/spec/bundler/definition_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundler/spec/bundler/definition_spec.rb b/bundler/spec/bundler/definition_spec.rb index 7bc48fce05c8..28c04e086003 100644 --- a/bundler/spec/bundler/definition_spec.rb +++ b/bundler/spec/bundler/definition_spec.rb @@ -35,7 +35,7 @@ it "does not create a lock file" do subject.lock - expect(File.file?("Gemfile.lock")).to eq false + expect(bundled_app_lock).not_to be_file end end end