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

Make the benchmarks group optional. #1173

Merged
merged 1 commit into from Mar 15, 2021

Conversation

junaruga
Copy link
Contributor

@junaruga junaruga commented Mar 2, 2021

I have a suggestion that is to make the benchmark group optional. Right now the installation of the benchmark group by bundle install fails on Ruby >= 2.4. I think this error might prevent a contributor from contributing. I think fixing this issue leads to the contributor's smooth onboarding.

Here is the current GitHub Actions log when trying to install the benchmarks group. You see it fails on Ruby >= 2.4.

$ bundle install --without development
...
Gem::Ext::BuildError: ERROR: Failed to build gem native extension.
...
 current directory:
/opt/hostedtoolcache/Ruby/2.4.10/x64/lib/ruby/gems/2.4.0/gems/mysql-2.9.1/ext/mysql_api
make "DESTDIR="
compiling mysql.c
mysql.c: In function ‘stmt_bind_result’:
mysql.c:1320:74: error: ‘rb_cFixnum’ undeclared (first use in this function);
did you mean ‘rb_isalnum’?
else if (argv[i] == rb_cNumeric || argv[i] == rb_cInteger || argv[i] ==
rb_cFixnum)
^~~~~~~~~~
rb_isalnum
...

After making the benchmark optional, we can install the benchmarks group like this when we need it.

$ bundle install --with benchmarks

I am also thinking if we can make the development group optional too. The pros is we can test the bundle install without command line options on the supported platforms in CI. The cons is we have to run bundle install --with development when we need the gems in the development group.

What do you think?

It's for a contributor's smooth onboarding.
This commit fixes the failed installation of the benchmarks group
on Ruby >= 2.4 by the following error.

```
$ bundle install --without development
...
Gem::Ext::BuildError: ERROR: Failed to build gem native extension.
...
 current directory:
/opt/hostedtoolcache/Ruby/2.4.10/x64/lib/ruby/gems/2.4.0/gems/mysql-2.9.1/ext/mysql_api
make "DESTDIR="
compiling mysql.c
mysql.c: In function ‘stmt_bind_result’:
mysql.c:1320:74: error: ‘rb_cFixnum’ undeclared (first use in this function);
did you mean ‘rb_isalnum’?
else if (argv[i] == rb_cNumeric || argv[i] == rb_cInteger || argv[i] ==
rb_cFixnum)
^~~~~~~~~~
rb_isalnum
...
```

You can install the benchmarks group like this.

```
$ bundle install --with benchmarks
```

See https://bundler.io/man/gemfile.5.html#BLOCK-FORM-OF-SOURCE-GIT-PATH-GROUP-and-PLATFORMS .
@junaruga
Copy link
Contributor Author

junaruga commented Mar 3, 2021

This PR is also related to #931 . I think making the benchmarks group and the mysql gem optional is more acceptable than removing it, as it still works on Ruby < 2.4.

@junaruga
Copy link
Contributor Author

@sodabrew ping.

@sodabrew sodabrew merged commit 692adc6 into brianmario:master Mar 15, 2021
@junaruga junaruga deleted the wip/bundle-install-benchmarks branch March 15, 2021 21:08
@junaruga
Copy link
Contributor Author

Thanks!

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.

None yet

2 participants