Ruby: Use compilation cache for the qltest CI workflow #11344
Conversation
erik-krogh
force-pushed
the
all-the-cache
branch
7 times, most recently
from
a13557f to
b53327e
Compare
erik-krogh
changed the title
erik-krogh
force-pushed
the
all-the-cache
branch
from
b53327e to
4af8d57
Compare
.github/workflows/ruby-qltest.yml
Outdated
| - name: Run QL tests | ||
| run: | | ||
| codeql test run --threads=0 --ram 5000 --slice ${{ matrix.slice }} --search-path "${{ github.workspace }}/ruby/extractor-pack" --check-databases --check-undefined-labels --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --consistency-queries ql/consistency-queries ql/test | ||
| codeql test run --threads=0 --search-path "${{ github.workspace }}/ruby/extractor-pack" --check-databases --check-undefined-labels --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --consistency-queries ql/consistency-queries ql/test --compilation-cache "${{ steps.query-cache.outputs.cache-dir }}" |
There was a problem hiding this comment.
AFAIK, it is still a good idea to set --ram. I believe these machines have 56 GB or RAM, so setting to e.g. 52 GB should work.
| BASE_BRANCH: ${{ github.base_ref }} | ||
| run: | | ||
| MERGE_BASE=$(git cat-file commit $GITHUB_SHA | grep '^parent ' | head -1 | cut -f 2 -d " ") | ||
| echo "merge-base=$MERGE_BASE" >> $GITHUB_ENV |
There was a problem hiding this comment.
merge-base is not a valid environment variable in bash. You cant write ${merge-base} in a script. I'd recommend using merge_base or MERGE_BASE.
There was a problem hiding this comment.
Is there a reason for using GITHUB_ENV here instead of GITHUB_OUTPUT ?
There was a problem hiding this comment.
Is there a reason for using GITHUB_ENV here instead of GITHUB_OUTPUT ?
Nope, it was just the first data-sharing mechanism I found.
But now that I look at it, I still like it. I don't have to do ${{ steps.step-name.outputs.output-name }}, but can just do ${{ env.merge_base }}.
There was a problem hiding this comment.
merge-base is not a valid environment variable in bash. You cant write ${merge-base} in a script. I'd recommend using merge_base or MERGE_BASE.
It works with merge-base 🤷
I changed it anyway.
There was a problem hiding this comment.
Thanks! It's true that merge-base works, however, things get annoying if you want to print the value in a bash script for debugging reasons. It's just to prevent our future selves from scratching our heads why ${merge-base} == "base" ;-)
| uses: actions/cache@v3 | ||
| with: | ||
| path: ruby/extractor-pack | ||
| key: ${{ runner.os }}-extractor-${{ hashFiles('ruby/rust-toolchain.toml', 'ruby/**/Cargo.lock') }}-${{ hashFiles('ruby/**/*.rs') }} |
There was a problem hiding this comment.
If we cache the entire extractor pack then we should also add the other files in the pack to the key.
erik-krogh
force-pushed
the
all-the-cache
branch
from
c548abc to
57a7f89
Compare
| - name: Run QL tests | ||
| run: | | ||
| codeql test run --threads=0 --ram 5000 --slice ${{ matrix.slice }} --search-path "${{ github.workspace }}/ruby/extractor-pack" --check-databases --check-undefined-labels --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --consistency-queries ql/consistency-queries ql/test | ||
| codeql test run --threads=0 --ram 52000 --search-path "${{ github.workspace }}/ruby/extractor-pack" --check-databases --check-undefined-labels --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --consistency-queries ql/consistency-queries ql/test --compilation-cache "${{ steps.query-cache.outputs.cache-dir }}" |
There was a problem hiding this comment.
@erik-krogh Did you mean to raise the --ram argument to 52G here?
There was a problem hiding this comment.
Yes.
That change was based on this comment by Tom further up.
I didn't observe any difference from changing --ram, so it seemed fine to me.
My first solution was just to delete the --ram argument and let CodeQL figure it out for itself.
No description provided.