Split out clang-tidy to not run in merge #4428
Conversation
| @@ -18,6 +18,9 @@ runs: | |||
| android: true | |||
| dotnet: true | |||
| haskell: true | |||
| # Without this, we save about 22GB in seconds. It adds minutes and only | |||
There was a problem hiding this comment.
I found this comment confusing; does this setting affect the saving of 22GB? If not, could we instead just mention what this does, eg "Enabling this costs several minutes and only saves about 4GB."
There was a problem hiding this comment.
Rephrased comments.
| echo "//..." >$TARGETS_FILE | ||
|
|
||
| # Compute the set of possible rules impacted by this change using | ||
| # Bazel-based diffing. This lets PRs and the merge queue have a much more |
There was a problem hiding this comment.
Is "and the merge queue" accurate? I thought the merge queue used the "push" event, so it'll hit the previous case
There was a problem hiding this comment.
I'm pretty sure merge queue uses merge_group; the list of events is at https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows. At the top of tests.yaml, there's:
on:
push:
branches: [trunk, action-test]
pull_request:
merge_group:
Since this is github.event_name != 'push', that leaves pull_request and merge_group.
Also note that base_sha is calculated differently based on the source:
base_sha:
${{ github.event_name == 'pull_request' &&
github.event.pull_request.base.sha ||
github.event.merge_group.base_sha }}
Note, you can also compare with if: ${{ github.event_name == 'merge_group' }} in https://github.com/orgs/community/discussions/43988#discussioncomment-9667914
.github/workflows/clang_tidy.yaml
Outdated
| build --config=clang-tidy -k \ | ||
| --target_pattern_file=$TARGETS_FILE | ||
|
|
||
| # See "Disk space before build". |
There was a problem hiding this comment.
| # See "Disk space before build". | |
| # See "Disk space before build" in the test setup action. |
or something?
.github/workflows/tests.yaml
Outdated
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
|
|
||
| - name: Checkout branch |
There was a problem hiding this comment.
Do we not need to specify the SHA for a PR? Was that simply redundant?
There was a problem hiding this comment.
The checkout with a sha was just moved to test-setup (i.e., choosing to checkout again in order to reduce duplication). But actually, looking at the history and behavior, I think this is just cargo culting -- it was added by #603, which used pull_request_target, which we don't anymore. Everything seems fine without it, so removing.
For reference (with a small code edit):
- tidy on push: https://github.com/carbon-language/carbon-lang/actions/runs/11410891835/job/31754107266
- test on push: https://github.com/carbon-language/carbon-lang/actions/runs/11410891832/job/31754107221
- tidy on PR: https://github.com/carbon-language/carbon-lang/actions/runs/11410892118/job/31754108277
- test on PR: https://github.com/carbon-language/carbon-lang/actions/runs/11410892127/job/31754119765
Note there, I'm primarily verifying that test execution all looks correct, caching is working, etc.
.github/workflows/tests.yaml
Outdated
| --attempts=5 \ | ||
| build --config=clang-tidy -k \ | ||
| --target_pattern_file=$TARGETS_FILE | ||
|
|
||
| # See "Disk space before build". |
|
Since I've made a few edits here, not sure if you want to look again. |
Because clang-tidy is slow (and I'm not sure we can make it really fast), trying to run it slightly less. Also, I noticed we can shave a few minutes by disabling apt removal without losing too much free space.
Note that since this removes the old clang-tidy, I'll need to change the branch protections before merging.