Skip to content

[WIP] ci: Re-enable clang-tidy #29257

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

phlax
Copy link
Member

@phlax phlax commented Aug 25, 2023

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax marked this pull request as draft August 25, 2023 07:54
@phlax phlax force-pushed the clang-tidy-again branch from 3b81fbc to 55e213e Compare August 27, 2023 12:37
@phlax phlax force-pushed the clang-tidy-again branch 2 times, most recently from e958624 to 69afecd Compare September 11, 2023 19:59
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 11, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #29257 was synchronize by phlax.

see: more, trace.

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still marked as draft, so I assume it's not yet ready for review. When it is ready for review, please update the PR description to explain what is changing beyond simply uncommenting what was commented out in #28569

@phlax phlax force-pushed the clang-tidy-again branch 10 times, most recently from 5b87322 to b3355f2 Compare September 17, 2023 14:49
@phlax
Copy link
Member Author

phlax commented Sep 17, 2023

fwiw its not realistic to add clang-tidy as a precheck/hook/etc as i had hoped as it actually compiles stuff to run so can be expensive if there are a lot of changes

this PR does decouple it from generating compdb tho which is good - and it makes use of bazels caching/expiry to test the correct things on change

probs i should add compdb generation as a separate check - im looking at bazelifying that also

@phlax phlax force-pushed the clang-tidy-again branch 8 times, most recently from 09988d2 to 315ac59 Compare September 20, 2023 12:10
ci/do_ci.sh Outdated
fi
exit 1
}
CLANG_TIDY_TARGETS=(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs fetch

@phlax phlax force-pushed the clang-tidy-again branch 2 times, most recently from ac9b6dc to 345d057 Compare September 21, 2023 14:55
@phlax phlax force-pushed the clang-tidy-again branch 7 times, most recently from 3de9f28 to 395d749 Compare February 12, 2024 17:24
@jmarantz
Copy link
Contributor

Hi @phlax -- I'm curious how far (roughly) this is before landing.

Thanks!

@phlax
Copy link
Member Author

phlax commented Feb 12, 2024

I'm curious how far (roughly) this is before landing.

not far off (for the fixes at least) - down to the last couple of errors/warnings

last ci run is here https://dev.azure.com/cncf/envoy/_build/results?buildId=162153&view=logs&j=b7634614-24f3-5416-e791-4f3affaaed6c&t=2977bca8-7d96-53cb-e518-b36532847421&l=159

i have a pending diff with some more fixes

@phlax phlax force-pushed the clang-tidy-again branch 2 times, most recently from 5548e49 to 1815000 Compare February 12, 2024 18:58
@phlax
Copy link
Member Author

phlax commented Feb 13, 2024

@jmarantz i have resolutions for ~all the current clang-tidy problems - im going to break out the fixes to a separate PR - they need to be checked fairly thoroughly

wrt the CI part of this PR - running this repeatedly in CI and the perf is not too bad if not totally optimal - so probably we can just add it back as a check that runs in prs/commits as usual

the one blocker is that i hacked the upstream that i forked to continue on error - not sure if i did that the best way but either way we need it to record and raise any errors at the end (of running the bazel aspect) - ill look further at this shortly

@phlax phlax force-pushed the clang-tidy-again branch 2 times, most recently from 87f748a to bcf059d Compare February 19, 2024 10:23
@phlax phlax force-pushed the clang-tidy-again branch 2 times, most recently from aa9c2c5 to 2f182fa Compare March 5, 2024 12:08
@phlax phlax force-pushed the clang-tidy-again branch from 2f182fa to 4436225 Compare March 15, 2024 20:12
@phlax phlax force-pushed the clang-tidy-again branch 4 times, most recently from 8cfc83e to a03630c Compare April 16, 2024 11:28
@phlax phlax force-pushed the clang-tidy-again branch from a03630c to e3a5f0f Compare April 16, 2024 13:06
phlax added 6 commits April 16, 2024 14:25
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
This reverts commit 17b1688.

Signed-off-by: Ryan Northey <[email protected]>
@phlax phlax force-pushed the clang-tidy-again branch from e3a5f0f to 188f5ed Compare April 16, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants