Use bundler-cache and simplify workflow #1190
Conversation
| @@ -1,6 +1,10 @@ | |||
| inherit_gem: | |||
| prawn-dev: rubocop.yml | |||
|
|
|||
| AllCops: | |||
| Exclude: | |||
| - vendor/**/* | |||
There was a problem hiding this comment.
I guess this might make sense to have in https://github.com/prawnpdf/prawn-dev/blob/main/rubocop.yml, if you agree could you commit that?
It's also possible to use
inherit_mode:
merge:
- Exclude(https://docs.rubocop.org/rubocop/0.88/configuration.html#merging-arrays-using-inherit_mode)
since vendor/**/* is excluded by default in rubocop.
There was a problem hiding this comment.
This is why I had the set up in the first place. I imagine Rubocop is not the only tool that can be tripped by vendored gems.
There was a problem hiding this comment.
RuboCop actually handles it by default, it just unfortunately doesn't default to merge Excludes yet.
It seems very sensible for any tool to ignore vendor/, what better place is there for "vendored code"?
There was a problem hiding this comment.
bundle install --deployment also uses that path, and it's frequent to see bundle config --local path vendor/bundle, so I think that's valuable to support, even without considering CI, as some devs might do that anyway (e.g., I do).
There was a problem hiding this comment.
I'm still not keen on changing the source tree. Any chance this path could be configured in the action? Would it be valuable to formally request this feature on the project page?
There was a problem hiding this comment.
Yes, please open an issue on https://github.com/ruby/setup-ruby/issues about that.
vendor/bundle is the typical and most used Bundler path, but I can see it causes issues in some cases, so something outside the working dir also has advantages.
There was a problem hiding this comment.
FWIW TravisCI also uses vendor/bundle by default, but it seemed to be somewhat configurable: https://docs.travis-ci.com/user/caching/#bundler
eregon
changed the title
| @@ -1,6 +1,10 @@ | |||
| inherit_gem: | |||
| prawn-dev: rubocop.yml | |||
|
|
|||
| AllCops: | |||
| Exclude: | |||
| - vendor/**/* | |||
There was a problem hiding this comment.
This is why I had the set up in the first place. I imagine Rubocop is not the only tool that can be tripped by vendored gems.
| gem install bundler | ||
| bundle config path ~/gems | ||
| bundle install --jobs 4 --retry 3 | ||
| bundler-cache: true |
There was a problem hiding this comment.
I admit that I didn't know about this option. However, from a cursory look it might not suite well Prawn. I don't want source tree to be modified. Bundling into the source root is explicitly not that.
For one, it confuses Rubocop. I suspect it might confuse other tools as well and I'd rather not deal with that.
Readme also claims:
it is verbose and very difficult to use a correct cache key
I agree, but I'm not convinced the action does the right thing for the gem case. For example, we don't have Gemfile.lock in the repo and our Gemfile is essentially never changes because it's just a reference to the gemspec. Will the action pick an appropriate cache key taking into account that?
There was a problem hiding this comment.
I agree, but I'm not convinced the action does the right thing for the gem case. For example, we don't have
Gemfile.lockin the repo and ourGemfileis essentially never changes because it's just a reference to the gemspec.
Yes, it will (I know, I'm the maintainer of it). It works by generated a Gemfile.lock first, just like bundle install would.
And then the key is based on the hash of that generated Gemfile.lock, and everything else (many things) relevant for caching.
Will the action pick an appropriate cache key taking into account that?
Yes, and better than that it will also automatically account for e.g. changing ABI for ruby-head (yet the ABI version doesn't change), etc.
Maybe you got confused by https://bibwild.wordpress.com/2020/11/12/deep-dive-moving-ruby-projects-from-travis-to-github-actions-for-ci/ ? That's unfortunately outdated by now, all these issues have been long fixed.
The previous key used in this workflow has several problems, ignoring the OS version (could be a problem when ubuntu-latest points to something newer), restores keys without bundle clean which could grow the cache over time, doesn't properly hash based on the Gemfile.lock, etc.
It also hashes the .gemspec, which is not enough as there could be a newer gem version matching the constraint, yet the .gemspec wouldn't change.
I wonder why sometimes people think they know better than the ruby org about Bundler caching. (not directed at you, just an observation)
| gem install bundler | ||
| bundle config path ~/gems | ||
| bundle install --jobs 4 --retry 3 | ||
| bundler-cache: true | ||
| - name: Run tests | ||
| run: bundle exec rake |
There was a problem hiding this comment.
Maybe outside the scope of this PR, but it might be worth only running rake spec and not rake rubocop as well for every Ruby version. I don't think the results should differ between Ruby versions (@eregon might know more) and it would save time in the CI runs as well as making clear why a CI run failed.
For example, the most recent CI run failed with all Ruby versions because of RuboCop. https://github.com/prawnpdf/prawn/actions/runs/474569589
Maybe, since no Gemfile.lock is used to lock to a specific rubocop version, prawn-dev could lock it to a minor version in the .gemspec file instead to prevent this from happening in the first place? https://github.com/prawnpdf/prawn-dev/blob/4b370450fb10a7b7333bea35f00bc57b757987b5/prawn-dev.gemspec#L31
PS: Looking at the matrix, maybe it's also time to reconsider testing every MAJOR.MINOR.0 version in addition to the latest patch version? I remember the reason for doing it, but I wonder if the few historic events in which patch versions introduced instead of fixed bugs are worth expanding the matrix like that.
Of course that's up to you, @pointlessone.
There was a problem hiding this comment.
I will pin Rubocop to minor releases to make master build fail a bit less frequently. This is in the works.
I thought about making rubocop a separate job. I haven't came to a conclusion yet. I'll give it another thought.
Regarding 0 patch versions I'm not convinced. I don't see major changes in development practices. 2.7 had quite a few big changes in non-0-patch releases. We didn't test against all the patch releases and we happen to not be affected by them but it doesn't convince me that this won't happen in the future.
Right now 0-patch failures do not block merges (they're not marked "Required") and are of "informational" nature. We're nowhere near our CI limits so I'd like to keep them for now.
There was a problem hiding this comment.
I thought about making rubocop a separate job. I haven't came to a conclusion yet. I'll give it another thought.
Is there any downside? As pros I see: 1) Don't run it 9 times with the same result. 2) Make it more obvious what failed (see master branch currently).
Maybe 1.1) Hypothetically, if you wanted to use rubocop's GitHub Actions Formatter this should probably only be run once.
We didn't test against all the patch releases and we happen to not be affected by them but it doesn't convince me that this won't happen in the future.
I don't have a strong opinion on this and of course it's up to you, but this argument feels a little contrived to me.
What it says is, we don't test against all patch releases and nothing bad has happened with patch releases in 2.7, but because nothing happened, we should do it (but again, not every patch release). What if 0-patch versions have a bug?
I'm only wondering what we gain by testing 0-patch versions specifically vs. the latest patch version. If indeed, so many things change between patch versions, why not (exaggerated!) 2.6.0, 2.6.1, 2.6.2, 2.6.3, 2.6.4, 2.6.5 and 2.6.6?
But admittedly, Ruby 2.6.4 did have a bug that a few people faced. It was this string corruption bug I believe and we skipped it at work.
Right now 0-patch failures do not block merges (they're not marked "Required") and are of "informational" nature. We're nowhere near our CI limits so I'd like to keep them for now.
That being said, I agree with you here and hope my words didn't sound too harsh. I'm always interested in how people approach testing! :)
|
@eregon I don't feel like setup-ruby's bundler integration fits well Prawn gems. I've opened a ticket for the changes I think might make it suitable. I'll close this PR now but please ping me when setup-ruby implements those changes. Once again, thank you for your contribution. |
If the Bundler path is configurable (ruby/setup-ruby#136), would you merge this PR? |
|
I believe that was the only objection I had, so yes. |
A subset of #1189 with only the simplifications to the workflow.