CI: try only to fill the compilation cache from main in the compile-queries workflow#11162
Conversation
aibaars
left a comment
There was a problem hiding this comment.
Looks good to me. Some suggestions to simplify the merge-base and to make the workflow also work for rc and other branches.
| uses: actions/cache@v3 | ||
| with: | ||
| path: '*/ql/src/.cache' | ||
| key: codeql-compile-main-${{ github.sha }} # just fill on main |
There was a problem hiding this comment.
Perhaps:
| key: codeql-compile-main-${{ github.sha }} # just fill on main | |
| key: codeql-compile-${{ github.ref }}-${{ github.sha }} # just fill on main |
| @@ -24,21 +24,21 @@ jobs: | |||
| run: | | |||
| MERGE_BASE=$(git merge-base --fork-point origin/$BASE_BRANCH) | |||
There was a problem hiding this comment.
I think we can simplify this to simply take the SHA of the first parent of the merge commit. That may actually be better than the "fork point". It also avoids having to fetch the commit history.
There was a problem hiding this comment.
Can I get a parent commit if I don't fetch the commit history?
Do you have some code you think would work?
There was a problem hiding this comment.
We have some code in the Foundations team benchmarking infrastructure that does this by calling the GitHub API, you could do something similar here: https://github.com/github/semmle-code/blob/db3086b9699c8c8c9846cd6239b78da431f27dc3/.github/actions/benchmark-prepare/lib/index.js#L55-L70 (with apologies to any external users for the link to our internal code)
There was a problem hiding this comment.
This should work too. There must be a simpler way....
git cat-file commit HEAD | grep '^parent ' | head -1 | cut -d ' ' -f
There was a problem hiding this comment.
This should work too. There must be a simpler way....
I'm not sure there is.
Your code gets the parent of the HEAD commit, which will only work when there's exactly one commit in the PR.
I could use the rest API (I got code doing something similar in DCA already), but I want to keep it simple, so I think I'll keep the current approach.
There was a problem hiding this comment.
The HEAD commit should be a merge commit of the the base and the head branches of a pull request.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, I know. But I still don't see how that's useful for getting the merge-base.
I have something better when running on main (´github.sha`).
And when running in a PR I don't see how I can use the HEAD commit, because the parent of the HEAD commit is most of the time not the commit I'm looking for.
| codeql-compile-main-${{ env.merge-base }} | ||
| codeql-compile-main- |
There was a problem hiding this comment.
| codeql-compile-main-${{ env.merge-base }} | |
| codeql-compile-main- | |
| codeql-compile-${{ github.base_ref }}-${{ env.merge-base }} | |
| codeql-compile-${{ github.base_ref }}- | |
| codeql-compile-refs/heads/main- |
I changed all the cache keys so it won't hit the old caches.
I didn't find a way to have
actions/cacherun in a "read-only" mode, so I'm using different cache keys depending on whether the workflow runs in a PR or onmain.When running on this PR the cache is empty, so the workflow is slow.
But this PR should help get better cache hits in the future.