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

warn users if their Ruby is outdated or insecure #378

Merged
merged 1 commit into from
Apr 27, 2016
Merged

Conversation

e2
Copy link
Contributor

@e2 e2 commented Apr 26, 2016

Show warnings if Ruby version isn't up-to-date and offically
supported on ruby-lang.org.

@e2 e2 force-pushed the check_ruby_on_startup branch from 88373c8 to 7b9a685 Compare April 27, 2016 01:06
@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage decreased (-0.5%) to 97.728% when pulling 7b9a685 on check_ruby_on_startup into d546f52 on master.

@@ -33,6 +33,9 @@ Gem::Specification.new do |s|
s.add_dependency 'rb-fsevent', '>= 0.9.3'
s.add_dependency 'rb-inotify', '>= 0.9.7'

# Used to show warnings at runtime
s.add_dependency 'ruby_dep', '~> 1.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean you no longer have to install ruby_dep before you can bundle install this gem locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - that was the intention.

It's a bit tricky, but the gem is needed in 3 places:

  1. During runtime, so warnings about Ruby version can be shown (add_dependency)
  2. During parsing of the listen.gemspec (with Bundler - that's why the exception handling is in place in the gemspec).
  3. When building the gem (but that's handled by Bundler).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My answer wasn't completely correct:

Local installation of the gem takes 2 steps:

  1. Building the gem (I can't really fix this, except with the LoadError exception handling to avoid setting the ruby dependency requirement)
  2. Installing the gem locally.

The second is where the line helps, because the rake task uses gem install and gem will try to install ruby_dep for you.

But ... since you're building locally, ruby_dep should already be installed by Bundler in the previous step.

So it depends whether you're using Bundler or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@e2 What is the reason you have to check the ruby version when you do bundle install in dev mode? Would it help to add a ruby_version to the gem file? In that case you can let Bundler take care of it.

Copy link
Contributor Author

@e2 e2 Apr 27, 2016

Choose a reason for hiding this comment

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

The Ruby version checking ("warnings") is not for dev mode.

It's for runtime. E.g. if there's a ton of community pressure to make Listen compatible with Ruby < 2.2, then the Ruby requirement in the gemspec will be relaxed - but the warning about the outdated Ruby will remain.

(This is mostly so Rails users and web designers are encouraged to migrate to Ruby 2.3 - or at least avoid the buggy/insecure Ruby versions).

So ...

  1. The version checking with RubyDep is mostly so people upgrade away from Ruby vulnerabilities at least. (Like upgrading to at least Ruby 2.2.4). That's what RubyDep::Warning is for: warning end users about their Rubies.
  2. Adding ruby_version to the gem file is not really necessary. I don't know if Bundler imports this gemspec line into the Gemfile as a ruby_version value. Ideally it should, but I don't know if it does.

Since the Gemfile is only for development, it doesn't make sense to make it another place to set and track the Ruby version. (More confusion for contributors if different Ruby version constraints are in multiple places).

So this commit alone is just so end users are warned about Ruby vulnerabilities and bugs. There is a "bonus" in that it helps make sure the gem is present during development.

The other function of ruby_dep is to extract a list of supported Rubies from the .travis.yml files. It's a bit tricky to use an external gem to generate a *.gemspec entries - because without that gem already installed, every Bundler command will fail during development.

The development dependency on ruby_dep is sufficient at 1.0.0. The warnings feature is in 1.1.0. I set both to 1.1.0 for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR title was misleading. I just fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok :-) Thanks for explaining!

@e2 e2 changed the title check Ruby version on startup warn users if their Ruby is outdated or insecure Apr 27, 2016
@e2 e2 merged commit 5146c2a into master Apr 27, 2016
@e2 e2 deleted the check_ruby_on_startup branch April 27, 2016 12:51
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.

3 participants