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

Actually Use GSL in CI Tests #197

Merged
merged 1 commit into from
May 26, 2022
Merged

Conversation

mkasberg
Copy link
Contributor

In #196, I attempted to make the CI tests execute both with and without
GSL support because classifier-reborn is supposed to work in both cases
(though GSL increases performance). However, while doing so, I missed
the fact that script/test (which our CI job runs) uses bundle exec
to run the tests. And when the tests are run with bundle exec, Bundler
will not load gems that aren't specified in the Gemfile. So our GSL
tests weren't actually using the GSL gem.

To fix this, we add gem 'gsl' if ENV["LOAD_GSL"] to our Gemfile. (This
solution was proposed by ashmaroli.) This won't do anything in most
cases (when LOAD_GSL isn't set). But when we set it (in our CI env),
it will include GSL in our bundled gems so that we can actually test
with GSL support in CI.

See here for more info:
#196 (comment)

In jekyll#196, I attempted to make the CI tests execute both with and without
GSL support because classifier-reborn is supposed to work in both cases
(though GSL increases performance). However, while doing so, I missed
the fact that `script/test` (which our CI job runs) uses `bundle exec`
to run the tests. And when the tests are run with `bundle exec`, Bundler
will not load gems that aren't specified in the Gemfile. So our GSL
tests weren't actually using the GSL gem.

To fix this, we add `gem 'gsl' if ENV["LOAD_GSL"]` to our Gemfile. (This
solution was proposed by ashmaroli.) This won't do anything in most
cases (when `LOAD_GSL` isn't set). But when we set it (in our CI env),
it will include GSL in our bundled gems so that we can actually test
with GSL support in CI.

See here for more info:
jekyll#196 (comment)
@@ -54,9 +55,6 @@ jobs:
with:
ruby-version: ${{ matrix.ruby_version }}
bundler-cache: true
- name: Install GSL Gem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer necessary - bundle install will install this gem now.

@mkasberg
Copy link
Contributor Author

You can tell that $GSL is correctly being set by looking at the difference in skipped tests when it's true/false.

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

LGTM!

@ashmaroli
Copy link
Member

Thank you @mkasberg
@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 1174d8a into jekyll:master May 26, 2022
jekyllbot added a commit that referenced this pull request May 26, 2022
@mkasberg mkasberg deleted the gsl-ci-fix branch May 27, 2022 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants