Unswallow uninstall error #2707

deivid-rodriguez · 2019-03-29T19:09:22Z

Description:

Sometimes gem uninstall <gem_name> fails to uninstall the gem, but does not give any errors. See #2701.

This error swallowing seems intentionally introduced specifically for the --all option in 564ffe2, but I don't think this is good. In that case, --all is leaving gems around so the user should be informed too?

Maybe gem uninstall should try all paths in GEM_PATH instead?

Not sure, but the current behavior is no good.

Tasks:

Describe the problem / feature
Write tests
Write code to solve the problem
Get code review from coworkers / friends

I will abide by the code of conduct.

deivid-rodriguez · 2019-03-29T19:09:33Z

/cc @eregon

deivid-rodriguez · 2019-03-29T19:13:28Z

If we want to get this in quickly without changing any behavior, I can also scope the swallowing to the --all option only.

deivid-rodriguez · 2019-03-29T19:22:11Z

We should fix the culprit of the error though, I'll try to figure out why this is succeeding under some version manager and failing on others.

deivid-rodriguez · 2019-03-29T19:41:19Z

Ok. So the explanation is that by default gem uninstall tries to uninstall from GEM_HOME and Gem.user_dir.

On a bare ruby installation, Gem.paths.path - [Gem.paths.home] == [Gem.user_dir] is true, so the uninstall succeeds. But on rvm, Gem.user_dir is a different directory not included in GEM_PATH, so uninstall fails because the uninstall command never looks in the proper location.

eregon · 2019-03-29T21:29:36Z

@deivid-rodriguez It's not urgent, I have a workaround, let's find a proper fix.

eregon · 2019-03-29T21:30:25Z

I think in all cases we'd want error messages to at least be printed to let the user know, so this fix looks good to me.

eregon · 2019-03-29T21:32:57Z

Ok. So the explanation is that by default gem uninstall tries to uninstall from GEM_HOME and Gem.user_dir.

On a bare ruby installation, Gem.paths.path - [Gem.paths.home] == [Gem.user_dir] is true, so the uninstall succeeds. But on rvm, Gem.user_dir is a different directory not included in GEM_PATH, so uninstall fails because the uninstall command never looks in the proper location.

That's surprising. Maybe Gem.user_dir shouldn't be considered here, because we want to install/uninstall from some directory of the GEM_PATH/Gem.path, but not outside of that, isn't it? (except maybe if specifically requested with --install-dir)
Or alternatively, if Gem.user_dir maybe it should always be part of Gem.path then?

deivid-rodriguez · 2019-03-29T22:29:21Z

Yeah, apparently this is optional

rubygems/lib/rubygems/commands/uninstall_command.rb

Lines 55 to 60 in 0fa3165

    
           add_option('--[no-]user-install', 
        
                      'Uninstall from user\'s home directory', 
        
                      'in addition to GEM_HOME.') do |value, options| 
        
             options[:user_install] = value 
        
           end

And defaults to true

rubygems/lib/rubygems/commands/uninstall_command.rb

Line 18 in 0fa3165

:version => Gem::Requirement.default, :user_install => true,

I'm not really sure how Gem.user_dir is used in other parts of rubygems. I was definitely surprised too when I found out about it being considered, yeah. But I was not surprised when gem uninstall succeeded while trying to reproduce this issue. So, I guess my naive expectation is that all paths in GEM_PATH are considered, not only GEM_HOME.

hsbt

It seems reasonable. We shouldn't ignore exception warnings.

deivid-rodriguez · 2019-04-02T08:44:13Z

Let's do this then!

@bundlerbot r=hsbt

2707: Unswallow uninstall error r=hsbt a=deivid-rodriguez # Description: Sometimes `gem uninstall <gem_name>` fails to uninstall the gem, but does not give any errors. See #2701. This error swallowing seems intentionally introduced specifically for the `--all` option in 564ffe2, but I don't think this is good. In that case, `--all` is leaving gems around so the user should be informed too? Maybe `gem uninstall` should try all paths in `GEM_PATH` instead? Not sure, but the current behavior is no good. # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

ghost · 2019-04-02T09:59:28Z

Build succeeded

continuous-integration/travis-ci/push

eregon · 2019-04-02T19:22:43Z

Thanks for the fix!

2707: Unswallow uninstall error r=hsbt a=deivid-rodriguez # Description: Sometimes `gem uninstall <gem_name>` fails to uninstall the gem, but does not give any errors. See #2701. This error swallowing seems intentionally introduced specifically for the `--all` option in 564ffe2, but I don't think this is good. In that case, `--all` is leaving gems around so the user should be informed too? Maybe `gem uninstall` should try all paths in `GEM_PATH` instead? Not sure, but the current behavior is no good. # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

aried3r · 2019-08-20T09:02:31Z

Hey, we're currently facing CI issues in rails/webpacker#2243. I know this is because we cannot (yet) support bundler 2.0 in that repo in all cases and thus is not directly a bug, but it's still backwards incompatible behavior, in case the CLI is considered public API.

I guess we're hitting this new behavior here and can just || true in this case? I don't necessarily advocate for rolling back this PR, since it actually fixes bugs, we were just caught off-guard by the change.

deivid-rodriguez · 2019-08-20T09:18:21Z

Yeah, @aried3r, I'm sorry we broke your CI...

