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

Missing minimal Ruby version #2319

Closed
wants to merge 2 commits into from
Closed

Conversation

marcandre
Copy link

@marcandre marcandre commented Apr 13, 2020

Please use required_ruby_version...

It's too bad that 4.0 is out without this, as anyone using/testing older Rubies will have to specify manually a "<4" version in their Gemfile.

I wish Rubygems made that mandatory

Please use `required_ruby_version`...

Could not see any version in the README, so assuming 2.3+.
@pirj
Copy link
Member

pirj commented Apr 13, 2020

Good catch.

Rails, which we depend on, has required_ruby_version, specifically rails 4.2 says it supports Ruby 1.9.3+, but we don't. As to my memory, we support Ruby 2.2+.

I feel your pain here if a tool is testing Rails 4.2 on Ruby 1.9.3, and Bundler installs rspec-rails version 4, manual configuration would be required.

Not sure, however, if adding required_ruby_version in rspec-rails 4.0.1 would solve the situation - instead of the latest 4.0.1 Bundler will fall back to 4.0.0, and I have no good idea how to fix this situation. Making required_ruby_version mandatory sounds like a really good idea.

@marcandre
Copy link
Author

You don't support Ruby 2.2.

Your .travis is 2.3+ and you use (at least one) &.

@marcandre
Copy link
Author

Making required_ruby_version mandatory sounds like a really good idea.

Make sure to add your thumbs up on rubygems/rubygems#3515 then :-)

Indeed, as I point out in the issue I opened, there is no actual fix for this (all too common) situation 🤷‍♂️.

I should also create a rubocop for this

@pirj
Copy link
Member

pirj commented Apr 13, 2020

I hope they'll support the idea. At the same time, I can't install rubygems 3.0.8 while installing Ruby 2.2 from RVM to test where we've opted for 2.3+.

@JonRowe
Copy link
Member

JonRowe commented Apr 14, 2020

We support Rails 5 and Rails 6 in this version (4.0.0) but we for transitionary purposes left the version of Rails at >= 4.2 for soft support. The idea being that major versions of rspec-rails will support the current major supported versions of Rails. E.g when Rails 7 is released we aim to release rspec-rails 5.0.0, that will support Rails 6 and 7.

It seems this is a casualty of that, I'd almost rather issue a warning about this with instructions on how to fix than apply this restriction given that it won't "fix" 4.0.0 (which probably should have had this).

@JonRowe
Copy link
Member

JonRowe commented May 12, 2020

Coming back to this, the reason why I haven't just merged it, is because it doesn't fix 4.0.0 which we can't yank (I believe) as that would break peoples builds. We almost need an interim step that fixes rspec-rails on Ruby 2.2 (its probably just those funky operators), warns about the change in minimum ruby version, then add it later?

@betesh
Copy link
Contributor

betesh commented May 12, 2020

I'm not following the reasoning about not being able to yank it. Isn't there a risk of breaking builds every time you yank a gem? Isn't that inconvenience outweighed by the need to yank the gem to avoid disrupting developers in some other way?

@JonRowe
Copy link
Member

JonRowe commented May 12, 2020

I'm not following the reasoning about not being able to yank it. Isn't there a risk of breaking builds every time you yank a gem?

Yes thats the reasoning.

Isn't that inconvenience outweighed by the need to yank the gem to avoid disrupting developers in some other way?

No, yanking affects everyone and the version of the gem isn't broken, its just a case case that is technically unsupported gets asub optimal situation where they are accidentally upgraded.

Ruby 2.2 is EOL'd and quite out of date being what, 5 years old? The majority of rspec-rails 4 users are going to be on more modern rubies and completely unaffected. So in this case the benefit of the majority outweighs the inconvenience to the minority.

@marcandre
Copy link
Author

Right, yanking is not the way.

I can check out how difficult it would be to make 4.0.1 that would be Ruby 2.2 compatible...

@JonRowe
Copy link
Member

JonRowe commented May 12, 2020

I can check out how difficult it would be to make 4.0.1 that would be Ruby 2.2 compatible...

I've been trying to take a look at this today, the first hurdle... is actually installing Ruby 2.2... 😂

@marcandre
Copy link
Author

Ok, I'll let you handle it, just let me know if you'd like me to have a look at it.

@JonRowe
Copy link
Member

JonRowe commented May 15, 2020

Fun fact, Rails 5.2.4.2 is not entirely Ruby 2.2 compatible, they patched it in December but haven't released that patch, assumedly they are waiting until 5.2.5?

@JonRowe JonRowe changed the base branch from master to 4-0-maintenance May 16, 2020 14:13
@JonRowe JonRowe changed the base branch from 4-0-maintenance to master May 16, 2020 14:14
JonRowe added a commit that referenced this pull request May 16, 2020
@JonRowe JonRowe closed this May 16, 2020
@JonRowe
Copy link
Member

JonRowe commented May 16, 2020

This is merged into master and 4-0-maintenance, I will release 4.0.1 shortly which sets the minimum ruby version to 2.2 and contains the fix so that rspec-rails at least, will not error on Ruby 2.2.

Thanks for the prompt on this @marcandre 💜 🧡 💚 💛 ❤️

@marcandre
Copy link
Author

Awesome! I'm glad that the &. I had noticed turned out to be the only actual incompatibility! <3

@JonRowe
Copy link
Member

JonRowe commented May 16, 2020

I'm amused by how much time it took to track down that Rails itself had one too 😂
That and playing whack a mole with our travis setup

@marcandre
Copy link
Author

Yeah, I saw you had to fiddle with a lot of dependency loading. I'm very glad you looked into it (instead of me), thanks again.

@pirj pirj mentioned this pull request Oct 24, 2021
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 this pull request may close these issues.

4 participants