Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Plugin] Fix "Installing" bug when install plugin from Gemfile #6643

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 14 additions & 4 deletions bundler/lib/bundler/plugin.rb
Expand Up @@ -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) }
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -253,7 +263,7 @@ def loaded?(plugin)
# @param [Array<String>] 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]

Expand Down
9 changes: 9 additions & 0 deletions bundler/lib/bundler/plugin/index.rb
Expand Up @@ -111,6 +111,15 @@ def command_plugin(command)
@commands[command]
end

# Plugin version is already installed?
def version_already_installed?(name, version)
plugin_path = @plugin_paths[name]
return false unless plugin_path

File.basename(plugin_path) == "#{name}-#{version}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for a plugin "my_plugin", path: "../my_plugin" in your Gemfile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @ccutrer ! Indeed, you're right, I was focusing my attention on the "avoid unnecessary installs" problem and forgot about more things that I could fix. I saw your approach, better solving the problem at the root, congrats.

end

# Plugin is already installed?
def installed?(name)
@plugin_paths[name]
end
Expand Down
13 changes: 12 additions & 1 deletion bundler/lib/bundler/plugin/installer.rb
Expand Up @@ -100,13 +100,24 @@ def install_from_specs(specs)
paths = {}

specs.each do |spec|
spec.source.install spec
next if version_already_installed?(spec.name, spec.version)

spec.source.install spec
paths[spec.name] = spec
end

paths
end

# 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 plugin version already installed
def version_already_installed?(plugin, version)
Index.new.version_already_installed?(plugin, version)
end
end
end
end
113 changes: 112 additions & 1 deletion bundler/spec/plugins/install_spec.rb
Expand Up @@ -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
Expand Down Expand Up @@ -232,6 +232,117 @@ 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_with_version("foo", "1.4.0")

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_with_version("foo", "1.5.0")
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_with_version("foo", "1.5.0")

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_with_version("foo", "1.4.0")
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"
Expand Down
8 changes: 8 additions & 0 deletions bundler/spec/support/matchers.rb
Expand Up @@ -228,6 +228,14 @@ def plugin_should_be_installed(*names)
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)
Expand Down