-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
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? |
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 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. |
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. |
@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. |
Here is a nice map of Rails tested ruby versions for reference: https://stackoverflow.com/a/51810150/1795433 |
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 |
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. |
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. |
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. |
I'd like us to support the versions of Ruby that the version of Rails the release is on supports. |
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: 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. |
Yes. I agree and since the Rails gem handles this constraint for us we don’t need to add one ourself (beside the one we need for our syntax). Bundler will handle this for us. I don’t want to update our required Ruby version every time a gem decides to not support an old version of Ruby anymore. Since this gem is setting the constraint in their gemspec we good, no?
|
Just to clarify.
I don’t talk about running specs on old Ruby versions, when I say support old Ruby versions. I just think we should not constraint our gemspec overly strict.
|
Agreed with @tvdeyen |
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 Otherwise we would have to remove all usages of WDYT? |
Be careful using Shouldn't be a an issue, but something to be aware of. |
Yeah, in Rails 4+ you pretty much always want |
Emphasis on pretty much always, but not always :p 👍 |
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? |
Rubocop has checks for that. We just need to set the
No, I don't think we need to test for EOL Ruby versions. |
But it didn't catch anything in #3210, not sure if it's a HoundCI issue? Anyway isn't odd to have the Rubocop |
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.
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. |
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 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. |
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. |
@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. |
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? |
@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. |
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. |
We'd agree, but Rubocop dropped 2.2 support. Should we lock Rubocop to its last version that supports 2.2? |
That sounds like a plan. |
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 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. |
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? |
@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 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. |
In the last core team meeting we decided to go with:
|
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 RubySolidus 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
The text was updated successfully, but these errors were encountered: