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

No minimum ruby version is specified #3302

Closed
softr8 opened this issue Aug 13, 2019 · 35 comments · Fixed by #3337
Closed

No minimum ruby version is specified #3302

softr8 opened this issue Aug 13, 2019 · 35 comments · Fixed by #3337

Comments

@softr8
Copy link
Contributor

softr8 commented Aug 13, 2019

Looking at https://extensions.solidus.io, some of them are in red in the latest Solidus Version (2.9) and master, the main reason is that this change 707e79b uses a method finite? that was introduced in Ruby 2.4, but the extensions are being tested using an older version of Ruby

Solidus Version:
2.9 master

To Reproduce
Try to run solidus 2.9 or master using an older version of ruby than 2.4 then perform any API call that renders a product object

Current behavior
It raises an error saying that undefined method `finite?' for Fixnum

Expected behavior
Not sure, maybe send a warning about minimum ruby version needed

@kennyadsl
Copy link
Member

At the moment Rails 6 requires Ruby >= 2.5 while Rails 5.2.x supports Ruby >= 2.2.1.

I'm not sure what's the best thing to do here considering Ruby 2.3 is EOL now and 2.4 will go EOL soon.

I think that to be in sync with Rails (supporting Ruby >= 2.5) could be a good solution here but I'd like to have more feedback from the community. Is this something that could break your stores for some reason?

@kennyadsl
Copy link
Member

It looks like this has already been discussed and I totally forgot #2794. 😱

Well, if we still want to keep support for Ruby 2.2, we'd need to change that commit and avoid using finite?, remove all &. usage we introduced so far and backport changes to Solidus v2.9.x.

We'll discuss this in the next Core Team meeting and I'll report back. Any feedback from the community is appreciated in the meantime.

@mdesantis
Copy link
Contributor

I think the minimum Ruby version should be at least limited by Ruby EOL. It doesn't make sense to me to support an EOL'd Ruby version.

@kennyadsl
Copy link
Member

@mdesantis I agree, e-commerce applications (and probably also other applications) should not run on Ruby versions that do not receive security patches but I don't want to exclude people blocked on some old Ruby version at the same time.

@skukx
Copy link
Contributor

skukx commented Aug 28, 2019

Here is a nice map of Rails tested ruby versions for reference: https://stackoverflow.com/a/51810150/1795433

@seandawes
Copy link

Think it would also be good to promote keeping extension development going. If you look at the commit history on most extensions which fail they have not seen activity for 6mths+. A more active extension support could also encourage a higher adoption rate

@tvdeyen
Copy link
Member

tvdeyen commented Aug 28, 2019

but I don't want to exclude people blocked on some old Ruby version at the same time.

Yes. We should not make this decision for our users. There are several reasons (other dependencies, server constraints) why people cannot immediately update the Ruby version.

The same is true for dependency versions. We should always take the lowest boundary that our code base is compatible with. If our code works with Ruby 2.0 we should not raise it’s minimum version just because we think it’s right.

We develop a library that is used inside a rails app that has many other dependencies and constraints we don’t have control over. That’s why we need to be much more loose in our constraints.

@tvdeyen
Copy link
Member

tvdeyen commented Aug 28, 2019

I think the minimum Ruby version should be at least limited by Ruby EOL. It doesn't make sense to me to support an EOL'd Ruby version.

That’s not the right thing to do from a Library standpoint. We need to set the Ruby version constraint to the minimum Ruby version our code is working with. The EOL of Ruby version has nothing to do with this decision. This is a decision store owners do.

@BenMorganIO
Copy link
Contributor

The minimum version of ruby should be what we can support. If we depend on Rails - and we do - then it's a little redundant to support 2.3 or 2.4 when the minimum from one of our dependencies is 2.5. Therefore, the minimum we can support is 2.5. When supporting Rails 6, we can raise the minimum version to 2.5.

2.3 and is EOL and 2.4 is coming to an end soon. Solidus has had a nice upgrade path for a long while, so when people update, they should be notified to upgrade their ruby version to the new minimum before coming to Rails 6.

@jarednorman
Copy link
Member

I'd like us to support the versions of Ruby that the version of Rails the release is on supports.

@skukx
Copy link
Contributor

skukx commented Aug 28, 2019

Since each solidus version is locked to a specific rails version it is probably the best practice to support the minimum version of ruby that the locked rails version supports.

For example: solidus ~> 2.8.0 is locked to rails ~> 5.0.0. So solidus v2.8 should support down to ruby ~> 2.2.0.

I don't see any reasons we should support a ruby version that the required rails does not support. However I see every reason we should support any ruby version that the required rails version supports.

@tvdeyen
Copy link
Member

tvdeyen commented Aug 28, 2019 via email

@tvdeyen
Copy link
Member

tvdeyen commented Aug 28, 2019 via email

@skukx
Copy link
Contributor

skukx commented Aug 28, 2019

Agreed with @tvdeyen

@tvdeyen
Copy link
Member

tvdeyen commented Aug 28, 2019

So, how to proceed, then?

I think if we still support Rails 5.2 we also need to support Ruby >= 2.2 and should remove the syntax that makes Ruby 2.4 our lowest supported version.

But on the other hand the safe navigation introduced in Ruby 2.3 is beneficial for us and the community. And Ruby 2.2 is EOL. So maybe our lowest supported Ruby version should be 2.3 although this violates the Rails 5.2 constraint? Are we actually using &. that much?

Otherwise we would have to remove all usages of &. and replace it with try (meh) and back port these changes to how many versions? This doesn’t seem like a good choice either.

WDYT?

@skukx
Copy link
Contributor

skukx commented Aug 28, 2019

Be careful using try as it could silence NoMethodError in some cases:
See: https://apidock.com/rails/v3.2.1/Object/try#1533-In-Rails-4-it-DOES-return-nil-even-if-the-object-you-try-from-isn-t-nil

Shouldn't be a an issue, but something to be aware of.

@jarednorman
Copy link
Member

jarednorman commented Aug 28, 2019

Yeah, in Rails 4+ you pretty much always want try!, which only swallows nil-values, not NoMethodError like try does.

@BenMorganIO
Copy link
Contributor

BenMorganIO commented Aug 28, 2019

Emphasis on pretty much always, but not always :p 👍

@kennyadsl
Copy link
Member

I'd like to also find a way to avoid the mistake we (actually I) made introducing something that is not compatible with Ruby 2.2 without any Ci tool being able to catch this problem. This process is too fragile since it relies only on PR reviews from the Core Team and community (which are always welcome!).

What about using the minimum Ruby version that we support (2.2 in this case) for running tests on the CI as well?

@tvdeyen
Copy link
Member

tvdeyen commented Aug 29, 2019

I'd like to also find a way to avoid the mistake we (actually I) made introducing something that is not compatible with Ruby 2.2 without any Ci tool being able to catch this problem. This process is too fragile since it relies only on PR reviews from the Core Team and community (which are always welcome!).

Rubocop has checks for that. We just need to set the TargetRubyVersion

What about using the minimum Ruby version that we support (2.2 in this case) for running tests on the CI as well?

No, I don't think we need to test for EOL Ruby versions.

@kennyadsl
Copy link
Member

Rubocop has checks for that. We just need to set the TargetRubyVersion

But it didn't catch anything in #3210, not sure if it's a HoundCI issue?

Anyway isn't odd to have the Rubocop TargetRubyVersion to 2.2 and perform tests against 2.5?

@mdesantis
Copy link
Contributor

Rubocop has checks for that. We just need to set the TargetRubyVersion

But Rubocop checks the syntax only, doesn't check if a spec passes or not; I think that a supported version should be added to the CI, because otherwise you can't really say whether it is supported, because features could be broken and you'll never know

@jacobherrington
Copy link
Contributor

Anyway isn't odd to have the Rubocop TargetRubyVersion to 2.2 and perform tests against 2.5?

This is a good point.

But Rubocop checks the syntax only, doesn't check if a spec passes or not; I think that a supported version should be added to the CI, because otherwise you can't really say whether it is supported, because features could be broken and you'll never know

This is a good point too.

Should we run tests against 2.2? I feel like that is the correct answer, if we really do support it.

@kennyadsl
Copy link
Member

kennyadsl commented Sep 5, 2019

Other pieces to add to the puzzle:

Rubocop support

They dropped 2.2 support. That's why I upgraded TargetRubyVersion to 2.3. That means that we should probably lock rubocop to an older version, and see how that work with HoundCI.

Ruby Usage Stats

Not sure if this can be helpful but I found these JetBrain Ruby usage statistics: https://www.jetbrains.com/lp/devecosystem-2019/ruby/ . It looks like Ruby 2.2 could be used by 3% only.

@BenMorganIO
Copy link
Contributor

BenMorganIO commented Sep 5, 2019

But why test against a version we cannot support due to a dependency? It seems silly. Rubocop should be set to at least 2.5.

@jacobherrington
Copy link
Contributor

@BenMorganIO I think we need to decide what we choose to support. Historically, we've tried to support the Ruby versions that Rails supports. I'm not sure what the right answer is now. We talked a bit about this in the core team meeting, but I do not know the correct answer personally.

@BenMorganIO
Copy link
Contributor

BenMorganIO commented Sep 6, 2019

Rails already made the decision for us. I find it a little redundant checking to see if you can support Ruby 2.4 when Solidus is built on a framework which will soon become >= 2.5. I might be missing something, but what value does Solidus get for 2.4 support on Rails 6?

@ericsaupe
Copy link
Contributor

@BenMorganIO You're right, Rails 6 only supports Ruby 2.5.0 or greater. The question is do we support Ruby 2.2 on older versions of Solidus, that have not hit their EOL, that still support Rails 5 which requires 2.2.2 or newer.

@BenMorganIO
Copy link
Contributor

The answer would be yes. People using Solidus, say, 2.0 would expect that when they do a patch or minor version update, their language version shouldn't change. Semantic version control has already established an implicit contract for this.

@kennyadsl
Copy link
Member

We'd agree, but Rubocop dropped 2.2 support. Should we lock Rubocop to its last version that supports 2.2?

@BenMorganIO
Copy link
Contributor

That sounds like a plan.

@aldesantis
Copy link
Member

aldesantis commented Sep 12, 2019

I changed my mind 4-5 times while trying to jot down my thoughts here, but I'll try anyway...

On one hand, I'd really like to retain all the benefits we get by using Ruby 2.5+. On the other, I understand that it would be weird to support Rails 5 while requiring a much higher Ruby version, so my preference would be to support Ruby 2.2+.

For the sake of simplicity, I think we should align with Rails, so:

  • In CircleCI, we should test each Rails version with the earliest Ruby version supported by Rails (2.2 for Rails 5, 2.5 for Rails 6).
  • In the core codebase, we should make everything compatible with the earliest Ruby version we support, which is currently 2.2.
  • In extensions, make the code compatible with 2.2+ and test every Solidus version against all needed Ruby versions (this would be 2.2 for < 2.9, 2.5 and 2.5 for >= 2.10).

In other words, we'd be adding one more element to the build matrix: DBMS, Solidus version and Ruby version. We should consider the toll this will take on our CI jobs.

I'm strongly against not testing extensions on Ruby 2.2, because then we'd have a product that supports Ruby 2.2, but every time you install an extension you don't know if it's going to break your code or not. If we can't find a way to test extensions thoroughly, I think we should simply disallow anything <= 2.5.

@BenMorganIO
Copy link
Contributor

This sounds very thoughtful. I like how you started with the basics and grew the complexity as you looked at how it would impact the rest of the community.

I think it's quite smart for Solidus to go the 2.5 route when doing Rails 6 support and retain 2.2 for Rails 5.

I also agree with the extensions. If they're supporting less than 2.10, then set their minimum version to 2.2.

When you talk about the core codebase, are you referring to solidus_core? Is solidus_core able to differentiate the rails version?

@aldesantis
Copy link
Member

aldesantis commented Sep 13, 2019

@BenMorganIO I'm referring to solidus_core, solidus_backend etc., so basically everything that's not an extension. We can check the Rails version at runtime and we do this in a few places to support multiple Rails versions, but we can't specify different Ruby constraints depending on the Rails version that is being used.

As for extensions, we can't simply test with 2.2 unfortunately, because if we did that, we wouldn't be able to test them with 2.10 and Rails 6. That is, if you test with SOLIDUS_BRANCH=master and Ruby 2.2, Bundler will silently install Rails 5 because the Ruby version constraint for Rails 6 is not satisfied. This actually happened to me this week when I was updating extensions for Rails 6 support and couldn't figure out why Bundler was installing Rails 5 even with Solidus 2.10.

That means we need to test all extensions both with Ruby 2.2, to ensure their code is compatible with the Ruby version, and Ruby 2.5, to ensure they are compatible with Rails 6.

@kennyadsl
Copy link
Member

In the last core team meeting we decided to go with:

  • bump minimum required ruby version of master and v2.9 to 2.4. If someone can't upgrade to latest Ruby, they cannot use latest Solidus. Still, they can use Solidus to older versions.
  • test all extensions with Ruby 2.5 for the sake of simplicity and avoid wasting core team and community members' time and CI resources creating a too complex matrix.

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 a pull request may close this issue.