Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse built clang-tidy plugin across the shards #79100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

moxian
Copy link
Contributor

@moxian moxian commented Jan 12, 2025

Summary

Infrastructure "Reuse built clang-tidy plugin across github action shards"

Purpose of change

This is a followup to #78801 (#78801 (comment))
It is a bit silly to re-build and re-test our clang tidy plugin thrice for each workflow run when we can do it only once instead. This PR does so.

Describe the solution

At a high level: split our matrixed clang-tidy job in two

  • the first one (non-matrixed) builds the plugin, tests it, and uploads the artifact
  • the second one (matrixed) downloads the plugin and runs it on the codebase (once per matrix shard, so thrice total)

This resulted in a split of ./build-scripts/clang-tidy.sh into two scripts (one for build, and one for run) because keeping it all in one file was getting cumbersome.
This also led to quite a bit of pruning of what seems to be legacy code in the script and the workflow, which was used for active development of the plugin back in the day. But in the current state (even without this PR) I find it hard to believe that anyone actually runs ./build-scripts/clang-tidy.sh for local development given the amount of effort needed to do so (and the sparse anecdotal experience of people in discord).

As a drive-by change, I bumped the github runner to ubuntu-24.04 which allowed me to stop adding custom llvm apt repo for clang-17, and instead reuse the stock ubuntu one (since ubuntu-24.04 has clang-17 in stock)

Describe alternatives you've considered

  • Genuinely not doing this. I don't think this PR makes things worse, but it might be hard to review, and the savings are on the order of 10 cpu-minutes per run, which is not that much? Maybe? But I haven't actually looked into github action economy, and perhaps those 10-minutes-per-run add up..

Testing

Seems to work in my local fork moxian#4
Let's see if it also works in this upstream CI

Additional context

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. Code: Build Issues regarding different builds and build environments [Markdown] Markdown issues and PRs Code: Tooling Tooling that is not part of the main game but is part of the repo. Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Build Issues regarding different builds and build environments Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tooling Tooling that is not part of the main game but is part of the repo. <Documentation> Design documents, internal info, guides and help. json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant