Skip to content

Commit

Permalink
Drop Ruby 2.5 or older versions of Ruby
Browse files Browse the repository at this point in the history 
README already mentions that `Ransack is supported for Rails 6.1, 6.0,
5.2 on Ruby 2.6.6 and later.`
d508857
  • Loading branch information
@yahonda
yahonda committed Dec 25, 2020
1 parent dc1c79e commit 87a1697
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ransack.gemspec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Gem::Specification.new do |s|
s.homepage = "https://github.com/activerecord-hackery/ransack"
s.summary = %q{Object-based searching for Active Record and Mongoid (currently).}
s.description = %q{Ransack is the successor to the MetaSearch gem. It improves and expands upon MetaSearch's functionality, but does not have a 100%-compatible API.}
s.required_ruby_version = '>= 2.3'
s.required_ruby_version = '>= 2.6'
s.license = 'MIT'

s.add_dependency 'activerecord', '>= 5.2.4'
Expand Down

25 comments on commit 87a1697

@tom-lord
Copy link

@tom-lord tom-lord commented on 87a1697 Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This seems odd, because ransack now depends on rails >= 5.2.4 which, in turn, depends on ruby >= 2.2.2, not >= 2.6.

Was there a clear reason for this constraint? I stumbled across this change via a random question on StackOverflow; it's not an issue that affected me personally.

The previous >= 2.3 constraint had a clear justification, and was introduced when ruby 2.2 reached EOL.

I think perhaps it was a misunderstanding -- it looks like after this change, the project started testing only against the latest ruby version, rather than all supported versions (I'm not sure why!!), which means the final version of travis.yml only mentioned ruby 2.6.6, and so now the re-introduced multi-ruby-version testing only runs up to ruby 2.6.6!!

I note also that ruby 2.5 has also reached EOL, but I still see no good reason why this version constraint exists on the gem.

@tom-lord
Copy link

Choose a reason for hiding this comment

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

In addition, the CONTRIBUTING.md file still says:

Please use Ruby features that are compatible down to Ruby 2.3. Since version 2.3.1, Ransack no longer maintains Ruby 2.2 compatibility.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's odd. We do support Rails 5.2, but only on modern rubies.

I think it's a good practice to explicitly drop support for rubies that have reached their EOL, and thus are not getting security fixes.

I admit that it might've been too much to do this support drop on a tiny patch release, but that's already done. Bundler should do the right thing anyways.

The CONTRIBUTING.md file needs to be updated, yes.

@tom-lord
Copy link

@tom-lord tom-lord commented on 87a1697 Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I consider it the responsibility of individual application maintainers to upgrade their ruby versions, not for third party gem authors to act as "the EOL ruby police" and force everyone to upgrade 😉

Like I said, this isn't actually affecting me personally, I just stumbled across the restriction and found it a bit surprising (especially with no clear justification). My suggestion would be to try running CI on ruby versions 2.3+, and relax the constraint back to >= 2.3 if the test suite remains green.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

Specifying a minimum ruby has also many benefits for gem authors, making gems easier to test and maintain, and allowing them to take advantage of features in newer rubies.

@tom-lord
Copy link

Choose a reason for hiding this comment

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

I agree, and if there is any significant inconvenience to continue supporting ruby 2.3-2.5 with this gem then I'd have no quarrels with setting a version constraint as it is now.

But if this gem still works perfectly well with ruby 2.3-2.5, then I disagree that there's a good reason to set this constraint.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

I consider it the responsibility of individual application maintainers to upgrade their ruby versions, not for third party gem authors to act as "the EOL ruby police" and force everyone to upgrade 😉

We're not forcing anyone, people can stay on old rubies as long as they also stay on their current ransack version 😉

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping code compatible with old rubies is painful, I really support the approach of dropping support for old rubies early, as soon as they reach their EOL, because the support load of open source maintainers is heavy enough already. And I can tell you from experience people understand it (mostly thanks to bundler doing the right thing here).

I admit it was wrong in this particular case to make this happen on a patch level version, but other than that, that's my preferred approach.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyways, I totally respect and understand other maintainers having different opinions here. This is just the approach that works for me and helps me keeping mental sanity.

@tom-lord
Copy link

@tom-lord tom-lord commented on 87a1697 Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I totally get where you're coming from. If (!!) there is actually a shred of "pain" in supporting ruby 2.3-2.5 with this gem, then I agree with the decision to set a >= 2.6 version constraint.

However as it stands, I can't see anything in the code that actually warrants this dependency constraint. I think it was added due to a misunderstanding:

  • .travis.ci was configured to only run against the latest ruby version (at the time, that was 2.6.6).
  • A new github workflow was set up to run against multiple ruby versions, replacing travis, and naively set the oldest version to 2.6.6 due to the above.
  • The README was updated to claim only ruby 2.6.6+ is supported, which wasn't really true.
  • The ruby constraint was set to >= 2.6 due to the misleading README.

As far as I can tell, this gem will still work fine with ruby >= 2.3.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether there was a misunderstanding from either @yahonda or @seanfcarroll, but speaking personally, when I accepted the PR I didn't assume there were any problems with old rubies. I was just starting to help out with maintaining this gem, just like @yahonda, and felt it would be nice to cleanup our support range and limit it to only rubies supported upstream.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the reason was to make my future life easier, essentially :)

@tom-lord
Copy link

Choose a reason for hiding this comment

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

I don't think it's standard practice to lock gems to the latest ruby versions, unless there's a clear benefit to doing so.

I respect your decision, but like I say, I think this was primarily done due to a misleading README / misunderstanding.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, note that it's not that this gem has trouble with keeping support for ruby 2.3, it has trouble with getting maintenance at all! It heavily monkeypatches internals of activerecord making it really hard to keep up to date with new rails releases, and gets really hard to maintain. Any measures intended to make the maintainers life easier have a 👍 from me.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we've both said our piece now, and we have to agree to disagree :)

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, some final thoughts. I'm not saying this is standard practice, it's just my opinion. And it's not even something I apply always. As an example, I've been refusing to drop ruby 2.3 support in bundler for almost two years now, against the opinion of other maintainers who (correctly) think it will make our lives easier. But when I start helping out in a semi-abandoned gem, I'm always positive towards dropping support for old versions of dependencies, trying to make things easier for maintainers and hopefully preventing the gem from getting semi-abandoned again.

@scarroll32
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to cleanup our support range and limit it to only rubies supported upstream.

@deivid-rodriguez I completely agree with this, and in fact we need to look to supporting the changes in Ruby 3

Ransack is supported for Rails 6.1, 6.0, 5.2 on Ruby 2.6.6 and later.

This is the position on the README

@scarroll32
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, this gem will still work fine with ruby >= 2.3.

@tom-lord thank you for your contribution here. Ransack may work on 2.3, but it won't be supported.

What does supported mean? A bug won't be considered if it happens on an older version of Ruby only. The project is really on life-support as it is, and needs a significant effort to clear out the backlog of issues and even add some new features, like Full text search.

Does that make sense?

@scarroll32
Copy link
Member

Choose a reason for hiding this comment

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

In addition, the CONTRIBUTING.md file still says:

Please use Ruby features that are compatible down to Ruby 2.3. Since version 2.3.1, Ransack no longer maintains Ruby 2.2 compatibility.

@tom-lord A PR to correct this would be welcome. Sorry for the misleading text.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

@deivid-rodriguez I completely agree with this, and in fact we need to look to supporting the changes in Ruby 3

@yahonda already added 3.0.0 to our CI matrix and everything should be fine :)

