From 6ceaa7523f6803d89f6fed0798f913db94156449 Mon Sep 17 00:00:00 2001 From: Thom Troy Date: Sat, 22 Jul 2017 20:32:24 +0100 Subject: [PATCH] refactor the clang format example and add test --- .../clang-format/CMakeLists.txt | 1 + 04-static-analysis/clang-format/README.adoc | 47 +++-- .../cmake/modules/FindClangFormat.cmake | 2 +- .../cmake/modules/clang-format.cmake | 28 ++- .../scripts/clang-format-check-changed.py | 163 ++++++++++++++++++ 04-static-analysis/clang-format/run_test.sh | 11 ++ dockerfiles/ubuntu14.04-cmake-3.4.3 | 1 + dockerfiles/ubuntu14.04-default-2.8.12.2 | 1 + dockerfiles/ubuntu16.04-default-cmake-3.5.1 | 1 + test.sh | 1 + 10 files changed, 235 insertions(+), 21 deletions(-) create mode 100755 04-static-analysis/clang-format/cmake/scripts/clang-format-check-changed.py create mode 100755 04-static-analysis/clang-format/run_test.sh diff --git a/04-static-analysis/clang-format/CMakeLists.txt b/04-static-analysis/clang-format/CMakeLists.txt index 0f848be..a83015d 100644 --- a/04-static-analysis/clang-format/CMakeLists.txt +++ b/04-static-analysis/clang-format/CMakeLists.txt @@ -11,4 +11,5 @@ add_subdirectory(subproject1) add_subdirectory(subproject2) set(CLANG_FORMAT_BIN_NAME clang-format-3.6) +set(CLANG_FORMAT_EXCLUDE_PATTERNS "build/" ${CMAKE_BINARY_DIR}) find_package(ClangFormat) diff --git a/04-static-analysis/clang-format/README.adoc b/04-static-analysis/clang-format/README.adoc index 07a211c..90f665c 100644 --- a/04-static-analysis/clang-format/README.adoc +++ b/04-static-analysis/clang-format/README.adoc @@ -32,14 +32,15 @@ $ tree ``` * link:CMakeLists.txt[] - Top level CMakeLists.txt - * link:.clang-format[] - The file describing the stype guide + * link:.clang-format[] - The file describing the style guide * link:cmake/modules/FindClangFormat.cmake[] - Script to find the clang-format binary * link:cmake/modules/clang-format.cmake[] - Script to setup the format targets - * link:cmake/scripts/clang-format-check-changed[] - A helper script to check against changed files in git + * link:cmake/scripts/clang-format-check-changed.py[] - A helper script to check against changed files in git + * link:cmake/scripts/clang-format-check-changed.py[] - An old simplified version of the above helper script * link:subproject1/CMakeLists.txt[] - CMake commands for subproject 1 - * link:subproject1/main.cpp[] - source for a subproject with no errors + * link:subproject1/main.cpp[] - source for a subproject with no style errors * link:subproject2/CMakeLists.txt[] - CMake commands for subproject 2 - * link:subproject2/main2.cpp[] - source for a subproject that includes errors + * link:subproject2/main2.cpp[] - source for a subproject that includes style errors # Requirements @@ -47,16 +48,18 @@ To run this example you must have clang format tool installed. This can be insta [source,bash] ---- -$ sudo apt-get install clang-format +$ sudo apt-get install clang-format-3.6 ---- It will result in the tool being available as: [source,bash] ---- -$ clang-format +$ clang-format-3.6 ---- +If you install a different version, you can edit the `CLANG_FORMAT_BIN_NAME` variable in the root CMakeLists.txt + # Concepts ## clang-format @@ -95,10 +98,10 @@ The format target will find any C++ source files and in place modify them to mat [source,cmake] ---- -file(GLOB_RECURSE ALL_SOURCE_FILES *.cpp *.h *.cxx *.hxx *.hpp *.cc) +file(GLOB_RECURSE ALL_SOURCE_FILES *.cpp *.h *.cxx *.hxx *.hpp *.cc *.ipp) # Don't include some common build folders -set(CLANG_FORMAT_EXCLUDE_PATTERNS ${CLANG_FORMAT_EXCLUDE_PATTERNS} "build/" "/CMakeFiles/") +set(CLANG_FORMAT_EXCLUDE_PATTERNS ${CLANG_FORMAT_EXCLUDE_PATTERNS} "/CMakeFiles/" "cmake") # get all project files file foreach (SOURCE_FILE ${ALL_SOURCE_FILES}) @@ -112,7 +115,14 @@ endforeach () ---- This will find files matching the common C++ suffixes and then remove any that match some -common CMake build directories. +common CMake directories. + +In the root `CMakeList.txt` we also exclude the build directory by adding the line + +[source,cmake] +---- +set(CLANG_FORMAT_EXCLUDE_PATTERNS "build/") +---- ## format-check @@ -123,13 +133,20 @@ if any files don't match the clang-format style This target will check the output of `git status` and scan the files to check if they match the style. This can be used by developers to make sure their changed files match the correct style. -In this example the actual check is done with a helper script +clang-format-check-changed+. This calls the following command to check files: +In this example the actual check is done with a helper script +clang-format-check-changed.py+. This script will run `git status --porcelain --ignore-submodules` +to get a list of changed files, match them against the allowed extensions from the above list, and finally remove any +that match the exclude pattern from +CLANG_FORMAT_EXCLUDE_PATTERNS+. It will then run these files through clang-format and +exit with an error if the files do not match the style. + +An example call to the +clang-format-check-changed.py+ script is: [source,bash] ---- -git status --porcelain \ - | egrep '*\.cpp|*\.h|*\.cxx|*\.hxx|*\.hpp|*\.cc' \ - | awk -F " " '{print $NF}' \ - | xargs -r clang-format -style=file -output-replacements-xml \ - | grep "replacement offset" 2>&1 > /dev/null +cmake/scripts/clang-format-check-changed.py --file-extensions ".cpp,*.cpp,*.h,*.cxx,*.hxx,*.hpp,*.cc,*.ipp" --exclude=build/ --exclude=/CMakeFiles/ --exclude=cmake --clang-format-bin /usr/bin/clang-format-3.6 ---- + +[NOTE] +==== +This will include all changed files in your git repository that match the patterns. In this example repository this can include files that +are part of different examples. +==== diff --git a/04-static-analysis/clang-format/cmake/modules/FindClangFormat.cmake b/04-static-analysis/clang-format/cmake/modules/FindClangFormat.cmake index 4dcd02a..c627343 100644 --- a/04-static-analysis/clang-format/cmake/modules/FindClangFormat.cmake +++ b/04-static-analysis/clang-format/cmake/modules/FindClangFormat.cmake @@ -6,7 +6,7 @@ if(NOT CLANG_FORMAT_BIN_NAME) endif() # if custom path check there first -if(CPPCHECK_ROOT_DIR) +if(CLANG_FORMAT_ROOT_DIR) find_program(CLANG_FORMAT_BIN NAMES ${CLANG_FORMAT_BIN_NAME} diff --git a/04-static-analysis/clang-format/cmake/modules/clang-format.cmake b/04-static-analysis/clang-format/cmake/modules/clang-format.cmake index 1527560..5e631b2 100644 --- a/04-static-analysis/clang-format/cmake/modules/clang-format.cmake +++ b/04-static-analysis/clang-format/cmake/modules/clang-format.cmake @@ -1,10 +1,11 @@ # A CMake script to find all source files and setup clang-format targets for them # Find all source files -file(GLOB_RECURSE ALL_SOURCE_FILES *.cpp *.h *.cxx *.hxx *.hpp *.cc) +set(CLANG_FORMAT_CXX_FILE_EXTENSIONS ${CLANG_FORMAT_CXX_FILE_EXTENSIONS} *.cpp *.h *.cxx *.hxx *.hpp *.cc *.ipp) +file(GLOB_RECURSE ALL_SOURCE_FILES ${CLANG_FORMAT_CXX_FILE_EXTENSIONS}) # Don't include some common build folders -set(CLANG_FORMAT_EXCLUDE_PATTERNS ${CLANG_FORMAT_EXCLUDE_PATTERNS} "main2.cpp" "build/" "/CMakeFiles/") +set(CLANG_FORMAT_EXCLUDE_PATTERNS ${CLANG_FORMAT_EXCLUDE_PATTERNS} "/CMakeFiles/" "cmake") # get all project files file foreach (SOURCE_FILE ${ALL_SOURCE_FILES}) @@ -24,21 +25,38 @@ add_custom_target(format ${ALL_SOURCE_FILES} ) + add_custom_target(format-check COMMENT "Checking clang-format changes" - COMMAND ${CLANG_FORMAT_BIN} + # Use ! to negate the result for correct output + COMMAND ! + ${CLANG_FORMAT_BIN} -style=file -output-replacements-xml ${ALL_SOURCE_FILES} - | grep "replacement offset" 2>&1 > /dev/null + | grep -q "replacement offset" ) # Get the path to this file get_filename_component(_clangcheckpath ${CMAKE_CURRENT_LIST_FILE} PATH) +# have at least one here by default +set(CHANGED_FILE_EXTENSIONS ".cpp") +foreach(EXTENSION ${CLANG_FORMAT_CXX_FILE_EXTENSIONS}) + set(CHANGED_FILE_EXTENSIONS "${CHANGED_FILE_EXTENSIONS},${EXTENSION}" ) +endforeach() + +set(EXCLUDE_PATTERN_ARGS) +foreach(EXCLUDE_PATTERN ${CLANG_FORMAT_EXCLUDE_PATTERNS}) + list(APPEND EXCLUDE_PATTERN_ARGS "--exclude=${EXCLUDE_PATTERN}") +endforeach() + # call the script to chech changed files in git add_custom_target(format-check-changed COMMENT "Checking changed files in git" WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} - COMMAND ${_clangcheckpath}/../scripts/clang-format-check-changed ${CLANG_FORMAT_BIN} + COMMAND ${_clangcheckpath}/../scripts/clang-format-check-changed.py + --file-extensions \"${CHANGED_FILE_EXTENSIONS}\" + ${EXCLUDE_PATTERN_ARGS} + --clang-format-bin ${CLANG_FORMAT_BIN} ) diff --git a/04-static-analysis/clang-format/cmake/scripts/clang-format-check-changed.py b/04-static-analysis/clang-format/cmake/scripts/clang-format-check-changed.py new file mode 100755 index 0000000..6c91c2a --- /dev/null +++ b/04-static-analysis/clang-format/cmake/scripts/clang-format-check-changed.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python + +import argparse +import os +import sys +import subprocess + + +def check_file(filename, excludes, extensions): + """ + Check if a file should be included in our check + """ + name, ext = os.path.splitext(filename) + + if len(ext) > 0 and ext in extensions: + if len(excludes) == 0: + return True + + for exclude in excludes: + if exclude in filename: + return False + + return True + + return False + + +def check_directory(directory, excludes, extensions): + output = [] + + if len(excludes) > 0: + for exclude in excludes: + if exclude in directory: + directory_excluded = False + return output + + for root, _, files in os.walk(directory): + for file in files: + filename = os.path.join(root, file) + if check_file(filename, excludes, extensions): + print("Will check file [{}]".format(filename)) + output.append(filename) + return output + +def get_git_root(git_bin): + cmd = [git_bin, "rev-parse", "--show-toplevel"] + try: + return subprocess.check_output(cmd).strip() + except subprocess.CalledProcessError, e: + print("Error calling git [{}]".format(e)) + raise + +def clean_git_filename(line): + """ + Takes a line from git status --porcelain and returns the filename + """ + file = None + git_status = line[:2] + # Not an exhaustive list of git status output but should + # be enough for this case + # check if this is a delete + if 'D' in git_status: + return None + # ignored file + if '!' in git_status: + return None + # Covers renamed files + if '->' in line: + file = line[3:].split('->')[-1].strip() + else: + file = line[3:].strip() + + return file + + +def get_changed_files(git_bin, excludes, file_extensions): + """ + Run git status and return the list of changed files + """ + extensions = file_extensions.split(",") + # arguments coming from cmake will be *.xx. We want to remove the * + for i, extension in enumerate(extensions): + if extension[0] == '*': + extensions[i] = extension[1:] + + git_root = get_git_root(git_bin) + + cmd = [git_bin, "status", "--porcelain", "--ignore-submodules"] + print("git cmd = {}".format(cmd)) + output = [] + returncode = 0 + try: + cmd_output = subprocess.check_output(cmd) + for line in cmd_output.split('\n'): + if len(line) > 0: + file = clean_git_filename(line) + if not file: + continue + file = os.path.join(git_root, file) + + if file[-1] == "/": + directory_files = check_directory( + file, excludes, file_extensions) + output = output + directory_files + else: + if check_file(file, excludes, file_extensions): + print("Will check file [{}]".format(file)) + output.append(file) + + except subprocess.CalledProcessError, e: + print("Error calling git [{}]".format(e)) + returncode = e.returncode + + return output, returncode + + +def run_clang_format(clang_format_bin, changed_files): + """ + Run clang format on a list of files + @return 0 if formatted correctly. + """ + if len(changed_files) == 0: + return 0 + cmd = [clang_format_bin, "-style=file", + "-output-replacements-xml"] + changed_files + print("clang-format cmd = {}".format(cmd)) + try: + cmd_output = subprocess.check_output(cmd) + if "replacement offset" in cmd_output: + print("ERROR: Changed files don't match format") + return 1 + except subprocess.CalledProcessError, e: + print("Error calling clang-format [{}]".format(e)) + return e.returncode + + return 0 + + +def cli(): + # global params + parser = argparse.ArgumentParser(prog='clang-format-check-changed', + description='Checks if files chagned in git match the .clang-format specification') + parser.add_argument("--file-extensions", type=str, + default=".cpp,.h,.cxx,.hxx,.hpp,.cc,.ipp", + help="Comma seperated list of file extensions to check") + parser.add_argument('--exclude', action='append', default=[], + help='Will not match the files / directories with these in the name') + parser.add_argument('--clang-format-bin', type=str, default="clang-format", + help="The clang format binary") + parser.add_argument('--git-bin', type=str, default="git", + help="The git binary") + args = parser.parse_args() + + # Run gcovr to get the .gcda files form .gcno + changed_files, returncode = get_changed_files( + args.git_bin, args.exclude, args.file_extensions) + if returncode != 0: + return returncode + + return run_clang_format(args.clang_format_bin, changed_files) + +if __name__ == '__main__': + sys.exit(cli()) diff --git a/04-static-analysis/clang-format/run_test.sh b/04-static-analysis/clang-format/run_test.sh new file mode 100755 index 0000000..40bd914 --- /dev/null +++ b/04-static-analysis/clang-format/run_test.sh @@ -0,0 +1,11 @@ +#!/bin/bash +mkdir -p build && cd build && cmake .. && make format-check +RET=$? +echo "return code was ${RET}" +if [ ${RET} == "0" ]; then + echo "test failed. Expected format-check to fail" + exit 1 +else + echo "test success" + exit 0 +fi diff --git a/dockerfiles/ubuntu14.04-cmake-3.4.3 b/dockerfiles/ubuntu14.04-cmake-3.4.3 index 497bb3f..280542a 100644 --- a/dockerfiles/ubuntu14.04-cmake-3.4.3 +++ b/dockerfiles/ubuntu14.04-cmake-3.4.3 @@ -8,6 +8,7 @@ RUN apt-get update && apt-get install -y build-essential \ libprotobuf-dev \ protobuf-compiler \ clang-3.6 \ + clang-format-3.6 \ ninja-build \ wget \ git \ diff --git a/dockerfiles/ubuntu14.04-default-2.8.12.2 b/dockerfiles/ubuntu14.04-default-2.8.12.2 index f704310..bcb1349 100644 --- a/dockerfiles/ubuntu14.04-default-2.8.12.2 +++ b/dockerfiles/ubuntu14.04-default-2.8.12.2 @@ -10,6 +10,7 @@ RUN apt-get update && apt-get install -y build-essential \ protobuf-compiler \ cppcheck \ clang-3.6 \ + clang-format-3.6 \ ninja-build \ wget \ git \ diff --git a/dockerfiles/ubuntu16.04-default-cmake-3.5.1 b/dockerfiles/ubuntu16.04-default-cmake-3.5.1 index a88893b..3500cba 100644 --- a/dockerfiles/ubuntu16.04-default-cmake-3.5.1 +++ b/dockerfiles/ubuntu16.04-default-cmake-3.5.1 @@ -9,6 +9,7 @@ RUN apt-get update && apt-get install -y build-essential \ libprotobuf-dev \ protobuf-compiler \ clang-3.6 \ + clang-format-3.6 \ ninja-build \ wget \ git \ diff --git a/test.sh b/test.sh index 9449a37..36ca41e 100755 --- a/test.sh +++ b/test.sh @@ -31,6 +31,7 @@ dirs=(./01-basic/A-hello-cmake \ ./04-static-analysis/cppcheck \ ./04-static-analysis/cppcheck-compile-commands \ ./04-static-analysis/clang-analyzer \ +./04-static-analysis/clang-format \ ./05-unit-testing/boost \ ./06-installer/deb \ )