search

noahgibbs / rsb

Rails Simpler Bench - a simple Rails app, with a variety of requests and Ruby versions that it can be tested with
42 stars 6 forks source link

current_ruby runner is broken on master #4

Closed eregon closed 4 years ago

eregon commented 5 years ago

Since 73985c139e7afb0f0fb8c2a7dc650a2c389c855f which seems to break several things, the benchmarks do not run anymore.

For instance:

$ chruby 2.6.2
$ ruby runners/current_ruby.rb          
Traceback (most recent call last):
runners/current_ruby.rb:38:in `<main>': undefined local variable or method `rr_opts' for main:Object (NameError)
zsh: exit 1     ruby runners/current_ruby.rb

That variable can be renamed to opts, but then the result is unused, and e.g. puma is not installed.

How should this be fixed?

eregon commented 5 years ago

Could a single Gemfile be used for all Ruby versions?

Is the main issue that we want something like ruby RUBY_VERSION in the Gemfile to make Bundler consider gems' minimum Ruby version requirements? Is there maybe another way to achieve that rather than having so many gemfiles?

noahgibbs commented 5 years ago

Oh hey! I didn't properly update the current_ruby runner to use Gemfile generation. I've begun updating that in the branch current_ruby_gemfile_generator, though it's not quite right yet.

But you're right, the option isn't enough by itself and somehow I didn't properly update that runner when I updated the others. I use rvm_batching frequently (and since then), but I should probably retest rvm_rubies too or get rid of it.

eregon commented 5 years ago

Thanks, https://github.com/noahgibbs/rsb/commit/f4ce0fe28f124264d32fddfeeb1d9bb108eff227 looks good, although it would probably be best to move the logic to bench_lib.rb so other runners like current_ruby_cli.rb can use it as well.

I think a good way to make sure it keeps working would be to add this in .travis.yml:

  - runners/current_ruby_cli.rb --benchmark-seconds 5 rack puma
  - runners/current_ruby_cli.rb --benchmark-seconds 5 rails puma

Currently those work with webrick but not puma, because the puma gem is not added.

noahgibbs commented 4 years ago

Okay. I think the changes on the branch should fix the problem - I'm having trouble with all this right now because of an unrelated (I think) libxml/nokogiri problem. The branch only touches the broken runner, and clearly does no harm, so I'm merging it to master. With luck I'll figure out my Nokogiri difficulties soon and either all's well, or I'll fix whatever the next item is...

noahgibbs commented 4 years ago

In any case, it works with Rack. If the Nokogiri problem is hiding a Rails error, that's a new issue. Closing this one as fixed/merged.

eregon commented 4 years ago

@noahgibbs Thanks that fixes it for current_ruby.rb. I shared the logic and made it work for other runners in https://github.com/noahgibbs/rsb/pull/7