ci: run clang-tidy as a part of CI #4666

lizan · 2018-10-10T06:32:38Z

Signed-off-by: Lizan Zhou lizan@tetrate.io

Description:
Run clang-tidy as a part of CI.

Risk Level: Low
Testing: CI, manual
Docs Changes: N/A
Release Notes: N/A
Fixes #921.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

mattklein123

Very nice!

mattklein123 · 2018-10-10T17:00:40Z

.clang-tidy

@@ -0,0 +1,306 @@
+---
+# -cppcoreguidelines-pro-type-vararg: It will alert on every ASSERT.


Can you add some more comments about how this file was generated / where it comes from?

ci/do_ci.sh

mattklein123 · 2018-10-10T20:15:17Z

@lizan did you mean to push a change?

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan · 2018-10-10T20:50:37Z

.clang-tidy

@@ -0,0 +1,315 @@
+---
+# This file defines what clang-tidy checks and parameter. It is experimental, and may let 


@envoyproxy/maintainers I am open to any suggestion to this file. With this .clang-tidy it still issue too many warnings. A result from a couple days ago:
https://gist.github.com/lizan/115f05ace9ac7dbae4c042f6fbdb3596

Hmm. Don't we need to fail if clang-tidy fails? So don't we need to be warning clean? Or we only fail on errors? In general it seems like we should be warning clean also.

No I don't think we should mark clang-tidy required, at least in a meanwhile. We can make some of checks error with -warnings-as-errors later to fail on them.

Then what's the point of using a CI slot for this? No one will look at it.

Just like mac CI that is not required (on GitHub) but we still check? At this point, there are too many warnings to fix at once so I am hesitating to make them error.

cc @jsedgwick @rlazarus @PiotrSikora on #921, any suggestion to enable some checks as WarningsAsErrors?

How does Google deal with this internally? Since Google fixes bugs raised by clang-tidy presumably some internal CI is catching them? Can we just start with Google's config? cc @htuch @alyssawilk @PiotrSikora

Internally clang-tidy is largely advisory I believe, but most reviewers will take its suggestions as actionable. For Envoy, only some importers (we have a weekly rotation) actually pay attention as it's largely mechanical.

I think we have a noise problem with a purely advisory clang-tidy, if the report is as noisy as in https://gist.github.com/lizan/115f05ace9ac7dbae4c042f6fbdb3596, reviewers will never both to look at it. Inside Google, our review system highlights issues only in the delta diff.

Maybe we can turn off all warnings, make clang-tidy compulsory and only turns on the ones that actually are low on the false positive side?

Yes running clang-tidy on diff (especially for PRs) is definitely the plan, considering how clang-tidy is heavy (it takes an hour to run on whole source code). There is a tool to run clang-tidy against diff.

If we do have a better .clang-tidy to start with, I'm happy to start with that rather than this.

+1, IMO we need to make this actionable or we shouldn't bother running it in CI.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan · 2018-10-11T20:15:16Z

re: #4666 (comment) @mattklein123

OK Now I cleaned up .clang-tidy a lot, now it fails on certain checks that we've been seen in code reviews, which is actionable. (e.g. use make_unique/make_shared, side-effect ASSERT calls)

The remaining warning checks are also valid, but hard to fix in short time. I'll explore if diff check will make more sense for those

Here is the latest result: https://gist.github.com/lizan/23b289b977e4d85c140d61b187a08e03

If we agree on errors checks, we need a single sweep to all source to fix existing errors in another PR.

cc @envoyproxy/maintainers

mattklein123 · 2018-10-11T20:18:52Z

Can we ignore warnings in thirdparty code?

In general yes these warnings seem reasonable to me. Can we do diffs and then turn it on as failing? Then we can potentially have someone go through and clean up the existing code as a background exercise.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan · 2018-10-12T20:47:38Z

Can we ignore warnings in thirdparty code?

