Fix plugins uninstallation for user installed gems #6456

voxik · 2023-03-09T10:30:15Z

What was the end-user or developer problem that led to this PR?

Originally, when the plugins system was reworked as part of #3108, the plugin directory was considered global. But that is mistake. The plugins are always related to specific gems and therefore the directories should be handled via Gem::Specification. This will prevent issues such as leftovers after gem uninstallation as demonstred by #6452:

$ gem uni rubygems-server
Successfully uninstalled rubygems-server-0.3.0

$ gem install rubygems-server --document=ri,rdoc
Error loading RubyGems plugin "/builddir/.local/share/gem/ruby/plugins/rubygems-server_plugin.rb": cannot load such file -- /builddir/.local/share/gem/ruby/gems/rubygems-server-0.3.0/lib/rubygems_plugin.rb (LoadError) Fetching rubygems-server-0.3.0.gem
WARNING:  You don't have /builddir/bin in your PATH,
	  gem executables will not run.
Successfully installed rubygems-server-0.3.0
Parsing documentation for rubygems-server-0.3.0
Installing ri documentation for rubygems-server-0.3.0 Installing fedora::darkfish documentation for rubygems-server-0.3.0 Done installing documentation for rubygems-server after 0 seconds 1 gem installed

What is your fix for the problem, implemented in this PR?

Gem.Specification should be responsible for providing plugins path.

Fixes #6452.

voxik · 2023-03-09T10:42:21Z

This is just initial attempt, I have not even run the test suite locally.

But there is more unfortunately.

I think that the Gem::InstallerUninstallerUtils could be dropped again and the methos should be moved to Gem::Specification
Not sure what to do about cb88467 because this treats just one location, while it should treat all locations from GEM_PATH or should not bother.

voxik · 2023-03-09T12:16:55Z

lib/rubygems/installer.rb

@@ -514,12 +513,10 @@ def generate_plugins # :nodoc:
    latest = Gem::Specification.latest_spec_for(spec.name)
    return if latest && latest.version > spec.version

-    ensure_writable_dir @plugins_dir


This was introduced in #3144, but I don't think this is the right place. If directories are missing during RubyGems update, then RubyGems update should fix them. No other directory (with exception to bindir, which is possibly out of RubyGems structure) is ensured this way.

On top of this, now it causes unfortunate test failures :/

voxik · 2023-03-09T17:42:15Z

I'm starting to be afraid there is no way to cover the whole dependency matrix between RubyGems/Bundler 😭

voxik · 2023-03-13T14:48:01Z

While I am not really happy about it, I have reverted back to the simpler approach. There is one test failure:

===============================================================================
Failure: test_remove_plugins_with_install_dir(TestGemUninstaller):
  plugin unintentionally removed.
  <false> is not true.