@afdev82
Copy link

@afdev82 afdev82 commented on 87a1697 Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The problem for me is not dropping the support to Ruby 2.5, but to have it in a patch version: https://github.com/activerecord-hackery/ransack/blob/v2.4.2/CHANGELOG.md
If I gem rely on ransack and it doesn't want to drop the support to Ruby 2.5 (because Ruby 2.5 is still supported by Rails 6.1), it's not possible to get security fixes and maintain the support to Ruby 2.5.
At least I expect it to happen in a minor version.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

If you read the conversation, I already said twice that I believe it was a mistake to drop support on a patch release.

@afdev82
Copy link

Choose a reason for hiding this comment

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

@deivid-rodriguez Sorry, I read the conversation, but I missed that.
So for now, is there any solution for that? A re-tagging maybe? Reintroduce the support in the next patch version and bump to a minor version instead?
Or nothing at all? I see the problem of possible future security fixes, but if you are not willing to do anything about that, fine for me.

@deivid-rodriguez
Copy link
Contributor

Choose a reason for hiding this comment

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

Reintroduce the support in the next patch version and bump to a minor version instead?

This would be the proper solution, but I don't think any of the maintainers will be handling that. At least I won't.

@scarroll32
Copy link
Member

Choose a reason for hiding this comment

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

@afdev82 @tom-lord the basic issue here is not about support of Ruby 2.5, but rather that Ransack receives very few PRs for it's large backlog of issues and needs a lot of support.

The efforts from @deivid-rodriguez, @gregmolnar and myself mean this project is just alive. The code is in terrible shape and need a big rewrite as well as improvements in the documentation and tests. The idea of supporting older Ruby versions is unfortunately a luxury, if we had a more active community this would of course change.

Please sign in to comment.