Enable clang-tidy in CI #12213

goldsborough · 2018-09-30T23:51:17Z

At long last, we will have clang-tidy enabled in CI. For a while I thought I could clean up the project enough to enable clang-tidy with all checks enabled, but I figure it's smarter to set up the minimal checks and at least have those in CI. We can fix more going forward.

@ezyang @apaszke

.travis.yml

@@ -29,3 +29,18 @@ matrix:
      - env: CPP_DOC_CHECK
        install: sudo apt-get install -y doxygen
        script: cd docs/cpp/source && ./check-doxygen.sh
+      - env: CLANG_TIDY


.travis.yml

+          - git submodule update --init --recursive
+          - mkdir clang-tidy-build
+          - cd clang-tidy-build
+          - cmake -DBUILD_TORCH=ON ..


ezyang · 2018-10-01T15:50:35Z

The job doesn't seem to be failing on errors. On https://travis-ci.org/pytorch/pytorch/jobs/435471449 I see:

1.33s$ python tools/clang_tidy.py -c "$(which clang-tidy-6.0)" -d clang-tidy-build -p torch/csrc -r HEAD~1
/home/travis/build/pytorch/pytorch/torch/csrc/jit/interned_strings.h:9:10: error: 'torch/csrc/jit/generated/aten_interned_strings.h' file not found [clang-diagnostic-error]
#include "torch/csrc/jit/generated/aten_interned_strings.h"

CMakeLists.txt

@@ -10,6 +10,8 @@ if (NOT MSVC)
  set(CMAKE_C_STANDARD 11)
 endif()

+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)


CONTRIBUTING.md