/home/runner/work/rubygems/rubygems/test/rubygems/test_gem_uninstaller.rb:205:in `test_remove_plugins_with_install_dir'
     202:     Dir.mkdir "#{@gemhome}2"
     203:     Gem::Uninstaller.new(nil, :install_dir => "#{@gemhome}2").remove_plugins @spec
     204: 
  => 205:     assert File.exist?(plugin_path), "plugin unintentionally removed"
     206:   end
     207: 
     208:   def test_regenerate_plugins_for
===============================================================================

And I think that the test case does not make too much sense in the context of the current code. I don't even think that the current implementation should be problematic in the context of original issue it covers. @deivid-rodriguez could you PTAL?

The problem is that as far as I understand the logic, the uninstaller always removes content from the location the gemspec was loaded from. The --install-dir, --user-install and @gem_home are used for some checks, which are mostly obstructing the uninstallation instead of helping. The --install-dir option could be useful only in the (rare) case there are multiple gem paths with the same version of gem. Is there any need for these options?

deivid-rodriguez · 2023-03-13T18:45:26Z

@voxik Shame on me that I didn't include more details of my real usage at the time when I wrote that test 😞.

Anyways, I think we can introduce your patch without breaking my test:

diff --git a/lib/rubygems/uninstaller.rb b/lib/rubygems/uninstaller.rb
index 5883ed1c41..a0e2af65bc 100644
--- a/lib/rubygems/uninstaller.rb
+++ b/lib/rubygems/uninstaller.rb
@@ -48,8 +48,8 @@ def initialize(gem, options = {})
     # TODO document the valid options
     @gem                = gem
     @version            = options[:version] || Gem::Requirement.default
-    @gem_home           = File.realpath(options[:install_dir] || Gem.dir)
-    @plugins_dir        = Gem.plugindir(@gem_home)
+    @install_dir        = File.realpath(options[:install_dir]) if options[:install_dir]
+    @gem_home           = @install_dir || File.realpath(Gem.dir)
     @force_executables  = options[:executables]
     @force_all          = options[:all]
     @force_ignore       = options[:ignore]
@@ -69,7 +69,7 @@ def initialize(gem, options = {})
 
     # only add user directory if install_dir is not set
     @user_install = false
-    @user_install = options[:user_install] unless options[:install_dir]
+    @user_install = options[:user_install] unless @install_dir
 
     # Optimization: populated during #uninstall
     @default_specs_matching_uninstall_params = []
@@ -285,7 +285,7 @@ def remove(spec)
   def remove_plugins(spec) # :nodoc:
     return if spec.plugins.empty?
 
-    remove_plugins_for(spec, @plugins_dir)
+    remove_plugins_for(spec, plugin_dir_for(spec))
   end
 
   ##
@@ -295,7 +295,7 @@ def regenerate_plugins
     latest = Gem::Specification.latest_spec_for(@spec.name)
     return if latest.nil?
 
-    regenerate_plugins_for(latest, @plugins_dir)
+    regenerate_plugins_for(latest, plugin_dir_for(@spec))
   end
 
   ##
@@ -407,4 +407,8 @@ def warn_cannot_uninstall_default_gems(specs)
       say "Gem #{spec.full_name} cannot be uninstalled because it is a default gem"
     end
   end
+
+  def plugin_dir_for(spec)
+    Gem.plugindir(@install_dir || spec.base_dir)
+  end
 end

Can you write a test for the issue this is fixing, by the way? Ideally you would also provide a reproducible example of the realworld issue and how this patch fixes the problem (what I didn't do when I introduced that test 🙏).

I can't reproduce your original issue:

$ gem install rubygems-server
Fetching rubygems-server-0.3.0.gem
Successfully installed rubygems-server-0.3.0
1 gem installed

$ gem uninstall rubygems-server
Successfully uninstalled rubygems-server-0.3.0

$ gem uninstall rubygems-server
Gem 'rubygems-server' is not installed

So my understanding is that it should be related to Fedora customizations in the operating_system.rb file.

Thanks for your work!

voxik · 2023-03-13T21:13:15Z

I'll take look tomorrow but

I can't reproduce your original issue:

I think the right command to reproduce is:

$ gem install rubygems-server --user-install

voxik · 2023-03-14T09:30:15Z

I think the right command to reproduce is:
$ gem install rubygems-server --user-install

This is the full reproducer with plain upstream Ruby:

$ gem install rubygems-server --user-install
Fetching rubygems-server-0.3.0.gem
Fetching webrick-1.8.1.gem
WARNING:  You don't have /builddir/.local/share/gem/ruby/3.2.0/bin in your PATH,
	  gem executables will not run.
Successfully installed webrick-1.8.1
Successfully installed rubygems-server-0.3.0
Parsing documentation for webrick-1.8.1
Installing ri documentation for webrick-1.8.1
Parsing documentation for rubygems-server-0.3.0
Installing ri documentation for rubygems-server-0.3.0
Done installing documentation for webrick, rubygems-server after 0 seconds
2 gems installed

A new release of RubyGems is available: 3.4.6 → 3.4.8!
Run `gem update --system 3.4.8` to update your installation.

$ gem uninstall rubygems-server 
Successfully uninstalled rubygems-server-0.3.0

$ gem install rubygems-server --user-install
Error loading RubyGems plugin "/builddir/.local/share/gem/ruby/3.2.0/plugins/rubygems-server_plugin.rb": cannot load such file -- /builddir/.local/share/gem/ruby/3.2.0/gems/rubygems-server-0.3.0/lib/rubygems_plugin.rb (LoadError)
Fetching rubygems-server-0.3.0.gem
WARNING:  You don't have /builddir/.local/share/gem/ruby/3.2.0/bin in your PATH,
	  gem executables will not run.
Successfully installed rubygems-server-0.3.0
Parsing documentation for rubygems-server-0.3.0
Installing ri documentation for rubygems-server-0.3.0
Done installing documentation for rubygems-server after 0 seconds
1 gem installed

$ gem env
RubyGems Environment:
  - RUBYGEMS VERSION: 3.4.6
  - RUBY VERSION: 3.2.1 (2023-02-08 patchlevel 31) [x86_64-linux]
  - INSTALLATION DIRECTORY: /usr/local/lib/ruby/gems/3.2.0
  - USER INSTALLATION DIRECTORY: /builddir/.local/share/gem/ruby/3.2.0
  - RUBY EXECUTABLE: /usr/local/bin/ruby
  - GIT EXECUTABLE: /usr/bin/git
  - EXECUTABLE DIRECTORY: /usr/local/bin
  - SPEC CACHE DIRECTORY: /builddir/.local/share/gem/specs
  - SYSTEM CONFIGURATION DIRECTORY: /usr/local/etc
  - RUBYGEMS PLATFORMS:
     - ruby
     - x86_64-linux
  - GEM PATHS:
     - /usr/local/lib/ruby/gems/3.2.0
     - /builddir/.local/share/gem/ruby/3.2.0
  - GEM CONFIGURATION:
     - :update_sources => true
     - :verbose => true
     - :backtrace => true
     - :bulk_threshold => 1000
  - REMOTE SOURCES:
     - https://rubygems.org/
  - SHELL PATH:
     - /usr/local/bin
     - /usr/bin
     - /bin
     - /usr/sbin
     - /sbin
     - /usr/local/sbin

voxik · 2023-03-14T10:51:21Z

TL;DR: @deivid-rodriguez the point I am trying to make is that the @gem_home is actually not used for any uninstallation action with the only exception being plugins.

The general problem I am facing here is that we have arrived at the current implementation by evolution, not by design.

My PR at this stage tries to return back to what could have been the original design. Previously, I was experimenting with bigger changes, but that would be much larger effort (which I cannot undertake due to other workload).

I need to split my comment into several chapters, because it grown long (sorry).

How `gem_home` is used

Speaking about @gem_home, because this is the only variable, which is influenced by the --install-dir option. These are the places where gem_home is used:

$ grep -n gem_home lib/rubygems/uninstaller.rb 
36:  attr_reader :gem_home
51:    @gem_home           = File.realpath(options[:install_dir] || Gem.dir)
52:    @plugins_dir        = Gem.plugindir(@gem_home)
108:      @gem_home == spec.base_dir ||
242:    unless path_ok?(@gem_home, spec) ||
245:            "Gem '#{spec.full_name}' is not installed in directory #{@gem_home}"

The only place where it really influences what is removed is here for the plugins:

rubygems/lib/rubygems/uninstaller.rb

Line 52 in 696264f

@plugins_dir = Gem.plugindir(@gem_home)

All the other real removal action is driven via Gem::Specification method calls, which are derived from #base_dir.

From this POV, your original implementation is non-systematic and what I am actually proposing is to address this exception. That is why I am questioning the test_remove_plugins_with_install_dir(TestGemUninstaller), because it only cements in the exception.

What are the expectations recorded in test suite?

If I check the current --install-dir test coverage, it is basically non-existing. It does not really focus on some real utility of --install-dir, it just focus on making sure that the install_dir == gem_home. Let me elaborate in detail.

This test just some implementation detail and would be probably better to drop it immediately:

rubygems/test/rubygems/test_gem_uninstaller.rb

Lines 25 to 30 in 696264f

    
           def test_initialize_expand_path 
        
             FileUtils.mkdir_p "foo/bar" 
        
             uninstaller = Gem::Uninstaller.new nil, :install_dir => "foo//bar" 
        
             assert_match %r{foo/bar$}, uninstaller.instance_variable_get(:@gem_home) 
        
           end

This is the install_dir == gem_home case:

rubygems/test/rubygems/test_gem_uninstaller.rb

Lines 137 to 153 in 696264f

    
           def test_remove_not_in_home 
        
             Dir.mkdir "#{@gemhome}2" 
        
             uninstaller = Gem::Uninstaller.new nil, :install_dir => "#{@gemhome}2" 
        
             e = assert_raise Gem::GemNotInHomeException do 
        
               use_ui ui do 
        
                 uninstaller.remove @spec 
        
               end 
        
             end 
        
             expected = 
        
               "Gem '#{@spec.full_name}' is not installed in directory #{@gemhome}2" 
        
             assert_equal expected, e.message 
        
             assert_path_exist @spec.gem_dir 
        
           end

The following checks special case with symlinks, but after all, it works with the original location, so does not test any real --install-dir use case, where the gem would be installed somewhere outside of some standard and expected location:

rubygems/test/rubygems/test_gem_uninstaller.rb

Lines 155 to 171 in 696264f

    
           def test_remove_symlinked_gem_home 
        
             pend "Symlinks not supported or not enabled" unless symlink_supported? 
        
             Dir.mktmpdir("gem_home") do |dir| 
        
               symlinked_gem_home = "#{dir}/#{File.basename(@gemhome)}" 
        
               FileUtils.ln_s(@gemhome, dir) 
        
               uninstaller = Gem::Uninstaller.new nil, :install_dir => symlinked_gem_home 
        
               use_ui ui do 
        
                 uninstaller.remove @spec 
        
               end 
        
               assert_path_not_exist @spec.gem_dir 
        
             end 
        
           end

and then there is the origin test_remove_plugins_with_install_dir(TestGemUninstaller). Nothing more.

The situation with --user-install is not any better ...

History

And here is the history lesson.

The --install-dir option was introduced here ~16 year ago. But at that era, the Gem::Specification was just pretty basic. All the paths were constructed in the uninstaller and the option could influence spec_dir:

rubygems/lib/rubygems/uninstaller.rb

Line 59 in 62864c3

spec_dir = File.join @gem_home, 'specifications'

I think that the last time the --install-dir was useful was prior this commit. This reduced the --install-dir to the "safeguards".

At this stage ~12 years ago, it can already be seen quite clearly that the --install-dir is used just to check the install_dir == gem_home condition.

deivid-rodriguez · 2024-05-03T18:52:38Z

Sorry for getting back so late @voxik.

I'm sure we're missing some test coverage, but from my realworld testing, the realworld --install-dir option works fine for both gem install and gem uninstall, it's properly documented, and seems useful to me. I don't think it should be removed if that's what you are suggesting.

Also, after looking again into my test, I think it makes sense to make sure that when --install-dir is given, plugins are uninstalled from that location and not from the default gem paths.

I was able to reproduce your realworld issue though and confirmed that your patch, together with my amend to not break the plugins spec fix it. So I added my suggestion to this PR and also added a test to cover this problem.

deivid-rodriguez · 2024-05-03T20:45:52Z

Also, after looking again into my test, I think it makes sense to make sure that when --install-dir is given, plugins are uninstalled from that location and not from the default gem paths.

Actually nevermind here. I had a second look and you're rigth. Your original patch is correct and it's the test that needs updating to better simulate gem uninstall --install-dir. I tried correcting it but I found another issue with it.

I'll fix everything on Monday and restore your original patch since it's indeed correct 👍.

voxik · 2024-05-03T22:05:44Z

I don't think it should be removed if that's what you are suggesting.

Heh, that would sound like me 😉 I'd love to remove exceptions and protect core principles

Thx for looking into this 👍

Instead of unit testing the `remove_plugins` method, test the whole removal process.

The plugin loader from `@gem_home` was removed during uninstallation. However, this could leave behind the plugins for `--user-install` installed gems. Use `Gem::Specifictaions#base_dir` instead. This ensures that the plugin loader for associated .gemspec is uninstalled.

