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 'call for update' to RubyGems install command. #5922

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

simi
Copy link
Member

@simi simi commented Sep 11, 2022

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

Promote update of RubyGems, since it is not currently best practice to update local installations.

Mostly based on similar features in yarn and gh CLI. More info at https://bundler.slack.com/archives/C0HJMKWH4/p1639683555027700.

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

I have added small banner to gem install command which is printed when all following conditions are met.

  • gem install command was successful
  • you have not opted-out this feature by prevent_update_suggestion entry in gemrc file
  • you have not opted-out this feature by RUBYGEMS_PREVENT_UPDATE_SUGGESTION environment variable
  • you have interactive terminal session active
  • you're not running rubygems dev/pre-release version (to not be annoying during development of RubyGems)
  • there is no disable_system_update_message set (since gem update --system is blocked when set)
  • CI is not detected
  • gemrc file is writable (empty one will be created during check to ensure if not present yet)
  • message wasn't shown already in last 7 days (to not be annoying, we can decrease this time later once battle-tested)

I tried to avoid any links in the message to make it futrue-proof (since link can expire easily).

Plan for later is add similar to bundle install and bundle update commands.

Example

[retro@retro  rubygems (call-for-update %=)]❤ ruby -Ilib -S bin/gem install ABO
Successfully installed ABO-0.0.2
Parsing documentation for ABO-0.0.2
Done installing documentation for ABO after 0 seconds
1 gem installed

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

[retro@retro  rubygems (call-for-update %=)]❤ 

Make sure the following tasks are checked


closes #1620, #3096

@simi simi force-pushed the call-for-update branch 2 times, most recently from e5a0343 to 3476aad Compare September 11, 2022 03:56
@indirect
Copy link
Member

Very nice work, thanks for setting this up!

@simi simi marked this pull request as ready for review September 11, 2022 09:48
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Awesome improvement! And great careful user-focused approach 😍

lib/rubygems/update_suggestion.rb Show resolved Hide resolved
@deivid-rodriguez
Copy link
Member

@simi Is this still a WIP or is it ready for review?

@simi
Copy link
Member Author

simi commented Sep 29, 2022

@simi Is this still a WIP or is it ready for review?

It is awaiting XDG_STATE_HOME change, I'll do my best to finish it this week. Anyway I think new minor version bump will be needed. If I remember well, there is also other case (Windows platform deprecation) awaiting minor version bump.

@deivid-rodriguez
Copy link
Member

Thanks, that's what I thought but I wanted to make sure :)

Copy link
Member

@indirect indirect left a comment

Choose a reason for hiding this comment

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

👍🏻 🙌🏻

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Just a couple of comments. Also I wonder whether this "call for update" message would make sense in other commands other than gem install. But regardless of the answer to that, gem install seems like a good place to start!

This is awesome by the way!

test/rubygems/test_gem_update_suggestion.rb Outdated Show resolved Hide resolved
lib/rubygems/update_suggestion.rb Show resolved Hide resolved
@simi simi requested review from deivid-rodriguez and removed request for skipkayhil November 4, 2022 21:57
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This looks good to me and I think it's an impressive piece of work! 💪

How should we ship this? Standard patch release, or wait for new years?

I'd go with shipping it right now with the next patch, but happy to do otherwise! End of year is so close anyways!

@simi
Copy link
Member Author

simi commented Nov 7, 2022

Thanks for the kind words.

I was looking around and it seems similar changes were released in other projects on minor release. I would prefer the same, but I'm not sure if we can just upgrade minor version simply at any time. If that's problem, I think we can roll it out actually with next patch version.

The scope is super limited and it would be great to get some community testing before new Ruby release, since if I understand it well, that's how majority of RubyGems installations are upgraded (when upgrading Ruby version). I do aim to prepare bundler support for same feature as well before Ruby 3.2 release. 🙏

@deivid-rodriguez
Copy link
Member

