Skip to content

Commit

Permalink
i#2876 clang-format: check format in test suite (#3094)
Browse files Browse the repository at this point in the history 
Adds checking of diff formatting to runsuite.cmake.  If
clang-format-diff{,.py} is found, it is used to check the formatting, and
any format change is a fatal error.

Adds installation of clang-format-6.0 from apt.llvm.org on Travis to the
6th build, 64-bit clang.  It's shorter than the test runs, so this won't add
extra time.  Letting the test runs complete also allows a PR to get feedback
on build and test failures along with format issues simultaneously, rather
than simply having all builds fail up front with format errors.

Fixes #2876
  • Loading branch information
@derekbruening
derekbruening authored Jul 9, 2018
1 parent f1713ec commit d224ab1
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 12 deletions.
12 changes: 10 additions & 2 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,18 @@ jobs:
- os: linux
compiler: clang
env: DYNAMORIO_CROSS_AARCHXX_LINUX_ONLY=no DEPLOY=no EXTRA_ARGS=32_only
# 64-bit Linux build with clang, no tests (runsuite.cmake disables the tests):
# 64-bit Linux build with clang, no tests (runsuite.cmake disables the tests),
# install and require clang-format:
- os: linux
compiler: clang
env: DYNAMORIO_CROSS_AARCHXX_LINUX_ONLY=no DEPLOY=no EXTRA_ARGS=64_only
env: DYNAMORIO_CROSS_AARCHXX_LINUX_ONLY=no DEPLOY=no EXTRA_ARGS="64_only require_format"
addons:
apt:
sources:
- ubuntu-toolchain-r-test
- llvm-toolchain-trusty-6.0
packages:
- clang-format-6.0
# 32-bit OSX build with clang and run tests:
- os: osx
# gcc on Travis claims to not be CMAKE_COMPILER_IS_GNUCC so we only run clang.
Expand Down
71 changes: 61 additions & 10 deletions suite/runsuite.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ include("${CTEST_SCRIPT_DIRECTORY}/runsuite_common_pre.cmake")
# the list and did not remove its args so be sure to avoid conflicts).
set(arg_travis OFF)
set(arg_package OFF)
set(arg_require_format OFF)
set(cross_aarchxx_linux_only OFF)
set(cross_android_only OFF)
foreach (arg ${CTEST_SCRIPT_ARG})
Expand All @@ -62,6 +63,8 @@ foreach (arg ${CTEST_SCRIPT_ARG})
# conflicts if different architectures are being built: e.g., ARM
# and AArch64. It's up to the caller to arrange things properly.
set(arg_package ON)
elseif (${arg} STREQUAL "require_format")
set(arg_require_format ON)
endif ()
endforeach (arg)