deivid-rodriguez · 2024-05-09T17:29:34Z

I improved the existing test to better represent realworld, and added a new test covering this fix. It should be ready now.

Fix plugins uninstallation for user installed gems (cherry picked from commit e933219)

voxik force-pushed the spec-plugins branch 2 times, most recently from d33c536 to 48ef371 Compare March 9, 2023 11:24

voxik commented Mar 9, 2023

View reviewed changes

voxik force-pushed the spec-plugins branch 4 times, most recently from cf9ee26 to bd6bb2b Compare March 9, 2023 17:06

voxik force-pushed the spec-plugins branch 2 times, most recently from adbcab7 to eada332 Compare March 13, 2023 13:37

voxik changed the title ~~[WIP] Use Specification for managing plugins~~ [WIP] Fix plugins uninstallation Mar 14, 2023

deivid-rodriguez force-pushed the spec-plugins branch from eada332 to 991a91a Compare May 3, 2024 18:32

deivid-rodriguez changed the title ~~[WIP] Fix plugins uninstallation~~ Fix plugins uninstallation for user installed gems May 3, 2024

deivid-rodriguez added the rubygems: bug fix label May 3, 2024

deivid-rodriguez and others added 2 commits May 9, 2024 18:33

Improve plugin removal test

bfdc60a

