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

Add overwrite-exec option to Install/Update #2338

Closed
wants to merge 1 commit into from

Conversation

MSP-Greg
Copy link
Contributor

Description:

Add --overwrite-exec option to gem install/update, allowing use from scripts without UI prompt

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.

@bronzdoc
Copy link
Member

Thanks for the PR @MSP-Greg!

I Prefer --assume-yes flag too as @duckinator suggested, since is a more general flag and not specific to overwriting an executable.

Maybe we can add a better test too that checks functionality and not only to test the flag is being passed.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Jun 27, 2018

@bronzdoc @duckinator @jrochkind

So to confirm, you want the option to be named --assume-yes, and you want it to answer yes (true) to any and all UI questions?

I purposely did not want that, as gem uninstall may ask other questions unrelated to execs. It also appears to have options that allow 'no UI' or scripting functionality.

I have written two tests that check the option as pertains to exec's.

@jrochkind
Copy link

jrochkind commented Jun 28, 2018

I have no opinion what the arg should be called, and my specific issue/use case was about the "overwrite executable" thing -- I don't know enough about the other things that might be asked to know if "answer yes to everything" is a good idea or not. I'd have to leave those to people who understand the whole system better.

For gem install I couldn't find any other yes/no questions, but I may have missed them.

@MSP-Greg
Copy link
Contributor Author

@bronzdoc @duckinator

I Prefer --assume-yes flag too as @duckinator suggested, since is a more general flag and not specific to overwriting an executable.

My follow up asked two yes/no questions. I'll leave it at that...

  1. You want the option to be named --assume-yes
  2. You want it to answer yes (true) to any and all UI questions, and hence, the option moved from install_update_options.rb to command.rb.

@hsbt
Copy link
Member

hsbt commented Jun 29, 2018

I'm against to merge this strongly. Its root cause is https://bugs.ruby-lang.org/issues/5060. I try to fix this problem with Ruby 2.6.0.

@bronzdoc
Copy link
Member

@MSP-Greg that is correct, sorry for the late respons, but i didn't knew about the root cuase @hsbt mentioned.

@MSP-Greg
Copy link
Contributor Author

FULL.STOP

The original issue was a clear & simple use case that I had previously come across. In this PR, I proposed a solution matching the use case. Obviously, since it's an option, it's not changing default behavior.

Two people suggested that it should be expanded to --assume-yes, as a global option. I never stated it, but what exactly is the difference between --force and that suggested new option? If you have to look, it's probably a bad UI design.

Then, it's suggested that the issue lies in ruby. But then it's suggested that the issue may also be affected by gem update --system.

Feel free to ping me once the solution to the original issue has been determined...

Greg

@jrochkind
Copy link

I gotta admit I don't understand what's going on -- I don't really understand why it was asking the interactive y/n in the first place.

But I know it's really annoying not to be able to put "gem install bundler" in a non-interactive batch script.

If I understand right, it would not be asking the interactive y/n for gem install bundler specifically but for a different bug (which I don't understand).

But I think it would still be asking that interactive y/n in some cases? After all, it is in the code, so there must be some cases where it's triggered.

If that's true, I still think an option to avoid it, so you can safely gem install X where X might something that would trigger the y/n (I don't understand in what circumstances it's triggered, so don't know how to predict if any given gem install might) -- might still be indicated?

@MSP-Greg
Copy link
Contributor Author

@jrochkind

I gotta admit I don't understand what's going on

That makes two of us...

From the doc on Gem::Installer#check_executable_overwrite - 'If filename exists and @bin_dir is Gem.default_bindir (/usr/local) the user is consulted.', that encompassed many/most of the gem executables if they're not installed to --user-install...

@hsbt
Copy link
Member

hsbt commented Jul 1, 2018

I don't really understand why it was asking the interactive y/n in the first place.

If you type 'y', the original file under the bindir is the completely lost. You never recover it without re-install ruby package.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Jul 1, 2018

@hsbt

If you type 'y', the original file under the bindir is the conpletely lost. You never recover it without re-install ruby package.

Are you referring to bundler, or any gem with an exec? Regardless, I don't think I've seen the behavior you describe.

I essentially have a duplicate of Appveyor's environment. At present, all Ruby versions are fully updated. I tried removing, installing old, installing again, jump thru any hoop I cold think of, and I couldn't get the 'overwrite exec' prompt. Recently I worked with several repos on Appveyor scripts, and many updated rake. I needed to add the--force option because I was seeing the prompt on Appveyor.

I mentioned it here or in the issue, but in general. gem uninstall has an option:

-x, -​-[no-]executables - Uninstall applicable executables without confirmation

Would this PR be acceptable if the option name was changed to match that (and also left as an install/update option)?

@jrochkind
Copy link

Aha, I get it, thanks @hsbt . So you will generally get this prompt if you are overwriting a non-ruby file, generally. Because of the way bundler is installed as a rubygems default now (and rubygems is installed as a ruby default), in this case it applies to /usr/local/bin/bundler. But maybe you aren't supposed to be here.

I still think it might be useful to allow batch execution of gem install in general without the y/n prompt?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Jul 1, 2018

@hsbt

Just looking at rbinstall.rb again, and thinking of this also involving bundler...

I'm against to merge this strongly. Its root cause is https://bugs.ruby-lang.org/issues/5060.

Ok, but if that's fixed, will it be backported to all ruby versions currently in use on Travis & Appveyor?

For at least a while, there should to be an option that will be picked up when those versions have RubyGems updated, as both Travis & Appveyor seem to update RubyGems as needed.

matzbot pushed a commit to ruby/ruby that referenced this pull request Nov 25, 2018
…bles.

It resolved the conflict issues when invoking `gem i rdoc` and the binstub
issues with Bundler and Rails.

  [Bug #5060][ruby-core:38257][Fix GH-2023]

  * rubygems/rubygems#2338
  * heroku/heroku-buildpack-ruby#829

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@65963 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
bannable pushed a commit to bannable/ruby that referenced this pull request Dec 10, 2018
…bles.

It resolved the conflict issues when invoking `gem i rdoc` and the binstub
issues with Bundler and Rails.

  [Bug ruby#5060][ruby-core:38257][Fix rubyGH-2023]

  * rubygems/rubygems#2338
  * heroku/heroku-buildpack-ruby#829

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@65963 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
@hsbt
Copy link
Member

hsbt commented Jan 7, 2019

This issue was fixed at Ruby 2.6.

And We should choose the approaches used by yes command or echo y. The current behavior disallows to use that workflow like yes | gem update rdoc.

@hsbt hsbt closed this Jan 7, 2019
@MSP-Greg MSP-Greg deleted the overwrite-exec branch May 30, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants