[CI] Add cpplint as sanity check

[Squadrick]

Make the required changes to ensure clean cpplint run.
Command:

$ cpplint --filter=-build/header_guard --recursive tensorflow_addons/custom_ops

[bhack]

Do we have a cpplint hook?

[bhack]

Probably we need a clang-format hook right?
https://www.tensorflow.org/community/contribute/code_style#style_di_codifica_c

[bhack]

We already have pre-commit and sanity check with clang-format so what happens here?

[bhack]

I see no special derived extra style rules in Tensorflow:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/.clang-format

So I think we could align with clang-format pass.

[Squadrick]

@bhack This is not for formatting, it's using cpplint. The changes here are to make everything uniform and some accepted practices for C++ (explicit c'tor).

We might want to consider adding cpplint to our CI (cc @gabrieldemarmiesse @seanpmorgan)

[bhack]

Ok so It Is like in the perimeter of clang-tidy (for clang tools)

[bhack]

I've seen some comments in Tensorflow source code related to clang-tidy

Do you know if there Is a clang-tidy pass with a configuration in the Tensorflow CI?

[Squadrick]

@bhack I don't think TensorFlow runs clang-tidy, at least I couldn't find anything in their "Ubuntu Sanity". There are two options for running C++ lining: cpplint, clang-tidy. cpplint is much each easier to install and run (pip), imo.

I used cpplint since it's a Google tool, but I can test with clang-tidy to maintain compatibility with TensorFlow.

[bhack]

Yes the scope was just to know how to create an uniform experience.
But other then that comments in the source code I found a public configuration only in MLIR but not in TF: https://github.com/llvm/llvm-project/blob/master/mlir/.clang-tidy

[gabrieldemarmiesse]

I'm not a C++ dev, so I'll trust you @Squadrick if you want to add another tool for c++ in the CI. You can add an entry to the sanity checks dockerfile.

[bhack]

I am ok but I don't find also cpplint in their public Sanity check CI scripts. @angerson do you have any info about a clang-tidy or cpplint pass in Tensorflow CI?

[WindQAQ]

Though we do not necessarily need to stick with the style of TF (like black), I personally prefer clang-tidy to cpplint.

[bhack]

E.g. on Chromium the config Is pubblic https://github.com/chromium/chromium/blob/master/.clang-tidy

[angerson]

Here's TensorFlow's style guide, which advocates clang-tidy: https://www.tensorflow.org/community/contribute/code_style?hl=en

[bhack]

@angerson If you see we already mentioned that public link in this ticket. But that Is related to clang-format not about static checks (clang-tidy Is only the package name to install there not the binary to run)

[angerson]

Either cpplint or clang-tidy's linter should be fine, then. Google exposes both for linting changesets, but IIRC TensorFlow's CI does not specifically enforce any linting.

[bhack]

As we have no formal pass in Tensorflow and we have already clang-tidy installed from the same package of clang-format in our CI we could do what we want. I am not sure but I Remember that if we will need clang-tidy can check a superset of cpplint but I need to double check this.

[gunan]

When importing changes, we do run clang-tidy with the internal config on them.
Unfortunately, the configuration is not under our control, and thus running them in OSS has been a problem, because when we ran it before, it changed without notifications and broke us.

So, the first question I have is, is ensorflow/addons mirrored internally, now or sometime in the future(@karmel @theadactyl ). If yes, you may want to use clang-tidy instead.
Otherwise, if the code does not need to get inside google, you can apply any linter you like.

[bhack]

The ideal soultion would be to have a public clang-tidy conf like in Chromium to have an homogenus experience across SIGs and Tensorflow code for contributors but I don't know the internal policies why this could be public in Chromium and not for Tensorflow as boths proejcts have close soruce versions.

[gunan]

Not exactly. Chromium uses a completely different scm than the rest of google.
Therefore, they are able to avoid running the google linters on their source code, and completely run open source versions.

[bhack]

@gunan Thanks for the little disclosure. I don't have a view on the internal Google repos so mine was just a reverse (engineering) guessing about how you are organized. I picked Chromium as an example cause at some point I thought it is going to integrate in the Chrome codebase and it could have a similar relationship like the opensource and closed source Tensorflow versions (and it is why I picked that specific example and e.g. not Clang tidy config in Goolge-cloud-cpp.
If we cannot know what Google it is doing internally I think that we could just expect that by an external point of view (the contributor) we don't need any enforcing pass and so we don't need to worry about the homogeneity of contributors coding experience across the Tensorflow ecosystem/SIGs.

[Squadrick]

@gabrieldemarmiesse @seanpmorgan I've added cpplint to run a linter on custom_ops. I chose cpplint since it's easier to work with and install, and it comes with a Google style built-in.

Could you review this, please?

[bhack]

I think that we could separate cpplint introduction in the PR and the cpplint pass in another PR.
Also you need to add this in https://github.com/tensorflow/addons/blob/master/tools/docker/pre-commit.Dockerfile.

[bhack]

I have an additional question. Is it still officially maintained by Google? See google/styleguide#528.

[bhack]

From https://pypi.org/project/cpplint/

While Google maintains cpplint, Google is not (very) responsive to issues and pull requests, this fork aims to be (somewhat) more open to add fixes to cpplint to enable fixes, when those fixes make cpplint usable in wider contexts. Also see discussion here google/styleguide#528.