From 8f844f3591173261617995f8b5db5603114256c8 Mon Sep 17 00:00:00 2001 From: Diogo Fernandes Date: Fri, 28 Apr 2023 17:12:17 -0300 Subject: [PATCH 1/5] [Plugin] Fix bug when install plugin from Gemfile --- bundler/lib/bundler/plugin.rb | 3 ++ bundler/lib/bundler/plugin/index.rb | 11 +++++++ bundler/lib/bundler/plugin/installer.rb | 13 ++++++++- bundler/spec/bundler/plugin_spec.rb | 8 +++++ bundler/spec/plugins/install_spec.rb | 39 +++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 1 deletion(-) diff --git a/bundler/lib/bundler/plugin.rb b/bundler/lib/bundler/plugin.rb index e3f698ec43b3..9bf8cbc1f44e 100644 --- a/bundler/lib/bundler/plugin.rb +++ b/bundler/lib/bundler/plugin.rb @@ -114,6 +114,9 @@ def gemfile_install(gemfile = nil, &inline) return if definition.dependencies.empty? plugins = definition.dependencies.map(&:name).reject {|p| index.installed? p } + + return if plugins.empty? + installed_specs = Installer.new.install_definition(definition) save_plugins plugins, installed_specs, builder.inferred_plugins diff --git a/bundler/lib/bundler/plugin/index.rb b/bundler/lib/bundler/plugin/index.rb index c2ab8f90da4f..d0dfe9013b34 100644 --- a/bundler/lib/bundler/plugin/index.rb +++ b/bundler/lib/bundler/plugin/index.rb @@ -111,6 +111,17 @@ def command_plugin(command) @commands[command] end + # Plugin cannot be installed and updating to install + def cannot_install?(name, version) + installed?(name) && !updating?(name, version) + end + + # An existing plugin must have a diff version to install again + def updating?(name, version) + version.to_s != @plugin_paths[name][/#{name}-(.*)$/, 1].to_s + end + + # Plugin is already installed? def installed?(name) @plugin_paths[name] end diff --git a/bundler/lib/bundler/plugin/installer.rb b/bundler/lib/bundler/plugin/installer.rb index c9ff12ce4b71..530f366553d5 100644 --- a/bundler/lib/bundler/plugin/installer.rb +++ b/bundler/lib/bundler/plugin/installer.rb @@ -100,13 +100,24 @@ def install_from_specs(specs) paths = {} specs.each do |spec| - spec.source.install spec + next if cannot_install?(spec.name, spec.version) + spec.source.install spec paths[spec.name] = spec end paths end + + # Check if the Plugin is already installed or has a diff version to install + # + # @param [String] name of the plugin gem to search in the source + # @param [String] version of the gem to install + # + # @return [Boolean] true if installed or not updating plugin + def cannot_install?(plugin, version) + Index.new.cannot_install?(plugin, version) + end end end end diff --git a/bundler/spec/bundler/plugin_spec.rb b/bundler/spec/bundler/plugin_spec.rb index 1731a2085e41..4c84644aa9cb 100644 --- a/bundler/spec/bundler/plugin_spec.rb +++ b/bundler/spec/bundler/plugin_spec.rb @@ -129,6 +129,14 @@ subject.gemfile_install(gemfile) end + it "doesn't calls installer when plugins already installed" do + allow(index).to receive(:installed?).and_return(true) + allow(definition).to receive(:dependencies) { [Bundler::Dependency.new("new-plugin", ">=0")] } + allow(installer).to receive(:install_definition).never + + subject.gemfile_install(gemfile) + end + context "with dependencies" do let(:plugin_specs) do { diff --git a/bundler/spec/plugins/install_spec.rb b/bundler/spec/plugins/install_spec.rb index aec131f2eeb2..35178763bb9c 100644 --- a/bundler/spec/plugins/install_spec.rb +++ b/bundler/spec/plugins/install_spec.rb @@ -232,6 +232,45 @@ def exec(command, args) plugin_should_be_installed("foo") end + it "install only plugins not installed yet listed in gemfile" do + gemfile <<-G + source '#{file_uri_for(gem_repo2)}' + plugin 'foo' + gem 'rack', "1.0.0" + G + + 2.times { bundle "install" } + + expect(out).to_not include("Installing foo") + expect(out).to_not include("Installed plugin foo") + + expect(out).to include("Bundle complete!") + + expect(the_bundle).to include_gems("rack 1.0.0") + plugin_should_be_installed("foo") + + gemfile <<-G + source '#{file_uri_for(gem_repo2)}' + plugin 'foo' + plugin 'kung-foo' + gem 'rack', "1.0.0" + G + + bundle "install" + + expect(out).to include("Installing kung-foo") + expect(out).to include("Installed plugin kung-foo") + + expect(out).to_not include("Installing foo") + expect(out).to_not include("Installed plugin foo") + + expect(out).to include("Bundle complete!") + + expect(the_bundle).to include_gems("rack 1.0.0") + plugin_should_be_installed("foo") + plugin_should_be_installed("kung-foo") + end + it "accepts plugin version" do update_repo2 do build_plugin "foo", "1.1.0" From 488634d22c7e2e45ff33f2f65f6443f8199fcd66 Mon Sep 17 00:00:00 2001 From: Diogo Fernandes Date: Sun, 28 May 2023 14:14:03 -0300 Subject: [PATCH 2/5] Fix upgrade/downgrade bug and Add more tests --- bundler/lib/bundler/plugin.rb | 3 -- bundler/lib/bundler/plugin/index.rb | 2 +- bundler/lib/bundler/plugin/installer.rb | 8 +-- bundler/spec/plugins/install_spec.rb | 72 +++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 8 deletions(-) diff --git a/bundler/lib/bundler/plugin.rb b/bundler/lib/bundler/plugin.rb index 9bf8cbc1f44e..e3f698ec43b3 100644 --- a/bundler/lib/bundler/plugin.rb +++ b/bundler/lib/bundler/plugin.rb @@ -114,9 +114,6 @@ def gemfile_install(gemfile = nil, &inline) return if definition.dependencies.empty? plugins = definition.dependencies.map(&:name).reject {|p| index.installed? p } - - return if plugins.empty? - installed_specs = Installer.new.install_definition(definition) save_plugins plugins, installed_specs, builder.inferred_plugins diff --git a/bundler/lib/bundler/plugin/index.rb b/bundler/lib/bundler/plugin/index.rb index d0dfe9013b34..f8196778ceb8 100644 --- a/bundler/lib/bundler/plugin/index.rb +++ b/bundler/lib/bundler/plugin/index.rb @@ -112,7 +112,7 @@ def command_plugin(command) end # Plugin cannot be installed and updating to install - def cannot_install?(name, version) + def version_already_installed?(name, version) installed?(name) && !updating?(name, version) end diff --git a/bundler/lib/bundler/plugin/installer.rb b/bundler/lib/bundler/plugin/installer.rb index 530f366553d5..c17db5a5baf4 100644 --- a/bundler/lib/bundler/plugin/installer.rb +++ b/bundler/lib/bundler/plugin/installer.rb @@ -100,7 +100,7 @@ def install_from_specs(specs) paths = {} specs.each do |spec| - next if cannot_install?(spec.name, spec.version) + next if version_already_installed?(spec.name, spec.version) spec.source.install spec paths[spec.name] = spec @@ -109,14 +109,14 @@ def install_from_specs(specs) paths end - # Check if the Plugin is already installed or has a diff version to install + # Check if the Plugin version is already installed or has a diff version to install # # @param [String] name of the plugin gem to search in the source # @param [String] version of the gem to install # # @return [Boolean] true if installed or not updating plugin - def cannot_install?(plugin, version) - Index.new.cannot_install?(plugin, version) + def version_already_installed?(plugin, version) + Index.new.version_already_installed?(plugin, version) end end end diff --git a/bundler/spec/plugins/install_spec.rb b/bundler/spec/plugins/install_spec.rb index 35178763bb9c..fb5bf48431d0 100644 --- a/bundler/spec/plugins/install_spec.rb +++ b/bundler/spec/plugins/install_spec.rb @@ -232,6 +232,78 @@ def exec(command, args) plugin_should_be_installed("foo") end + it "upgrade plugins version listed in gemfile" do + update_repo2 do + build_plugin "foo", "1.4.0" + build_plugin "foo", "1.5.0" + end + + gemfile <<-G + source '#{file_uri_for(gem_repo2)}' + plugin 'foo', "1.4.0" + gem 'rack', "1.0.0" + G + + bundle "install" + + expect(out).to include("Installing foo 1.4.0") + expect(out).to include("Installed plugin foo") + expect(out).to include("Bundle complete!") + + expect(the_bundle).to include_gems("rack 1.0.0") + plugin_should_be_installed("foo") + + gemfile <<-G + source '#{file_uri_for(gem_repo2)}' + plugin 'foo', "1.5.0" + gem 'rack', "1.0.0" + G + + bundle "install" + + expect(out).to include("Installing foo 1.5.0") + expect(out).to include("Bundle complete!") + + expect(the_bundle).to include_gems("rack 1.0.0") + plugin_should_be_installed("foo") + end + + it "downgrade plugins version listed in gemfile" do + update_repo2 do + build_plugin "foo", "1.4.0" + build_plugin "foo", "1.5.0" + end + + gemfile <<-G + source '#{file_uri_for(gem_repo2)}' + plugin 'foo', "1.5.0" + gem 'rack', "1.0.0" + G + + bundle "install" + + expect(out).to include("Installing foo 1.5.0") + expect(out).to include("Installed plugin foo") + expect(out).to include("Bundle complete!") + + expect(the_bundle).to include_gems("rack 1.0.0") + plugin_should_be_installed("foo") + + gemfile <<-G + source '#{file_uri_for(gem_repo2)}' + plugin 'foo', "1.4.0" + gem 'rack', "1.0.0" + G + + bundle "install" + + expect(out).to include("Installing foo 1.4.0") + expect(out).to include("Bundle complete!") + + expect(the_bundle).to include_gems("rack 1.0.0") + plugin_should_be_installed("foo") + end + it "install only plugins not installed yet listed in gemfile" do gemfile <<-G source '#{file_uri_for(gem_repo2)}' From 29c212a68c32a4bc02ba740436badd7b0e2e6779 Mon Sep 17 00:00:00 2001 From: Diogo Fernandes Date: Mon, 5 Jun 2023 19:23:41 -0300 Subject: [PATCH 3/5] Remove missing old spec --- bundler/spec/bundler/plugin_spec.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/bundler/spec/bundler/plugin_spec.rb b/bundler/spec/bundler/plugin_spec.rb index 4c84644aa9cb..1731a2085e41 100644 --- a/bundler/spec/bundler/plugin_spec.rb +++ b/bundler/spec/bundler/plugin_spec.rb @@ -129,14 +129,6 @@ subject.gemfile_install(gemfile) end - it "doesn't calls installer when plugins already installed" do - allow(index).to receive(:installed?).and_return(true) - allow(definition).to receive(:dependencies) { [Bundler::Dependency.new("new-plugin", ">=0")] } - allow(installer).to receive(:install_definition).never - - subject.gemfile_install(gemfile) - end - context "with dependencies" do let(:plugin_specs) do { From 4819d0e53b1ff449701f454b86cc91a3bbb6bc8e Mon Sep 17 00:00:00 2001 From: Diogo Fernandes Date: Thu, 8 Jun 2023 21:13:18 -0300 Subject: [PATCH 4/5] Refactoring updates --- bundler/lib/bundler/plugin.rb | 18 ++++++++++++++---- bundler/lib/bundler/plugin/index.rb | 10 ++++------ bundler/lib/bundler/plugin/installer.rb | 2 +- bundler/spec/plugins/install_spec.rb | 10 +++++----- bundler/spec/support/matchers.rb | 4 +++- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/bundler/lib/bundler/plugin.rb b/bundler/lib/bundler/plugin.rb index e3f698ec43b3..95f9d3894672 100644 --- a/bundler/lib/bundler/plugin.rb +++ b/bundler/lib/bundler/plugin.rb @@ -39,8 +39,9 @@ def install(names, options) raise InvalidOption, "You cannot specify `--branch` and `--ref` at the same time." if options["branch"] && options["ref"] specs = Installer.new.install(names, options) + plugins = plugins_to_save(names, specs) - save_plugins names, specs + save_plugins(plugins, specs) rescue PluginError specs_to_delete = specs.select {|k, _v| names.include?(k) && !index.commands.values.include?(k) } specs_to_delete.each_value {|spec| Bundler.rm_rf(spec.full_gem_path) } @@ -113,10 +114,10 @@ def gemfile_install(gemfile = nil, &inline) return if definition.dependencies.empty? - plugins = definition.dependencies.map(&:name).reject {|p| index.installed? p } installed_specs = Installer.new.install_definition(definition) + plugins = plugins_to_save(definition.dependencies.map(&:name), installed_specs) - save_plugins plugins, installed_specs, builder.inferred_plugins + save_plugins(plugins, installed_specs, builder.inferred_plugins) end rescue RuntimeError => e unless e.is_a?(GemfileError) @@ -142,6 +143,15 @@ def root end end + # Return array of plugins that we need save or update the path + # based if was not installed with that version yet + def plugins_to_save(plugins, installed_specs) + plugins.reject do |p| + installed_spec = installed_specs[p] + installed_spec.nil? || index.version_already_installed?(p, installed_spec.version) + end + end + def local_root Bundler.app_config_path.join("plugin") end @@ -253,7 +263,7 @@ def loaded?(plugin) # @param [Array] names of inferred source plugins that can be ignored def save_plugins(plugins, specs, optional_plugins = []) plugins.each do |name| - next if index.installed?(name) + next if index.version_already_installed?(name, specs[name].version) spec = specs[name] diff --git a/bundler/lib/bundler/plugin/index.rb b/bundler/lib/bundler/plugin/index.rb index f8196778ceb8..724ee708a592 100644 --- a/bundler/lib/bundler/plugin/index.rb +++ b/bundler/lib/bundler/plugin/index.rb @@ -111,14 +111,12 @@ def command_plugin(command) @commands[command] end - # Plugin cannot be installed and updating to install + # Plugin version is already installed? def version_already_installed?(name, version) - installed?(name) && !updating?(name, version) - end + plugin_path = @plugin_paths[name] + return false unless plugin_path - # An existing plugin must have a diff version to install again - def updating?(name, version) - version.to_s != @plugin_paths[name][/#{name}-(.*)$/, 1].to_s + File.basename(plugin_path) == "#{name}-#{version}" end # Plugin is already installed? diff --git a/bundler/lib/bundler/plugin/installer.rb b/bundler/lib/bundler/plugin/installer.rb index c17db5a5baf4..c26500f732b2 100644 --- a/bundler/lib/bundler/plugin/installer.rb +++ b/bundler/lib/bundler/plugin/installer.rb @@ -114,7 +114,7 @@ def install_from_specs(specs) # @param [String] name of the plugin gem to search in the source # @param [String] version of the gem to install # - # @return [Boolean] true if installed or not updating plugin + # @return [Boolean] true if plugin version already installed def version_already_installed?(plugin, version) Index.new.version_already_installed?(plugin, version) end diff --git a/bundler/spec/plugins/install_spec.rb b/bundler/spec/plugins/install_spec.rb index fb5bf48431d0..047c92098c45 100644 --- a/bundler/spec/plugins/install_spec.rb +++ b/bundler/spec/plugins/install_spec.rb @@ -89,7 +89,7 @@ expect(out).to include("Installing foo 1.1") bundle "plugin install foo --source #{file_uri_for(gem_repo2)} --verbose" - expect(out).to include("Using foo 1.1") + expect(out).to_not include("foo 1.1") end it "installs when --branch specified" do @@ -251,7 +251,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo") + plugin_should_be_installed("foo", version: "1.4.0") gemfile <<-G source '#{file_uri_for(gem_repo2)}' @@ -265,7 +265,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo") + plugin_should_be_installed("foo", version: "1.5.0") end it "downgrade plugins version listed in gemfile" do @@ -287,7 +287,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo") + plugin_should_be_installed("foo", version: "1.5.0") gemfile <<-G source '#{file_uri_for(gem_repo2)}' @@ -301,7 +301,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo") + plugin_should_be_installed("foo", version: "1.4.0") end it "install only plugins not installed yet listed in gemfile" do diff --git a/bundler/spec/support/matchers.rb b/bundler/spec/support/matchers.rb index ea7c78468351..e7e2c1d38b21 100644 --- a/bundler/spec/support/matchers.rb +++ b/bundler/spec/support/matchers.rb @@ -220,10 +220,12 @@ def indent(string, padding = 4, indent_character = " ") RSpec::Matchers.define_negated_matcher :not_include_gems, :include_gems RSpec::Matchers.alias_matcher :include_gem, :include_gems - def plugin_should_be_installed(*names) + def plugin_should_be_installed(*names, version: nil) names.each do |name| expect(Bundler::Plugin).to be_installed(name) path = Pathname.new(Bundler::Plugin.installed?(name)) + + expect(File.basename(path)).to eq("#{name}-#{version}") unless version.nil? expect(path + "plugins.rb").to exist end end From 9d86879f62c84b3eba5a9f6bc397b306ffd42c97 Mon Sep 17 00:00:00 2001 From: Diogo Fernandes Date: Thu, 13 Jul 2023 12:07:31 -0300 Subject: [PATCH 5/5] create a separate matcher to avoid failures --- bundler/spec/plugins/install_spec.rb | 8 ++++---- bundler/spec/support/matchers.rb | 12 +++++++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/bundler/spec/plugins/install_spec.rb b/bundler/spec/plugins/install_spec.rb index 047c92098c45..01707d20c71b 100644 --- a/bundler/spec/plugins/install_spec.rb +++ b/bundler/spec/plugins/install_spec.rb @@ -251,7 +251,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo", version: "1.4.0") + plugin_should_be_installed_with_version("foo", "1.4.0") gemfile <<-G source '#{file_uri_for(gem_repo2)}' @@ -265,7 +265,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo", version: "1.5.0") + plugin_should_be_installed_with_version("foo", "1.5.0") end it "downgrade plugins version listed in gemfile" do @@ -287,7 +287,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo", version: "1.5.0") + plugin_should_be_installed_with_version("foo", "1.5.0") gemfile <<-G source '#{file_uri_for(gem_repo2)}' @@ -301,7 +301,7 @@ def exec(command, args) expect(out).to include("Bundle complete!") expect(the_bundle).to include_gems("rack 1.0.0") - plugin_should_be_installed("foo", version: "1.4.0") + plugin_should_be_installed_with_version("foo", "1.4.0") end it "install only plugins not installed yet listed in gemfile" do diff --git a/bundler/spec/support/matchers.rb b/bundler/spec/support/matchers.rb index e7e2c1d38b21..c2d2abd5321f 100644 --- a/bundler/spec/support/matchers.rb +++ b/bundler/spec/support/matchers.rb @@ -220,16 +220,22 @@ def indent(string, padding = 4, indent_character = " ") RSpec::Matchers.define_negated_matcher :not_include_gems, :include_gems RSpec::Matchers.alias_matcher :include_gem, :include_gems - def plugin_should_be_installed(*names, version: nil) + def plugin_should_be_installed(*names) names.each do |name| expect(Bundler::Plugin).to be_installed(name) path = Pathname.new(Bundler::Plugin.installed?(name)) - - expect(File.basename(path)).to eq("#{name}-#{version}") unless version.nil? expect(path + "plugins.rb").to exist end end + def plugin_should_be_installed_with_version(name, version) + expect(Bundler::Plugin).to be_installed(name) + path = Pathname.new(Bundler::Plugin.installed?(name)) + + expect(File.basename(path)).to eq("#{name}-#{version}") + expect(path + "plugins.rb").to exist + end + def plugin_should_not_be_installed(*names) names.each do |name| expect(Bundler::Plugin).not_to be_installed(name)