Disallow downgrades to too old versions #3566
Conversation
deivid-rodriguez
force-pushed
the
minimum_versions
branch
from
fda1c32
to
e13ca32
Compare
deivid-rodriguez
force-pushed
the
minimum_versions
branch
3 times, most recently
from
27b8b14
to
b2a522e
Compare
Consider the version original included with each ruby as the minimum supported version.
deivid-rodriguez
force-pushed
the
minimum_versions
branch
from
b2a522e
to
e6d773b
Compare
|
I switched to a simpler approach motivated by #3004 (comment). I also saved the initial implementation to a separate branch so that we can pick it up again in the future if we decide to try it. |
| @@ -75,6 +75,13 @@ def check_latest_rubygems(version) # :nodoc: | |||
| end | |||
| end | |||
|
|
|||
| def check_oldest_rubygems(version) # :nodoc: | |||
| if oldest_supported_version > version | |||
| alert_error "rubygems #{version} is not supported. The oldest supported version is #{oldest_supported_version}" | |||
There was a problem hiding this comment.
Do we need to fail here? Isn't warning enough here?
There was a problem hiding this comment.
The related ticket proposed to prevent downgrades, warnings were not considered.
As I explained in the related ticket, when I downgraded to a very old rubygems, my ruby installation was completely messed up (rubygems is specially subject to these issues since it's required by ruby by default). A warning wouln't have saved me from anything.
The criteria comes from bundler testing matrix, where we test against rubygems versions higher than or equal to the rubygems version that comes with each ruby. I think using that criteria for defining the oldest version we allow downgrading to is reasonable.
There was a problem hiding this comment.
Also note that if you really want to downgrade to an ancient version and mess up your installation, you can still do it: downgrade to a rubygems version without this feature, and from there downgrade to wherever :)
| private | ||
|
|
||
| def oldest_supported_version | ||
| @oldest_supported_version ||= Gem::Version.new("2.5.2") |
There was a problem hiding this comment.
What's the rationale behind deciding to support >= 2.5.2 for now?
There was a problem hiding this comment.
It's the rubygems version that comes by default with the oldest ruby that we support (2.3).
There was a problem hiding this comment.
I'm just thinking about the way to not forget about this to move forward once 2.3 support would be dropped.
There was a problem hiding this comment.
maybe some kind of Ruby 2.3 related comment would be enough
There was a problem hiding this comment.
Aha. I'll add a more useful test that will break if we forget to do that 👍.
| private | ||
|
|
||
| def oldest_supported_version | ||
| @oldest_supported_version ||= Gem::Version.new("2.5.2") |
There was a problem hiding this comment.
Can you add the comment about the Ruby version? It's hard to understand the Ruby version from RubyGems version.
There was a problem hiding this comment.
Yes, I will, good point 👍.
There was a problem hiding this comment.
Maybe instead of a comment put it in a constant?
There was a problem hiding this comment.
What should I put in a constant?
There was a problem hiding this comment.
Oh, I see. You mean something like RUBYGEMS_VERSION_PROVIDED_BY_RUBY_2_3 or something like that?
There was a problem hiding this comment.
RUBYGEMS_VERSION_PROVIDED_BY_OLDEST_RUBY_SUPPORTED? 
Disallow downgrades to too old versions
deivid-rodriguez
added
rubygems: enhancement
and removed
rubygems: minor enhancement
labels
What was the end-user or developer problem that led to this PR?
The problem is that for each ruby, we only test rubygems versions higher than or equal to the rubygems version that came with that ruby. So we don't really know if older versions actually work under that ruby.
What is your fix for the problem, implemented in this PR?
My fix is to prevent downgrading past the original version shipped with each ruby.
For the implementation, I considered different alternatives like:
rubygems.rbinsideRbConfig::CONFIG["rubylibdir"]to figure out the original ruby.But finally went with the current approach of finding the version through a subprocess that uses that ruby.
Fixes #3004.
Tasks:
I will abide by the code of conduct.