Let's ship this with RubyGems 3.4 then. I expect to release than at the beginning of December, so we have about a month to test before final Ruby release. Merging this now!

@deivid-rodriguez
Copy link
Member

By the way, big thanks to @soapy1 for getting the ball rolling at #3096 ❤️.

@deivid-rodriguez deivid-rodriguez merged commit d66a446 into master Nov 7, 2022
@deivid-rodriguez deivid-rodriguez deleted the call-for-update branch November 7, 2022 09:05
@simi
Copy link
Member Author

simi commented Nov 11, 2022

🤔 @enebo @eregon is it ok to recommend update of RubyGems for JRuby and TruffleRuby? We can scope this also to CRuby only for now.

@eregon
Copy link
Contributor

eregon commented Nov 11, 2022

Thank you for asking.
I think it's not good at least for TruffleRuby because TruffleRuby no-ops on gem update --system because it generally causes more problems than it solves and it's very slow (there is --force-update to force it).
So it's going to be confusing, and I think better not have it on TruffleRuby.

oracle/truffleruby@f6d2715 is an example of a change I needed to make to RubyGems, and that of course gets lost if Rubygems gets updated.
Of course I plan to contribute this patch here, or change TruffleRuby launchers to no longer need the patch (a clear loss in functionality so probably not), but there will likely always be a gap in between.


IMHO any such message is just annoying no matter for which Ruby, but that's my personal opinion.
Kind of similar to the super annoying bug where bundle would constantly updated the BUNDLED WITH section years ago.
Also it would be terribly annoying if any stdlib/default gem/other gem you use would warn if you're not using the latest version.
But I guess I won't convince you about that anyway.

At least since it's only shown on gem install and not on bundle install it won't appear often, that's good.


