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

Remove path limitation and adjust skip-actions logic in clang-tidy job #78339

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

kevingranade
Copy link
Member

@kevingranade kevingranade commented Dec 4, 2024

Summary

None

Purpose of change

Tried making the "clang-tidy (src)" job mandatory (again), was reminded that if the job never runs the system counts that as a failure or at least a not-pass.
While looking at docs for skip-duplicate-actions I found that required matrix jobs require special handling https://github.com/marketplace/actions/skip-duplicate-actions#how-to-use-skip-check-with-required-matrix-jobs

Describe the solution

Don't use GHA path-based optional triggering for the job, it has to run on every PR now.
Adjust skip-duplicate-actions logic to skip just the (expensive) clang-tidy step instead of the whole job, this allows the matrix job to run but be marked as a success despite being skipped.

Describe alternatives you've considered

Do we really need clang-tidy to be mandatory? Yes we do, it breaking all the time is a pain.

Testing

This just makes things more permissive, so the failure mode is if we have types of changes that erroneously trigger a full clang-tidy check somehow.

Additional context

In-flight non-cpp changes will need to be rebased to pick this up before we mark the clang-tidy job as required again, otherwise their lack of a required check will block merging.
I'm not in a HUUUGE hurry to do so, so we can take our time a bit, but we will definitely need to do some amount of rebasing.

This needs to always run since we want to make it a mandatory check.
It is set up to exit quickly if there are no .cpp changes.
@github-actions github-actions bot added Code: Tooling Tooling that is not part of the main game but is part of the repo. astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Dec 4, 2024
… job

This allows the required clang-tidy (src) step to start running and get marked as success if it is skipped.
It allows the setup tasks to run, we're concerned about runtime not correctness so those can just run and be ignored.
@kevingranade kevingranade changed the title Remove path limitation from clang-tidy job Remove path limitation and adjust skip-actions logic in clang-tidy job Dec 4, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 4, 2024
@akrieger
Copy link
Member

akrieger commented Dec 4, 2024

Eventually we should also make it so it doesn't build clang-tidy even on cpp changes if none of the cpp changes are relevant, to save another few minutes.

@kevingranade
Copy link
Member Author

Oh is that minutes? I can patch that in then, I just didn't want to clutter the hell out of the job def.

@akrieger
Copy link
Member

akrieger commented Dec 4, 2024

This eats about 5 runner minutes per PR push, worth skipping.
@akrieger
Copy link
Member

akrieger commented Dec 4, 2024

Sorry I meant when we actually invoke clang-tidy.sh, we spend 2:30s building cata clang tidy plugins and running their tests, only to possibly just skip running tidy because eg. only third party sources or json were touched. The steps you just skipped are only some seconds.

@kevingranade
Copy link
Member Author

Oh I misread you, yea it looks like we can move to https://github.com/marketplace/actions/skip-duplicate-actions#changed_files for skipping based on analyzing the paths that actually changed, and I think that should happen later.

I made a productive change based on the misreading though, I updated it to skip the clang-tidy build step and a step that will probably fail if the build step is skipped if there are no cpp files changed since you pointed out it's not super cheap.

@kevingranade
Copy link
Member Author

Double misread you :)
Yea we can also do some surgery on the script to try and exit earlier without wasted effort, we can probably build the include files and analyze the changes before building clang-tidy itself.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 4, 2024
@Night-Pryanik Night-Pryanik reopened this Dec 9, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 9, 2024
@Maleclypse Maleclypse merged commit 928f41a into master Dec 17, 2024
31 of 45 checks passed
@kevingranade kevingranade deleted the kevingranade-mandatory-clang-tidy-prep branch December 17, 2024 17:24
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: 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.

4 participants