The default header filter already filtered out many of them, but some of them are still raised regardless what I specify for header filter, I guess some of them are in macro expansion etc. The compilation database doesn't contain third party cc files so they are not checked at all.

Added readability-braces-around-statements, fixes #4706.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan · 2018-10-23T23:57:16Z

#4835 addressed the CI failure, @mattklein123 PTAL again.

mattklein123 · 2018-10-24T01:05:47Z

@lizan I don't see clang-tidy running in the checks. Will it only run when merged to master? I thought it should run as part of this PR?

PiotrSikora · 2018-10-24T01:10:52Z

ci/build_setup.sh

-ln -sf "${ENVOY_SRCDIR}"/.bazelrc "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/
-ln -sf "${ENVOY_SRCDIR}"/.bazelrc "${ENVOY_CI_DIR}"/
+cp "${ENVOY_SRCDIR}"/.bazelrc "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/
+cp "${ENVOY_SRCDIR}"/.bazelrc "${ENVOY_CI_DIR}"/


What's the reason for this change? It leads to an error and early termination of CI step for me:

$ ENVOY_DOCKER_BUILD_DIR=~/build ./ci/run_envoy_docker.sh './ci/do_circle_ci.sh bazel.clang_tidy' disk space at beginning of build: Filesystem Size Used Avail Use% Mounted on overlay 504G 322G 163G 67% / tmpfs 64M 0 64M 0% /dev tmpfs 97G 0 97G 0% /sys/fs/cgroup /dev/sda1 504G 322G 163G 67% /build shm 64M 0 64M 0% /dev/shm tmpfs 97G 0 97G 0% /proc/acpi tmpfs 97G 0 97G 0% /sys/firmware No remote cache bucket is set, skipping setup remote cache. ENVOY_SRCDIR=/source HEAD is now at 3e5b733... cleanup: match NOT_IMPLEMENTED_GCOVR_EXCL_LINE change (#53) cp: '/source/.bazelrc' and '/source/ci/.bazelrc' are the same file disk space at end of build: Filesystem Size Used Avail Use% Mounted on overlay 504G 322G 163G 67% / tmpfs 64M 0 64M 0% /dev tmpfs 97G 0 97G 0% /sys/fs/cgroup /dev/sda1 504G 322G 163G 67% /build shm 64M 0 64M 0% /dev/shm tmpfs 97G 0 97G 0% /proc/acpi tmpfs 97G 0 97G 0% /sys/firmware

the bazel-compile-database doesn't read bazel options from env variable so the script dumps to bazelrc. this keeps git clean without writing to original file.

.circleci/config.yml

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

mattklein123 · 2018-10-24T18:19:55Z

@lizan can you introduce a temporary clang-tidy failure into this PR (that we can then revert) just so we can see what the CI output/failure looks like?

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

mattklein123 · 2018-10-24T21:29:19Z

@lizan nice this is awesome. If you revert the test commit this LGTM!

This reverts commit 6904c85. Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Using cp results in: cp: '/source/.bazelrc' and '/build/envoy-filter-example/.bazelrc' are the same file *Risk Level*: Low *Testing*: ./ci/run_envoy_docker.sh './ci/do_circle_ci.sh bazel.dev' *Docs Changes*: n/a *Release Notes*: n/a Broken in envoyproxy#4666. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Using cp results in: cp: '/source/.bazelrc' and '/build/envoy-filter-example/.bazelrc' are the same file *Risk Level*: Low *Testing*: ./ci/run_envoy_docker.sh './ci/do_circle_ci.sh bazel.dev' *Docs Changes*: n/a *Release Notes*: n/a Broken in #4666. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Using cp results in: cp: '/source/.bazelrc' and '/build/envoy-filter-example/.bazelrc' are the same file *Risk Level*: Low *Testing*: ./ci/run_envoy_docker.sh './ci/do_circle_ci.sh bazel.dev' *Docs Changes*: n/a *Release Notes*: n/a Broken in envoyproxy#4666. Signed-off-by: Piotr Sikora <piotrsikora@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

ci: add CI script to run clang-tidy

00fb3b5

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan mentioned this pull request Oct 10, 2018

ci: install clang-tidy in build image #4667

Merged

remove debug

ce3d2c6

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

alyssawilk assigned mattklein123 Oct 10, 2018

mattklein123 requested changes Oct 10, 2018

View reviewed changes

lizan force-pushed the clang_tidy branch from a55db4a to ab99343 Compare October 10, 2018 20:43

lizan added 3 commits October 10, 2018 20:44

add more comments

81565c3

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

add ci doc

48e6eb3

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Merge remote-tracking branch 'upstream/master' into clang_tidy

9a94f10

lizan force-pushed the clang_tidy branch from ab99343 to 9a94f10 Compare October 10, 2018 20:45

lizan commented Oct 10, 2018

View reviewed changes

lizan added 2 commits October 10, 2018 20:57

fix typo

fac609e

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Cleaning up .clang-tidy

3441099

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan added 3 commits October 12, 2018 19:39

add braces check

67bea26

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

run against diff

f55bfd3

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

fix path

1406e3b

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan added 4 commits October 19, 2018 20:19

Merge remote-tracking branch 'upstream/master' into clang_tidy

b1936dc

update build image

7817f46

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Merge remote-tracking branch 'upstream/master' into clang_tidy

8ad5da9

Merge remote-tracking branch 'upstream/master' into clang_tidy

125ba28

PiotrSikora reviewed Oct 24, 2018

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

review comments

8078e74

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

test clang-tidy

6904c85

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan added 2 commits October 24, 2018 21:32

Revert "test clang-tidy"

7cd3c85

This reverts commit 6904c85. Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Merge remote-tracking branch 'upstream/master' into clang_tidy

cf09277

mattklein123 approved these changes Oct 25, 2018

View reviewed changes

mattklein123 merged commit 08129e1 into envoyproxy:master Oct 25, 2018

lizan mentioned this pull request Oct 25, 2018

format: check/fix braces for all control statements #4706

Closed

lizan deleted the clang_tidy branch October 26, 2018 05:04

PiotrSikora mentioned this pull request Nov 6, 2018

build: cleanup ci/.bazelrc. #4978

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: run clang-tidy as a part of CI #4666

ci: run clang-tidy as a part of CI #4666

lizan commented Oct 10, 2018

mattklein123 left a comment

mattklein123 Oct 10, 2018

lizan Oct 10, 2018

mattklein123 commented Oct 10, 2018

lizan Oct 10, 2018

mattklein123 Oct 10, 2018

lizan Oct 10, 2018

mattklein123 Oct 10, 2018

lizan Oct 10, 2018

mattklein123 Oct 10, 2018

htuch Oct 10, 2018

lizan Oct 10, 2018

mattklein123 Oct 10, 2018

lizan commented Oct 11, 2018

mattklein123 commented Oct 11, 2018

lizan commented Oct 12, 2018

lizan commented Oct 23, 2018

mattklein123 commented Oct 24, 2018

PiotrSikora Oct 24, 2018

lizan Oct 24, 2018

mattklein123 commented Oct 24, 2018

mattklein123 commented Oct 24, 2018

		@@ -0,0 +1,306 @@
		---
		# -cppcoreguidelines-pro-type-vararg: It will alert on every ASSERT.

		@@ -0,0 +1,315 @@
		---
		# This file defines what clang-tidy checks and parameter. It is experimental, and may let

ci: run clang-tidy as a part of CI #4666

ci: run clang-tidy as a part of CI #4666

Conversation

lizan commented Oct 10, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan commented Oct 11, 2018

mattklein123 commented Oct 11, 2018

lizan commented Oct 12, 2018

lizan commented Oct 23, 2018

mattklein123 commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Oct 24, 2018

mattklein123 commented Oct 24, 2018