BTW this message should also not be shown if where RubyGems is installed is not writable. For instances this message is not actionable for a Ruby installed by apt/dnf under /usr (isn't it?). Unless that's already handled?

@simi
Copy link
Member Author

simi commented Nov 11, 2022

@eregon I'll update to print out message only on CRuby for now.

IMHO any such message is just annoying no matter for which Ruby, but that's my personal opinion.
Kind of similar to the super annoying bug where bundle would constantly updated the BUNDLED WITH section years ago.
Also it would be terribly annoying if any stdlib/default gem/other gem you use would warn if you're not using the latest version.
But I guess I won't convince you about that anyway.

We're doing our best to keep it as friendly as possible. There is no impact on the Gemfile.lock for now. Since even you update RubyGems, thanks to auto-switch in Bundler it is going to use locked bundler for resolving.

BTW this message should also not be shown if where RubyGems is installable is not writable. For instances this message is not actionable for a Ruby installed by apt/dnf under /usr (isn't it?). Unless that's already handled?

It is recommended to prevent any gem update --system using Gem.disable_system_update_message (already implemented in Debian for example). It respects this option and doesn't show any message in this kind of installation. Same for CIs, non-tty envs, ... Feel free to check initial description of PR for all cases this is skipped. And in the end, the message pops just once per week now and could be completely disabled by gemrc entry or ENV var.

If you think 1 week is still annoying, we can double that. I would be ok with that.

We're following same pattern as similar CLI tools do already for years. Personally I had no problem with npm/terraform/gh cli printing out those messages from time to time. It is good notification of thinking about the upgrade (not only locally, but also on project side).

@eregon
Copy link
Contributor

eregon commented Nov 11, 2022

It is recommended to prevent any gem update --system using Gem.disable_system_update_message

Nice, I think I can use this in rubygems/defaults/truffleruby.rb so then there is no need to change anything in RubyGems.
I find the Debian message interesting, they seem to be of the opinion of "don't update when there is no need" which I agree with. IMHO frequently updating to latest makes the development environment less stable, which is why I don't like these messages.

If you think 1 week is still annoying, we can double that. I would be ok with that.

I think no need to change, and probably would be good to have opinions from more people. I know I'm rather biased on this matter.

@simi
Copy link
Member Author

simi commented Nov 12, 2022

Nice, I think I can use this in rubygems/defaults/truffleruby.rb so then there is no need to change anything in RubyGems.
I find the Debian message interesting, they seem to be of the opinion of "don't update when there is no need" which I agree with. IMHO frequently updating to latest makes the development environment less stable, which is why I don't like these messages.

OK, I'm going to keep it as is.

@simi
Copy link
Member Author

simi commented Nov 12, 2022

...is it ok to recommend update of RubyGems for JRuby? We can scope this also to CRuby only for now.

/cc @headius

@enebo
Copy link
Contributor

enebo commented Nov 14, 2022

@simi I somewhat agree with @eregon opinion on these messages and that I personally would only want to see a message like this if it fixed something I should-not/cannot ignore like a security update. If I am using an older version of something I typically have a reason for it.

Rubygems release stride is also pretty quick so I think people will be seeing this message once every couple of weeks (RG averages about 2 releases a month) from just new releases. If you change the re-notification period to once every two weeks the odds are higher they will only see this once per RG udpate.

If we do not want to see these by default on JRuby we can just put Gem.disable_system_update_message in our defaults file right? If so then we will figure out which way we want to go with presenting this warning after it is released.

@schneems
Copy link
Contributor

👋🏻 hi. As the main maintainer for the Ruby Buildpack for Heroku I care about RubyGems quite a bit. Our policy (https://devcenter.heroku.com/articles/ruby-support) is currently:

Heroku supports the following Ruby versions and the associated Rubygems. A supported version means that you can expect our tools and platform to work with a given version. It also means you can receive technical support. Here are our supported Ruby versions:

Basically we never re-install the rubygem. If someone wants a newer rubygems version they need to upgrade to a newer Ruby. This is great for consistency but is somewhat annoying in cases where there are large updates on RubyGems but there's no associated release of MRI. Also the version support between RubyGems and MRI somewhat differs.

With the addition of this prompt I can see a few paths forward:

  • Do nothing: Always an option
  • Disable the warning via env var: Same behavior as today
  • Upgrade all RubyGems versions to the latest: Seems bad, hard to guarantee consistency
  • Introduce a gem update --not-greedy flag or something that would only bump patch level
  • Let the customer define a rubygems version.

For this last option "Let the customer define a rubygems version." ideally it would be pinned to their local development environment in the same way that RUBY VERSION is in the Gemfile.lock and isn't yet another config that needs to be updated that is vendor specific that everyone will forget about until it breaks.

I think my two favorites would be gem update --not-greedy (naming TBD) or to introduce the specific RubyGems version into the Gemfile.lock (or maybe both).

@kaporn1990
Copy link

sudo gem install cocoapods

@simi
Copy link
Member Author

simi commented Nov 14, 2022

@schneems can you explain how this affects Ruby Buildpack for Heroku? Message is skipped on non-tty environments and by doing local upgrade there is no change to any of the files (including Gemfile.lock). Bundler currently supports auto-switch/install, and even you upgrade RubyGems or Bundler, bundle commands are going to automatically use locked Bundler. Personally I don't see any reason to not use latest RubyGems with non-EOL CRuby.

Apart of that, gem command on its own is not directly used by users for installing gems often, since most of the times bundle install takes over. That leads me to other change we can do, make update banner into gem push command instead of gem install. That's independent command and it should be best practice to use latest RubyGems to push new gem if possible.

@simi
Copy link
Member Author

simi commented Nov 14, 2022

sudo gem install cocoapods

?

@simi
Copy link
Member Author

simi commented Nov 14, 2022

@enebo By setting Gem.disable_system_update_message you'll get any gem update --system call blocked as well. When set, it doesn't make any sense to promote upgrade. I'm not sure that's what you're looking for.

@schneems
Copy link
Contributor

I missed that it's only for non-tty environments. I guess i'm using this as an opportunity to talk about what our support SHOULD be as i'm re-writing the Ruby Buildpack in Rust and nows the time to make changes.

One big change we're making is relying on the bundler version in the Gemfile.lock instead of stripping it out.

https://github.com/heroku/buildpacks-ruby/tree/schneems/fresh-start-rust#known-differences-against-herokuheroku-buildpack-ruby

Feel free to disregard the rest, as it's mostly for your information. I'm mostly latching on to this conversation to have a related but different one.

Bundler currently supports auto-switch/install, and even you upgrade RubyGems or Bundler, bundle commands are going to automatically use locked Bundler. Personally I don't see any reason to not use latest RubyGems with non-EOL CRuby.

The only guarantee of behavior is specific bundler, ruby, and rubygems version. Right now we lock down on Ruby, in my upcoming buildpack, we'll lock to the customer's bundler version, that just leaves RubyGems as the odd duck out. When Bundler 2 came out there were TONS of problems with it's RubyGems and Ruby version support. It basically caused a 3 dimensional matrix of possible problem cases and we had no capacity to mediate the problem because we have no way of decoupling Ruby version from RubyGems version (at the moment).

Personally I don't see any reason to not use latest RubyGems with non-EOL CRuby.

We don't [provide] support for EOL Ruby versions, but we don't remove them either. The oldest version of Ruby on the Heroku platform right now is 2.4.10. So whatever we put in the buildpack needs to play nice with 2.4.10 or we need to otherwise have code that says "update rubygems version".

Generally speaking there's never a case where customers won't need a specific version. RubyGems and Bundler and Ruby all have bugs in them, especially new releases. I try to be wary that (most) blanket decisions I make for my customers can be opted out of in some way. So if I move forward with auto upgrading everyone to the latest, I would still want some way to opt out of that behavior and I would want that to be supported by community tooling.

@enebo
Copy link
Contributor

enebo commented Nov 14, 2022

@simi Yeah. I misread that. I guess we will just ENV["RUBYGEMS_PREVENT_UPDATE_SUGGESTION"] if we decide not to provide this with the rubygems we ship with. Looking at the PR there are other ways too. Whatever we do we want to be able to make someone be able to opt into this if they want it (if we decide to disable it by default). Thanks for the ping on this.

@simi
Copy link
Member Author

simi commented Nov 15, 2022

@schneems

I missed that it's only for non-tty environments.

It is actually only for TTY environments if I understand it well.

I guess i'm using this as an opportunity to talk about what our support SHOULD be as i'm re-writing the Ruby Buildpack in Rust and nows the time to make changes.

Feel free to ping us on Slack to get in sync about your expectations and your plans.

One big change we're making is relying on the bundler version in the Gemfile.lock instead of stripping it out.

That's seems a good step. If I understand it well, the challenge currently is to detect proper Ruby. Once detected, you can install latest bundler (or keep the Ruby default if already supporting auto-install/switch feature) and let the bundler do its best to manage Gemfile.lock with proper version itself.

@simi
Copy link
Member Author

simi commented Nov 17, 2022

@simi Yeah. I misread that. I guess we will just ENV["RUBYGEMS_PREVENT_UPDATE_SUGGESTION"] if we decide not to provide this with the rubygems we ship with. Looking at the PR there are other ways too. Whatever we do we want to be able to make someone be able to opt into this if they want it (if we decide to disable it by default). Thanks for the ping on this.

@enebo If welcomed, I can add also special config for this so there is no need to manipulate ENV just because of this.

@enebo
Copy link
Contributor

enebo commented Nov 21, 2022

@simi That's ok. Sorry I meant to reply to this a few days ago and it got lost in the tabs

@deivid-rodriguez deivid-rodriguez mentioned this pull request Dec 9, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gem command should notify the user when it is out of date
9 participants