Use bundler-cache and simplify workflow #1190
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,18 +28,6 @@ jobs: | |
| uses: ruby/setup-ruby@v1 | ||
| with: | ||
| ruby-version: ${{ matrix.ruby }} | ||
| 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 Maybe, since no PS: Looking at the matrix, maybe it's also time to reconsider testing every 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.
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 Maybe 1.1) Hypothetically, if you wanted to use rubocop's GitHub Actions Formatter this should probably only be run once.
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. 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.
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! :) |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 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. There was a problem hiding this comment.
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. There was a problem hiding this comment. FWIW TravisCI also uses |
||
|
|
||
| Lint/ConstantDefinitionInBlock: | ||
| Exclude: | ||
| - manual/text/formatted_callbacks.rb | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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. Will the action pick an appropriate cache key taking into account that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will (I know, I'm the maintainer of it). It works by generated a Gemfile.lock first, just like
bundle installwould.And then the key is based on the hash of that generated Gemfile.lock, and everything else (many things) relevant for caching.
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-latestpoints to something newer), restores keys withoutbundle cleanwhich 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.gemspecwouldn'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)