+how to install the latest version. Note that newer versions of clang-tidy will
+have more checks than older versions. In our CI, we run clang-tidy-6.0.
+
+2. Use our driver script to run clang-tidy over your git revision (you may want


CONTRIBUTING.md

+  $ python tools/clang_tidy.py -d /path/to/build/ -p torch/csrc -r HEAD~1
+  ```
+
+Above, it is assumed you are in the PyTorch root folder. `path/to/build` should


tools/autograd/templates/Functions.cpp

+// NB: Must be at the top of file to avoid including the deprecated "math.h".
+// https://stackoverflow.com/questions/6563810/m-pi-works-with-math-h-but-not-with-cmath-in-visual-studio
+#ifdef _MSC_VER
+#define _USE_MATH_DEFINES


tools/clang_tidy.py

@@ -8,28 +8,35 @@
 import sys


-DEFAULT_FILE_PATTERN = r".*\.[ch](pp)?"
+DEFAULT_FILE_PATTERN = r".*\.c(c|pp)?"


tools/clang_tidy.py

    """Executes a shell command."""
    assert len(arguments) > 0
    try:
-        output = subprocess.check_output(arguments, stderr=subprocess.STDOUT)


torch/CMakeLists.txt

@@ -7,6 +7,7 @@ else()
  project(torch CXX C)
  find_package(Caffe2 REQUIRED)
  option(USE_CUDA "Use CUDA" ON)
+  set(CMAKE_EXPORT_COMPILE_COMMANDS ON)


tools/clang_tidy.py

-        output = subprocess.check_output(arguments, stderr=subprocess.STDOUT)
-    except OSError:
-        _, e, _ = sys.exc_info()
+        if verbose:


tools/clang_tidy.py

        process_name = process_name or arguments[0]
-        raise RuntimeError("Error executing {}: {}".format(process_name, e))
+        _, error, _ = sys.exc_info()
+        message = "Error executing '{}'".format(process_name)


ezyang

I have quibbles about how the Python script is written, but I'm requesting changes here because it doesn't seem like it is actually flagging errors as failures in CI.

goldsborough · 2018-10-01T16:08:23Z

What you're seeing are clang compiler errors (clang-diagnostic-error). Those are because I don't do a full build of PyTorch since that would take too long in CI. I only generate the .compile_commands.json and clang-tidy will happily lint 99% of code that it can parse and compile. The clang diagnostics can be ignored. It means we're not linting generated files, but I think that's fine. In any case, there is nothing broken here, it's expected. When there is an actual lint error (which I have verified of course), those will make the CI error.

goldsborough · 2018-10-01T16:09:27Z

Now that I think of it, I can just run our gen.py script to generate the missing files. I also do that in our doc CI build. In any case the lint job will take 2 minutes and not 20 minutes or however long it takes to build PyTorch

CONTRIBUTING.md

+should be able to install it easily with your package manager, e.g. by writing
+`apt-get install clang-tidy` on Ubuntu. See https://apt.llvm.org for details on
+how to install the latest version. Note that newer versions of clang-tidy will
+have more checks than older versions. In our CI, we run clang-tidy-6.0.


tools/run-clang-tidy-in-ci.sh

+
+set -ex
+
+if [[ ! -d build ]]; then


tools/run-clang-tidy-in-ci.sh

+  popd
+
+  # Generate ATen files.
+  time python aten/src/ATen/gen.py \


tools/clang_tidy.py

@@ -8,28 +8,35 @@
 import sys


ezyang · 2018-10-03T14:32:59Z

CR on clang_tidy.py

https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L164 This is not the idiomatic way to pass extra arguments to an underlying executable, when there is a unique such executable (as is this case); these arguments should come after a -- in the command line. See Allow passing through arguments to unittest #11209 for an example of adding this functionality to a Python script using argparse.
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L207 This is not the idiomatic flag name; the idiomatic name is --dry-run with abbreviation -n. See for example https://linux.die.net/man/1/rsync https://linux.die.net/man/1/git-review https://linux.die.net/man/1/hg (Sometimes people accept -s, but that's short for --simulate)
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L59 This is not the correct way to query git programatically. You should use a plumbing command, rather than the porcelain command (see also Git - Plumbing and Porcelain <https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain>_). Using plumbing will reduce the amount of preparatory work you have to do to get the command to give you something machine only (e.g., you don't have to pass --no-pager and --no-color). In this case, because you want to diff against the working tree, you should use git-diff-index. Read the manpage for this command to see how it can be done.
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L110 No, please don't do this. If you pass the base directory, this will walk recursively into the build directory and .git and grab you all of your build products too. Even if you use it on a subdirectory, it will still grab any build products. Use git-ls-files to get an accurate listing of all files in Git which are checked into source control and can be profitably linted.
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L232 This seems like an awfully circuitous way to implement recursive file removal.
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L183 The doc here should mention that regexes implicitly get $ appended to them
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L44 Uhhhhh no, please don't do this. Use https://docs.python.org/2/library/fnmatch.html#fnmatch.fnmatch instead
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L138 That's weird, http://manpages.ubuntu.com/manpages/xenial/man1/clang-tidy-3.7.1.html seems to imply that clang-tidy understands YAML here too. What's going on? In fact, clang-tidy seems to imply that it will automatically try to read .clang-tidy itself
https://github.com/pytorch/pytorch/blob/5564d8c0af37d74bd9ff0f19d083d065244acc08/tools/clang_tidy.py#L17 I still find this function deeply dubious.

ezyang · 2018-10-03T14:42:35Z

(sorry, accidentally closed the PR with an incorrect comment, please disregard)

ezyang · 2018-10-03T14:45:08Z

Message me if you want to chat more about the verbose output situation. I want to understand what's going on here.

Move code into script

goldsborough · 2018-10-03T18:22:38Z

Hard to respond to your comments directly this way. I'll fix the code you commented on. I think the only open ended question was "why pass the .yaml file to clang-tidy". Believe me, I've spent many hours fighting clang tidy because it wouldn't pick up my .clang-tidy file. It seems a thing about different clang tidy versions. There's unanswered stackoverflow questions about this too. The best way to ensure that all the config gets picked up is to pass it in directly the way I do.

I'll see how hard it is to create a cmake target to generate the files. I think it's more important to have clang tidy in CI as soon as possible than for this to be 100% clean. We could literally already be finding tons of bugs right now

goldsborough · 2018-10-03T18:48:00Z

No, please don't do this. If you pass the base directory, this will walk recursively into the build directory and .git and grab you all of your build products too. Even if you use it on a subdirectory, it will still grab any build products. Use git-ls-files to get an accurate listing of all files in Git which are checked into source control and can be profitably linted.

Why would you pass the base directory? This gets e.g. torch/csrc as its argument, that would certainly only include source files?

ezyang · 2018-10-03T19:39:06Z

Believe me, I've spent many hours fighting clang tidy because it wouldn't pick up my .clang-tidy file. It seems a thing about different clang tidy versions. There's unanswered stackoverflow questions about this too. The best way to ensure that all the config gets picked up is to pass it in directly the way I do.

That's fine, but then it should be commented in the code :)

Why would you pass the base directory? This gets e.g. torch/csrc as its argument, that would certainly only include source files?

I've seen too many programs written using os.walk (it worked fine on my machine!) blow up to be that sympathetic to this argument :) It should work without weird performance cliffs in all cases, or we shouldn't provide it at all.

ezyang · 2018-10-03T19:42:16Z

In terms of priorities, the main things I want to see fixed are:

Removal of footguns from the Python script (this is mostly from the perspective of, this kind of Python code tends to replicate itself in the codebase if let alone for a while, and I don't want the footguns replicating themselves.)
Some resolution on the issue where clang-tidy will silently choke if compile commands are busted without erroring. I'm willing to be convinced this is not a problem in practice, but we've had multiple cases in the past where we silently stopped testing code and if there is something easy we can do now to prevent this from happening, I'd prefer it.

Everything else is non-essential.

facebook-github-bot

goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

goldsborough requested review from apaszke, colesbury, ebetica, ezyang, gchanan, soumith and zdevito as code owners September 30, 2018 23:51

goldsborough force-pushed the clang-tidy-ci branch 3 times, most recently from 8cd3ee4 to 56e25cc Compare October 1, 2018 07:27