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

Fix minor issues with clang-tidy workflow #79234

Merged
merged 2 commits into from
Jan 19, 2025

Conversation

moxian
Copy link
Contributor

@moxian moxian commented Jan 18, 2025

Summary

None

Purpose of change

This is a followup to #79100

@akrieger brought up two issues with the change in #78801 (comment)

  • the LOCALIZE flag is no longer set by default in the script, and gets its value from env. This is actually undesirable, since LOCALIZE=0 makes clang-tidy complain about every use of _, among others.
    More generally speaking, it's probably worth it to have the stock scripts run as close as CI as possible
  • the workflow sometimes fails on json-only changes, since no plugin gets built, and thus cannot be downloaded afterwards (example)

Describe the solution

  • Set all the relevant default env vars in the script itself, not relying on the CI
    • this is a behavior change - the defaults for TILES and SOUND switch from 0 to 1
  • (drive-by change) don't rebuild compilation database if it already exists force-overwrite the compilation database if it exists (local runs were failing on me trying to symlink the already exiting one)
  • add the missing "should we even run clang-tidy here in the first place?" check to download-artifact step

Describe alternatives you've considered

Testing

CATA_CLANG_TIDY=clang-tidy-17 ./build-scripts/clang-tidy-run.sh with no extra args works for me locally

On this PR, before I un-drafted it, here's the job that properly skips attempting to download the plugin: link

Additional context

This is mostly unrelated and a problem for another time, but

skip_duplicate check actually doesn't do what we want it to do. For example it thinks that CMakeLists.txt changed in this JSON-only PR: https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12826245046?pr=79204
This is because it checks the difference between the branch base and the merge pr, which includes all the other changes between when the branch was created and now.
skip_duplicates also can fail the other direction too. For example the intent is there for draft prs to not trigger clang-tidy checks. However that means that one can create a draft PR, skip clang-tidy checks, and then un-draft it, and skip the checks again because skip_duplicates treats it as a duplicate (see this check on this PR itself https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12848556910?pr=79234)

Now, we are also not abiding to what skip_duplicates tells us to do, because (i think) it sets the should_skip to "Yes" and we later check whether should_skip == "true" and assuming we should never skip things... (see last link above again)
This is all rather broken overall...

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments Code: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 18, 2025
@moxian moxian marked this pull request as ready for review January 18, 2025 23:14
@moxian moxian marked this pull request as draft January 18, 2025 23:15
@moxian moxian marked this pull request as ready for review January 18, 2025 23:36
@moxian moxian marked this pull request as draft January 19, 2025 00:02
@moxian moxian marked this pull request as ready for review January 19, 2025 00:10
build-scripts/clang-tidy-run.sh Outdated Show resolved Hide resolved
@akrieger
Copy link
Member

Thankee~~

@akrieger akrieger merged commit b36cbc7 into CleverRaven:master Jan 19, 2025
25 checks passed
@akrieger
Copy link
Member

akrieger commented Jan 19, 2025

Bah. I forgot to check that the master-only broken pipe issue got fixed. It wasn't.

https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12849902105/job/35829044260#step:7:458

@moxian
Copy link
Contributor Author

moxian commented Jan 19, 2025

That's my bad, I misread your comment "This broke clang-tidy on master runs" as "this is broken as of the current latest commit", not "this fails when trying to process pushes to master brainch". So I never realized this was an issue. (I assumed that was just a manifestation of the LOCALIZE troubles).

Apologies once more 😔

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: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants