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

Migrate TravisCI to GitHub Actions & Update Tested Ruby Versions #195

Merged
merged 2 commits into from
May 9, 2022

Conversation

mkasberg
Copy link
Contributor

@mkasberg mkasberg commented May 7, 2022

This repo hasn't received much attention recently. As such, the TravisCI
yaml file was referencing
EOL versions of
ruby. Moreover, TravisCI itself isn't generally used for open source
anymore. There are heavy restrictions on build minutes, as noted
here in the core Jekyll
project.

This PR does the following:

  • Removes .travis.yml. We won't run jobs on TravisCI anymore.
  • Replaces it with .github/workflows/ci.yml. We'll start running CI
    on GitHub Actions.
  • Updates tested Ruby versions to 2.7, 3.0, 3.1, and JRuby 9.3.4. This
    matches the ruby versions currently supported/tested by Jekyll core.
  • Adds matrix to the gemspec. In Ruby 3.1+, matrix is no longer a
    default gem
    so we need to
    declare a dependency on it.

This makes progress toward #192. I plan to work toward supporting Ruby 3
with fast SVD support and Numo in classifier-reborn, but the first step
is getting CI working with the existing code.

@mkasberg
Copy link
Contributor Author

mkasberg commented May 7, 2022

For reference/comparison, here is the last CI run on TravisCI against Ruby versions 2.4, 2.6, and JRuby 9.2.5. https://travis-ci.org/github/jekyll/classifier-reborn/builds/727612969

@mkasberg
Copy link
Contributor Author

mkasberg commented May 7, 2022

And here's the GitHub Actions run on my own fork of this repo: https://github.com/mkasberg/classifier-reborn/actions/runs/2282758186

Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

Would you mind explaining why matrix was added to the gemspec?

@mkasberg
Copy link
Contributor Author

mkasberg commented May 7, 2022

Sorry, it got buried in my second commit message and I neglected to pull it up to the PR description. I'll add it now.

One of the changes
in Ruby 3.1 is that matrix is no longer a default gem. As such, we
need to add it as a dependency to our gemspec for compatibility with
Ruby 3.1.

@mattr-
Copy link
Member

mattr- commented May 8, 2022

No worries. I was on mobile and didn't think to check the commits.

Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

This is looking good. I think the only thing I'd like to have addressed before merging this is the addition of main to the branch triggers. Everything else is optional.

Thank you for taking the time to contribute! ❤️

Comment on lines 5 to 13
branches:
- master
- "*-stable"
pull_request:
branches:
- master
- "*-stable"
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding main to the list of branches here? While there aren't any plans yet to wholesale move repos away from using master as the default branch, adding main here will make that task easier.

ruby-version: ${{ matrix.ruby_version }}
bundler-cache: true
- name: Run Minitest based tests
run: bash script/test
Copy link
Member

Choose a reason for hiding this comment

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

If you'd like, you can remove bash here

Suggested change
run: bash script/test
run: script/test

This repo hasn't received much attention recently. As such, the TravisCI
yaml file was referencing
[EOL](https://www.ruby-lang.org/en/downloads/branches/) versions of
ruby. Moreover, TravisCI itself isn't generally used for open source
anymore. There are heavy restrictions on build minutes, as noted
[here](jekyll/jekyll#8492) in the core Jekyll
project.

This PR does the following:

 * Removes `.travis.yml`. We won't run jobs on TravisCI anymore.
 * Replaces it with `.github/workflows/ci.yml`. We'll start running CI
   on GitHub Actions.
 * Updates tested Ruby versions to 2.7, 3.0, and jruby 9.3.4. This is a
   subset of the ruby versions currently supported/tested by [Jekyll core](https://github.com/jekyll/jekyll/blob/796ae15c31147d1980662744ef0f19a15a27cdee/.github/workflows/ci.yml#L20-L28).
   I plan to add support for Ruby 3.1 in a subsequent commit, but doing so
   will require additional code changes and I wanted to start be getting
   the existing code under test in CI.

This makes progress toward jekyll#192. I plan to work toward supporting Ruby 3
with fast SVD support and Numo in classifier-reborn, but the first step
is getting CI working with the existing code.
One of the [changes](https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/)
in Ruby 3.1 is that `matrix` is no longer a default gem. As such, we
need to add it as a dependency to our gemspec for compatibility with
Ruby 3.1.
@mattr-
Copy link
Member

mattr- commented May 9, 2022

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 713f50e into jekyll:master May 9, 2022
jekyllbot added a commit that referenced this pull request May 9, 2022
@mattr-
Copy link
Member

mattr- commented May 9, 2022

Thank you! 🎉

mkasberg added a commit to mkasberg/classifier-reborn that referenced this pull request May 10, 2022
classifier-reborn is designed to work with or without
[GSL](https://www.gnu.org/software/gsl/) support.

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68

If GSL is installed, classifier-reborn will detect it and use it. If GSL
is not installed, classifier-reborn will fall back to a pure-ruby
implementation. The mechanism for doing so is in `lsi.rb`:

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17

Notably, there's a comment there about how to test with/without GSL
enabled.

> to test the native vector class, try `rake test NATIVE_VECTOR=true`

As far as I can tell, this was only ever used for local
development/testing, and was never tested in CI (though it was
previously discussed
[here](jekyll#46 (comment))).
I missed this in my last PR (jekyll#195) because I was focused on porting
existing testing functionality from TravisCI to GitHub Actions. Now that
this is working, I think it's important to expand our CI coverage to
test with and without GSL in CI. So, in this PR, I'm doing so by setting
`NATIVE_VECTOR` to true or false in our test matrix.

While working on this, I noticed some tests in the LSI spec that return
early when `$GSL` is not enabled. It would be better for those tests to
report as skipped when GSL is not enabled (and this matches the pattern
of the redis tests, that report as skipped if redis isn't available).
mkasberg added a commit to mkasberg/classifier-reborn that referenced this pull request May 10, 2022
classifier-reborn is designed to work with or without
[GSL](https://www.gnu.org/software/gsl/) support.

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68

If GSL is installed, classifier-reborn will detect it and use it. If GSL
is not installed, classifier-reborn will fall back to a pure-ruby
implementation. The mechanism for doing so is in `lsi.rb`:

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17

Notably, there's a comment there about how to test with/without GSL
enabled.

> to test the native vector class, try `rake test NATIVE_VECTOR=true`

As far as I can tell, this was only ever used for local
development/testing, and was never tested in CI (though it was
previously discussed
[here](jekyll#46 (comment))).
I missed this in my last PR (jekyll#195) because I was focused on porting
existing testing functionality from TravisCI to GitHub Actions. Now that
this is working, I think it's important to expand our CI coverage to
test with and without GSL in CI. So, in this PR, I'm doing so by setting
`NATIVE_VECTOR` to true or false in our test matrix.

While working on this, I noticed some tests in the LSI spec that return
early when `$GSL` is not enabled. It would be better for those tests to
report as skipped when GSL is not enabled (and this matches the pattern
of the redis tests, that report as skipped if redis isn't available).
mkasberg added a commit to mkasberg/classifier-reborn that referenced this pull request May 10, 2022
classifier-reborn is designed to work with or without
[GSL](https://www.gnu.org/software/gsl/) support.

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68

If GSL is installed, classifier-reborn will detect it and use it. If GSL
is not installed, classifier-reborn will fall back to a pure-ruby
implementation. The mechanism for doing so is in `lsi.rb`:

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17

Notably, there's a comment there about how to test with/without GSL
enabled.

> to test the native vector class, try `rake test NATIVE_VECTOR=true`

As far as I can tell, this was only ever used for local
development/testing, and was never tested in CI (though adding GSL
testing to CI was previously discussed
[here](jekyll#46 (comment))).
I did not include this in my last PR (jekyll#195) because I was focused on
porting existing testing functionality from TravisCI to GitHub Actions.
Now that GitHub Actions is working, I think it's important to expand our
CI coverage to test with and without GSL in CI. So, in this PR, I'm
doing so by setting `NATIVE_VECTOR` to true or false in our test matrix
and installing the required `libgsl-dev` package in the Ubuntu test
environment.

While working on this, I noticed some tests in the LSI spec that return
early when `$GSL` is not enabled. It would be better for those tests to
report as skipped when GSL is not enabled (and this matches the pattern
of the redis tests, that report as skipped if redis isn't available).
mkasberg added a commit to mkasberg/classifier-reborn that referenced this pull request May 10, 2022
classifier-reborn is designed to work with or without
[GSL](https://www.gnu.org/software/gsl/) support.

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68

If GSL is installed, classifier-reborn will detect it and use it. If GSL
is not installed, classifier-reborn will fall back to a pure-ruby
implementation. The mechanism for doing so is in `lsi.rb`:

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17

Notably, there's a comment there about how to test with/without GSL
enabled.

> to test the native vector class, try `rake test NATIVE_VECTOR=true`

As far as I can tell, this was only ever used for local
development/testing, and was never tested in CI (though it was
previously discussed
[here](jekyll#46 (comment))).
I did not include this in my last PR (jekyll#195) because I was focused on
porting existing testing functionality from TravisCI to GitHub Actions.
Now that GitHub Actions is working, I think it's important to expand our
CI coverage to test with and without GSL in CI. So, in this PR, I'm
doing so by setting `NATIVE_VECTOR` to true or false in our test matrix.
Lucky for us, GitHub already [includes libgsl as pre-installed
software](https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#installed-apt-packages),
so we don't need to do anything special there.

While working on this, I noticed some tests in the LSI spec that return
early when `$GSL` is not enabled. It would be better for those tests to
report as skipped when GSL is not enabled (and this matches the pattern
of the redis tests, that report as skipped if redis isn't available).
mkasberg added a commit to mkasberg/classifier-reborn that referenced this pull request May 10, 2022
classifier-reborn is designed to work with or without
[GSL](https://www.gnu.org/software/gsl/) support.

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68

If GSL is installed, classifier-reborn will detect it and use it. If GSL
is not installed, classifier-reborn will fall back to a pure-ruby
implementation. The mechanism for doing so is in `lsi.rb`:

https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17

Notably, there's a comment there about how to test with/without GSL
enabled.

> to test the native vector class, try `rake test NATIVE_VECTOR=true`

As far as I can tell, this was only ever used for local
development/testing, and was never tested in CI (though it was
previously discussed
[here](jekyll#46 (comment))).
I did not include this in my last PR (jekyll#195) because I was focused on
porting existing testing functionality from TravisCI to GitHub Actions.
Now that GitHub Actions is working, I think it's important to expand our
CI coverage to test with and without GSL in CI. So, in this PR, I'm
doing so by setting `NATIVE_VECTOR` to true or false in our test matrix.
Lucky for us, GitHub already [includes libgsl as pre-installed
software](https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#installed-apt-packages),
so we don't need to do anything special there.

While working on this, I noticed some tests in the LSI spec that return
early when `$GSL` is not enabled. It would be better for those tests to
report as skipped when GSL is not enabled (and this matches the pattern
of the redis tests, that report as skipped if redis isn't available).
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