Expand Down Expand Up @@ -129,22 +132,19 @@ if (EXISTS "${CTEST_SOURCE_DIRECTORY}/.svn" OR
if (svn_result OR svn_err)
message(FATAL_ERROR "*** ${SVN} diff failed: ***\n${svn_result} ${svn_err}")
endif (svn_result OR svn_err)
# Remove tabs from the revision lines
string(REGEX REPLACE "\n(---|\\+\\+\\+)[^\n]*\t" "" diff_contents "${diff_contents}")
endif (SVN)
else ()
if (EXISTS "${CTEST_SOURCE_DIRECTORY}/.git")
find_program(GIT git DOC "git client")
if (GIT)
# Included committed, staged, and unstaged changes.
# We assume "origin/master" contains the top-of-trunk.
execute_process(COMMAND "${GIT} diff origin/master"
# We pass -U0 so clang-format-diff only complains about touched lines.
execute_process(COMMAND ${GIT} diff -U0 origin/master
WORKING_DIRECTORY "${CTEST_SOURCE_DIRECTORY}"
RESULT_VARIABLE git_result
ERROR_VARIABLE git_err
OUTPUT_VARIABLE diff_contents)
# Remove tabs from the revision lines
string(REGEX REPLACE "\n(---|\\+\\+\\+)[^\n]*\t" "" diff_contents "${diff_contents}")
if (git_result OR git_err)
if (git_err MATCHES "unknown revision")
# It may be a cloned branch
Expand All @@ -156,7 +156,7 @@ else ()
endif ()
if (git_result OR git_err)
# Not a fatal error as this can happen when mixing cygwin and windows git.
message(STATUS "${GIT} remote -v failed: ${git_err}")
message(STATUS "${GIT} remote -v failed (${git_result}): ${git_err}")
set(git_out OFF)
endif (git_result OR git_err)
if (NOT git_out)
Expand All @@ -179,10 +179,61 @@ if (NOT DEFINED diff_contents)
message(FATAL_ERROR "Unable to construct diff for pre-commit checks")
endif ()

# Check for tabs. We already removed them from svn's diff format.
string(REGEX MATCH "\t" match "${diff_contents}")
# Ensure changes are formatted according to clang-format.
# XXX i#2876: we'd like to ignore changes to files like this which we don't
# want to mark up with "// clang-format off":
# + ext/drsyms/libefltc/include/*.h
# + third_party/libgcc/*.c
# + third_party/valgrind/*.h
# However, there's no simple way to do that. For now we punt until someone
# changes one of those.
#
# Prefer named version 6.0 from apt.llvm.org.
find_program(CLANG_FORMAT_DIFF clang-format-diff-6.0 DOC "clang-format-diff")
if (NOT CLANG_FORMAT_DIFF)
find_program(CLANG_FORMAT_DIFF clang-format-diff DOC "clang-format-diff")
endif ()
if (NOT CLANG_FORMAT_DIFF)
find_program(CLANG_FORMAT_DIFF clang-format-diff.py DOC "clang-format-diff")
endif ()
if (CLANG_FORMAT_DIFF)
get_filename_component(CUR_DIR "." ABSOLUTE)
set(diff_file "${CUR_DIR}/runsuite_diff.patch")
file(WRITE ${diff_file} "${diff_contents}")
execute_process(COMMAND ${CLANG_FORMAT_DIFF} -p1
WORKING_DIRECTORY "${CTEST_SOURCE_DIRECTORY}"
INPUT_FILE ${diff_file}
RESULT_VARIABLE format_result
ERROR_VARIABLE format_err
OUTPUT_VARIABLE format_out)
if (format_result OR format_err)
message(FATAL_ERROR
"Error (${format_result}) running clang-format-diff: ${format_err}")
endif ()
if (format_out)
# The WARNING and FATAL_ERROR message types try to format the diff and it
# looks bad w/ extra newlines, so we use STATUS for a more verbatim printout.
message(STATUS
"Changes are not formatted properly:\n${format_out}")
message(FATAL_ERROR
"FATAL ERROR: Changes are not formatted properly (see diff above)!")
else ()
message("clang-format check passed")
endif ()
else ()
if (arg_require_format)
message(FATAL_ERROR "FATAL ERROR: clang-format is required but not found!")
else ()
message("clang-format-diff not found: skipping format checks")
endif ()
endif ()

# Check for tabs other than on the revision lines.
# The clang-format check will now find these in C files, but not non-C files.
string(REGEX REPLACE "\n(---|\\+\\+\\+)[^\n]*\t" "" diff_notabs "${diff_contents}")
string(REGEX MATCH "\t" match "${diff_notabs}")
if (NOT "${match}" STREQUAL "")
string(REGEX MATCH "\n[^\n]*\t[^\n]*" match "${diff_contents}")
string(REGEX MATCH "\n[^\n]*\t[^\n]*" match "${diff_notabs}")
message(FATAL_ERROR "ERROR: diff contains tabs: ${match}")
endif ()

Expand All @@ -199,6 +250,7 @@ endif ()

# Check for trailing space. This is a diff with an initial column for +-,
# so a blank line will have one space: thus we rule that out.
# The clang-format check will now find these in C files, but not non-C files.
string(REGEX MATCH "[^\n] \n" match "${diff_contents}")
if (NOT "${match}" STREQUAL "")
# Get more context
Expand All @@ -208,7 +260,6 @@ endif ()

##################################################


# for short suite, don't build tests for builds that don't run tests
# (since building takes forever on windows): so we only turn
# on BUILD_TESTS for TEST_LONG or debug-internal-{32,64}
Expand Down
9 changes: 9 additions & 0 deletions suite/runsuite_wrapper.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,14 @@
'code_api|api.detach' => 1, # i#2246
'code_api|api.detach_spawn' => 1, # i#2611
'code_api|api.startstop' => 1, # i#2093
'code_api|client.drmgr-test' => 1, # i#653
'code_api|client.nudge_test' => 1, # i#2978
'code_api|client.nudge_ex' => 1);
%ignore_failures_64 = ('code_api|common.floatpc_xl8all' => 1,
'code_api|win32.reload-newaddr' => 1,
'code_api|client.loader' => 1,
'code_api|client.drmgr-test' => 1, # i#1369
'code_api|client.nudge_test' => 1, # i#2978
'code_api|client.nudge_ex' => 1,
'code_api|api.detach' => 1, # i#2246
'code_api|api.detach_spawn' => 1, # i#2611
Expand Down Expand Up @@ -155,6 +158,12 @@
} else {
$issue_no = "#2417";
}
} else {
# FIXME i#2921: fix flaky ptsig test
%ignore_failures_32 = ('code_api|pthreads.ptsig' => 1);
# FIXME i#2941: fix flaky threadfilter test
%ignore_failures_64 = ('code_api|tool.drcacheoff.burst_threadfilter' => 1);
$issue_no = "#2941";
}

# Read ahead to examine the test failures:
Expand Down

0 comments on commit d224ab1

Please sign in to comment.