Skip to content

Load plugin immediately #6673

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 25, 2023
Merged

Load plugin immediately #6673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/rubygems/installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ def install

Gem::Specification.add_spec(spec)

load_plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

If run_post_install_hooks happened before load_plugin, then any post install hooks potentially registered by the gem would be run immediately too, right?

For example, say the rdoc gem installs a plugin that generates documentation after gems are installed, then gem install rdoc would immediately generate docs for rdoc itself.

Did you consider this? Do you think it would be desirable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry. I didn't consider this.

rdoc (and yard) use done_installing hook not post_install hook but I can understand what you want to say.

I'll move load_plugin BEFORE run_post_install_hooks.

Copy link
Member Author

@kou kou May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done.

BTW, I think that we can remove https://github.com/rubygems/rubygems/blob/master/lib/rubygems/rdoc.rb and implement this by RubyGems plugin in RDoc after we merge this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! I'm not fully sure which behavior is best, but I guess running hooks for the gem right after installing is inline with the spirit of this PR.

Regarding RDoc, I think that's what was proposed by #6021, and it makes sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't notice #6021.
I'll work on it after we merge this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation: ruby/rdoc#1171


run_post_install_hooks

spec
Expand Down Expand Up @@ -1006,4 +1008,17 @@ def bash_prolog_script
""
end
end

def load_plugin
specs = Gem::Specification.find_all_by_name(spec.name)
# If old version already exists, this plugin isn't loaded
# immediately. It's for avoiding a case that multiple versions
# are loaded at the same time.
return unless specs.size == 1

plugin_files = spec.plugins.map do |plugin|
File.join(@plugins_dir, "#{spec.name}_plugin#{File.extname(plugin)}")
end
Gem.load_plugin_files(plugin_files)
end
end
7 changes: 7 additions & 0 deletions test/rubygems/test_gem.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1449,6 +1449,8 @@ def test_self_vendor_dir_missing
def test_load_plugins
plugin_path = File.join "lib", "rubygems_plugin.rb"

foo1_plugin_path = nil
foo2_plugin_path = nil
Dir.chdir @tempdir do
FileUtils.mkdir_p "lib"
File.open plugin_path, "w" do |fp|
Expand All @@ -1458,17 +1460,22 @@ def test_load_plugins
foo1 = util_spec "foo", "1" do |s|
s.files << plugin_path
end
foo1_plugin_path = File.join(foo1.gem_dir, plugin_path)

install_gem foo1

foo2 = util_spec "foo", "2" do |s|
s.files << plugin_path
end
foo2_plugin_path = File.join(foo2.gem_dir, plugin_path)

install_gem foo2
end

Gem::Specification.reset
PLUGINS_LOADED.clear
$LOADED_FEATURES.delete(foo1_plugin_path)
$LOADED_FEATURES.delete(foo2_plugin_path)

gem "foo"

Expand Down
2 changes: 1 addition & 1 deletion test/rubygems/test_gem_commands_pristine_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ def test_execute_only_plugins
fp.puts "puts __FILE__"
end
write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |fp|
fp.puts "puts __FILE__"
fp.puts "# do nothing"
end
write_file File.join(@tempdir, "bin", "foo") do |fp|
fp.puts "#!/usr/bin/ruby"
Expand Down
2 changes: 1 addition & 1 deletion test/rubygems/test_gem_commands_setup_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ def gem_install_with_plugin(name)
s.files = %W[lib/rubygems_plugin.rb]
end
write_file File.join @tempdir, "lib", "rubygems_plugin.rb" do |f|
f.puts "require '#{gem.plugins.first}'"
f.puts "# do nothing"
end
install_gem gem

Expand Down
52 changes: 50 additions & 2 deletions test/rubygems/test_gem_installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ def test_generate_bin_with_dangling_symlink
def test_generate_plugins
installer = util_setup_installer do |spec|
write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |io|
io.write "puts __FILE__"
io.write "# do nothing"
end

spec.files += %w[lib/rubygems_plugin.rb]
Expand Down Expand Up @@ -856,11 +856,59 @@ def test_generate_plugins_with_build_root
refute_includes File.read(build_root_path), build_root
end

class << self
attr_accessor :plugin_loaded
attr_accessor :post_install_is_called
end

def test_use_plugin_immediately
self.class.plugin_loaded = false
self.class.post_install_is_called = false
spec_version = nil
plugin_path = nil
installer = util_setup_installer do |spec|
spec_version = spec.version
plugin_path = File.join("lib", "rubygems_plugin.rb")
write_file File.join(@tempdir, plugin_path) do |io|
io.write <<-PLUGIN
#{self.class}.plugin_loaded = true
Gem.post_install do
#{self.class}.post_install_is_called = true
end
PLUGIN
end
spec.files += [plugin_path]
plugin_path = File.join(spec.gem_dir, plugin_path)
end
build_rake_in do
installer.install
end
assert self.class.plugin_loaded, "plugin is not loaded"
assert self.class.post_install_is_called,
"post install hook registered by plugin is not called"

self.class.plugin_loaded = false
$LOADED_FEATURES.delete(plugin_path)
installer_new = util_setup_installer do |spec_new|
spec_new.version = spec_version.version.succ
plugin_path = File.join("lib", "rubygems_plugin.rb")
write_file File.join(@tempdir, plugin_path) do |io|
io.write "#{self.class}.plugin_loaded = true"
end
spec_new.files += [plugin_path]
end
build_rake_in do
installer_new.install
end
assert !self.class.plugin_loaded,
"plugin is loaded even when old version is already loaded"
end

def test_keeps_plugins_up_to_date
# NOTE: version a-2 is already installed by setup hooks

write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |io|
io.write "puts __FILE__"
io.write "# do nothing"
end

build_rake_in do
Expand Down
8 changes: 4 additions & 4 deletions test/rubygems/test_gem_uninstaller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ def test_remove_symlinked_gem_home

def test_remove_plugins
write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |io|
io.write "puts __FILE__"
io.write "# do nothing"
end

@spec.files += %w[lib/rubygems_plugin.rb]
Expand All @@ -190,7 +190,7 @@ def test_remove_plugins

def test_remove_plugins_with_install_dir
write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |io|
io.write "puts __FILE__"
io.write "# do nothing"
end

@spec.files += %w[lib/rubygems_plugin.rb]
Expand All @@ -208,7 +208,7 @@ def test_remove_plugins_with_install_dir

def test_regenerate_plugins_for
write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |io|
io.write "puts __FILE__"
io.write "# do nothing"
end

@spec.files += %w[lib/rubygems_plugin.rb]
Expand Down Expand Up @@ -635,7 +635,7 @@ def test_uninstall_no_permission

def test_uninstall_keeps_plugins_up_to_date
write_file File.join(@tempdir, "lib", "rubygems_plugin.rb") do |io|
io.write "puts __FILE__"
io.write "# do nothing"
end

plugin_path = File.join Gem.plugindir, "a_plugin.rb"
Expand Down