There's always a fine line between backwards incompatibility and bug fixes, because every change in the code, even bug fixes, change the behaviour of programs. In this case, we've chosen to consider this a bug fix, and thus ship it in a patch release.

If you want your CI to be locked, and only upgrade whenever you want to / need to, you can lock the rubygems version in your CI with gem update --system 3.0.6.

Hope it helps!

aried3r · 2019-08-20T09:55:33Z

No worries, just wanted to make sure we actually know why it happened. Thanks for your quick reply and your and everybody else's work on rubygems!

2893: Improve `gem uninstall --all` r=bronzdoc a=deivid-rodriguez # Description: After we started unswallowing errors of `gem uninstall` in #2707, some people got failing CI runs because they were doing things like `gem uninstall <default_gem> --all --force`. Previously that would work just fine, because the error while trying to uninstall a default gem was being swallowed. In my opinion, if you try to do `gem uninstall bundler:1.17.2` (on a ruby where this is the default bundler version), then it is fine to fail hard, but if you do `gem uninstall bundler --all`, then the command should uninstall all bundler versions that can be uninstalled (and skip the default version), but not fail just because bundler is a default gem. It can be argued though that the command should indeed fail hard, because it didn't really succeeded on trying to do what the user asked for: the user tried to uninstall _all_versions, but that wasn't achieved because the default version was left around. In order to find a compromise between these two visions, and maximize usability, I propose to not fail hard, but log a line for each default version that couldn't be removed. Note that the current behavior is completely inconsistent (in my opinion), because it fails if the default copy is the single copy in the system, but succeeds if it's not: ``` $ gem uninstall rdoc --all --force ERROR: While executing gem ... (Gem::InstallError) gem "rdoc" cannot be uninstalled because it is a default gem $ gem install rdoc Fetching rdoc-6.1.1.gem Successfully installed rdoc-6.1.1 1 gem installed $ gem uninstall rdoc --all --force Successfully uninstalled rdoc-6.1.1 ``` After this PR, the messages are more consistent and informative, in my opinion, and the status code is always success (as it was before #2707). ``` $ ruby -Ilib bin/gem uninstall rdoc --all --force Ignored rdoc-6.1.0 because it is a default gem $ gem install rdoc Fetching rdoc-6.1.1.gem Successfully installed rdoc-6.1.1 1 gem installed $ ruby -Ilib bin/gem uninstall rdoc --all --force Ignored rdoc-6.1.0 because it is a default gem Successfully uninstalled rdoc-6.1.1 ``` Thoughts? # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Closes #2892. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

A change in rubygems [0] made our CI fail, but since it was every run, it showed that this line was indeed not needed. A fix in a future rubygems version [1] should fix this as well, but I think this is the way to go. [0] rubygems/rubygems#2707 [1] rubygems/rubygems#2893

Unswallow install error

7e27b99

bronzdoc added the status: triage label Mar 29, 2019

deivid-rodriguez changed the title ~~Unswallow install error~~ Unswallow uninstall error Mar 29, 2019

hsbt approved these changes Apr 1, 2019

View reviewed changes

ghost merged commit 7e27b99 into master Apr 2, 2019

bronzdoc removed the status: triage label Apr 2, 2019

ghost deleted the unswallow_install_error branch April 2, 2019 09:59

bronzdoc mentioned this pull request Apr 3, 2019

RubyGems is silent when trying to uninstall a gem outside of the gem home #2701

Closed

5 tasks

deivid-rodriguez mentioned this pull request Aug 1, 2019

Default bundler version for ruby 2.6.x installations #2842

Closed

hsbt added this to the 3.0.5 milestone Aug 2, 2019

deivid-rodriguez mentioned this pull request Aug 14, 2019

gem uninstall won't work on windows 10 #2866

Closed

1 task

aried3r mentioned this pull request Aug 20, 2019

Travis failing rails/webpacker#2243

Closed

deivid-rodriguez mentioned this pull request Aug 21, 2019

Improve gem uninstall --all #2893

Merged

4 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unswallow uninstall error #2707

Unswallow uninstall error #2707

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019 •

edited

eregon commented Mar 29, 2019

eregon commented Mar 29, 2019

eregon commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019 •

edited

hsbt left a comment

deivid-rodriguez commented Apr 2, 2019

ghost commented Apr 2, 2019

eregon commented Apr 2, 2019

aried3r commented Aug 20, 2019

deivid-rodriguez commented Aug 20, 2019

aried3r commented Aug 20, 2019

Unswallow uninstall error #2707

Unswallow uninstall error #2707

Conversation

deivid-rodriguez commented Mar 29, 2019

Description:

Tasks:

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019 • edited

eregon commented Mar 29, 2019

eregon commented Mar 29, 2019

eregon commented Mar 29, 2019

deivid-rodriguez commented Mar 29, 2019 • edited

hsbt left a comment

Choose a reason for hiding this comment

deivid-rodriguez commented Apr 2, 2019

ghost commented Apr 2, 2019

Build succeeded

eregon commented Apr 2, 2019

aried3r commented Aug 20, 2019

deivid-rodriguez commented Aug 20, 2019

aried3r commented Aug 20, 2019

deivid-rodriguez commented Mar 29, 2019 •

edited

deivid-rodriguez commented Mar 29, 2019 •

edited