Instead of unit testing the `remove_plugins` method, test the whole removal process.

deivid-rodriguez force-pushed the spec-plugins branch from 991a91a to 6047f78 Compare May 9, 2024 16:39

deivid-rodriguez merged commit e933219 into rubygems:master May 13, 2024

deivid-rodriguez added a commit that referenced this pull request May 16, 2024

Merge pull request #6456 from voxik/spec-plugins

fc348a8

Fix plugins uninstallation for user installed gems (cherry picked from commit e933219)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix plugins uninstallation for user installed gems #6456

Fix plugins uninstallation for user installed gems #6456

Uh oh!

voxik commented Mar 9, 2023 •

edited by deivid-rodriguez

Loading

Uh oh!

voxik commented Mar 9, 2023

Uh oh!

voxik Mar 9, 2023

Uh oh!

voxik commented Mar 9, 2023

Uh oh!

voxik commented Mar 13, 2023

Uh oh!

deivid-rodriguez commented Mar 13, 2023

Uh oh!

voxik commented Mar 13, 2023

Uh oh!

voxik commented Mar 14, 2023 •

edited

Loading

Uh oh!

voxik commented Mar 14, 2023

Uh oh!

deivid-rodriguez commented May 3, 2024

Uh oh!

deivid-rodriguez commented May 3, 2024

Uh oh!

voxik commented May 3, 2024

Uh oh!

deivid-rodriguez commented May 9, 2024

Uh oh!

Uh oh!

Uh oh!

Fix plugins uninstallation for user installed gems #6456

Fix plugins uninstallation for user installed gems #6456

Uh oh!

Conversation

voxik commented Mar 9, 2023 • edited by deivid-rodriguez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Uh oh!

voxik commented Mar 9, 2023

Uh oh!

voxik Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

voxik commented Mar 9, 2023

Uh oh!

voxik commented Mar 13, 2023

Uh oh!

deivid-rodriguez commented Mar 13, 2023

Uh oh!

voxik commented Mar 13, 2023

Uh oh!

voxik commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voxik commented Mar 14, 2023

How gem_home is used

What are the expectations recorded in test suite?

History

Uh oh!

deivid-rodriguez commented May 3, 2024

Uh oh!

deivid-rodriguez commented May 3, 2024

Uh oh!

voxik commented May 3, 2024

Uh oh!

deivid-rodriguez commented May 9, 2024

Uh oh!

Uh oh!

voxik commented Mar 9, 2023 •

edited by deivid-rodriguez

Loading

voxik commented Mar 14, 2023 •

edited

Loading

How `gem_home` is used