diff options
| -rw-r--r-- | .travis.yml | 6 | ||||
| -rw-r--r-- | Makefile.am | 73 | ||||
| -rw-r--r-- | changes/ticket31919 | 13 | ||||
| -rwxr-xr-x | scripts/coccinelle/check_cocci_parse.sh | 52 | ||||
| -rw-r--r-- | scripts/coccinelle/exceptions.txt | 24 | ||||
| -rwxr-xr-x | scripts/coccinelle/try_parse.sh | 25 | ||||
| -rwxr-xr-x | scripts/git/pre-commit.git-hook | 78 | ||||
| -rwxr-xr-x | scripts/git/pre-push.git-hook | 153 |
8 files changed, 301 insertions, 123 deletions
diff --git a/.travis.yml b/.travis.yml index eda8d17bc5..ea06d21db5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -117,6 +117,8 @@ addons: - libseccomp-dev ## zstd doesn't exist in Ubuntu Trusty #- libzstd + ## Optional build dependencies + - coccinelle - shellcheck ## Conditional build dependencies ## Always installed, so we don't need sudo @@ -146,6 +148,7 @@ addons: - pkg-config ## Optional build dependencies - ccache + - coccinelle - shellcheck ## Conditional build dependencies ## Always installed, because manual brew installs are hard to get right @@ -202,6 +205,9 @@ install: - if [[ "$CHUTNEY" != "" ]]; then pushd "$CHUTNEY_PATH"; git log -1 ; popd ; fi ## If we're running stem, show the stem version and commit - if [[ "$TEST_STEM" != "" ]]; then pushd stem; python -c "from stem import stem; print(stem.__version__);"; git log -1; popd; fi + ## Get the coccinelle version + ## Installs are unreliable on macOS, so we just rely on brew list --versions + - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then spatch --version; fi ## We don't want Tor tests to depend on default configuration file at ## ~/.torrc. So we put some random bytes in there, to make sure we get build ## failures in case Tor is reading it during CI jobs. diff --git a/Makefile.am b/Makefile.am index 32cb21f38c..9866f87f8c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -158,37 +158,43 @@ include doc/include.am include contrib/include.am EXTRA_DIST+= \ - ChangeLog \ - CONTRIBUTING \ - CODE_OF_CONDUCT \ - INSTALL \ - LICENSE \ - Makefile.nmake \ - README \ - ReleaseNotes \ - scripts/maint/checkIncludes.py \ - scripts/maint/checkSpace.pl \ - scripts/maint/checkShellScripts.sh \ - scripts/maint/practracker/README \ - scripts/maint/practracker/exceptions.txt \ - scripts/maint/practracker/includes.py \ - scripts/maint/practracker/metrics.py \ - scripts/maint/practracker/practracker.py \ - scripts/maint/practracker/practracker_tests.py \ - scripts/maint/practracker/problem.py \ - scripts/maint/practracker/testdata/.may_include \ - scripts/maint/practracker/testdata/a.c \ - scripts/maint/practracker/testdata/b.c \ - scripts/maint/practracker/testdata/ex0-expected.txt \ - scripts/maint/practracker/testdata/ex0.txt \ - scripts/maint/practracker/testdata/ex1-expected.txt \ - scripts/maint/practracker/testdata/ex1.txt \ - scripts/maint/practracker/testdata/ex1-overbroad-expected.txt \ - scripts/maint/practracker/testdata/ex.txt \ - scripts/maint/practracker/testdata/header.h \ - scripts/maint/practracker/testdata/not_c_file \ - scripts/maint/practracker/test_practracker.sh \ - scripts/maint/practracker/util.py + ChangeLog \ + CONTRIBUTING \ + CODE_OF_CONDUCT \ + INSTALL \ + LICENSE \ + Makefile.nmake \ + README \ + ReleaseNotes \ + scripts/maint/checkIncludes.py \ + scripts/maint/checkSpace.pl \ + scripts/maint/checkShellScripts.sh \ + scripts/maint/practracker/README \ + scripts/maint/practracker/exceptions.txt \ + scripts/maint/practracker/includes.py \ + scripts/maint/practracker/metrics.py \ + scripts/maint/practracker/practracker.py \ + scripts/maint/practracker/practracker_tests.py \ + scripts/maint/practracker/problem.py \ + scripts/maint/practracker/testdata/.may_include \ + scripts/maint/practracker/testdata/a.c \ + scripts/maint/practracker/testdata/b.c \ + scripts/maint/practracker/testdata/ex0-expected.txt \ + scripts/maint/practracker/testdata/ex0.txt \ + scripts/maint/practracker/testdata/ex1-expected.txt \ + scripts/maint/practracker/testdata/ex1.txt \ + scripts/maint/practracker/testdata/ex1-overbroad-expected.txt \ + scripts/maint/practracker/testdata/ex.txt \ + scripts/maint/practracker/testdata/header.h \ + scripts/maint/practracker/testdata/not_c_file \ + scripts/maint/practracker/test_practracker.sh \ + scripts/maint/practracker/util.py \ + scripts/coccinelle/apply.sh \ + scripts/coccinelle/check_cocci_parse.sh \ + scripts/coccinelle/exceptions.txt \ + scripts/coccinelle/test-operator-cleanup \ + scripts/coccinelle/tor-coccinelle.h \ + scripts/coccinelle/try_parse.sh ## This tells etags how to find mockable function definitions. AM_ETAGSFLAGS=--regex='{c}/MOCK_IMPL([^,]+,\W*\([a-zA-Z0-9_]+\)\W*,/\1/s' @@ -246,7 +252,7 @@ test: all shellcheck: $(top_srcdir)/scripts/maint/checkShellScripts.sh -check-local: check-spaces check-changes check-includes check-best-practices shellcheck +check-local: check-spaces check-changes check-includes check-best-practices shellcheck check-cocci need-chutney-path: @if test ! -d "$$CHUTNEY_PATH"; then \ @@ -379,6 +385,9 @@ if USEPYTHON @$(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir) $(TOR_PRACTRACKER_OPTIONS) endif +check-cocci: + VERBOSE=1 $(top_srcdir)/scripts/coccinelle/check_cocci_parse.sh $(OWNED_TOR_C_FILES) + practracker-regen: $(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py --regen $(top_srcdir) diff --git a/changes/ticket31919 b/changes/ticket31919 new file mode 100644 index 0000000000..5344db956b --- /dev/null +++ b/changes/ticket31919 @@ -0,0 +1,13 @@ + o Minor features (developer tools): + - Add a check_cocci_parse.sh script that checks that new code + is parseable by Coccinelle. Add an exceptions file for unparseable + files. Closes ticket 31919. + - Call the check_cocci_parse.sh script from a 'check-cocci' Makefile + target. Closes ticket 31919. + o Minor features (git scripts): + - Call the check_cocci_parse.sh script from the git commit and push hooks. + Closes ticket 31919. + - Skip unmodified source files when doing some existing git hook checks. + Related to ticket 31919. + o Minor features (continuous integration): + - Call the check_cocci_parse.sh script from Travis CI. Closes ticket 31919. diff --git a/scripts/coccinelle/check_cocci_parse.sh b/scripts/coccinelle/check_cocci_parse.sh new file mode 100755 index 0000000000..5c27c7bc88 --- /dev/null +++ b/scripts/coccinelle/check_cocci_parse.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +# If we have coccinelle installed, run try_parse.sh on every filename passed +# as an argument. If no filenames are supplied, scan a standard Tor 0.3.5 or +# later directory layout. +# +# Uses the default coccinelle exceptions file, or $TOR_COCCI_EXCEPTIONS_FILE, +# if it is set. +# +# Use TOR_COCCI_EXCEPTIONS_FILE=/dev/null check_cocci_parse.sh to disable +# the default exception file. +# +# If spatch is not installed, remind the user to install it, but exit with +# a success error status. + +scripts_cocci="$(dirname "$0")" +top="$scripts_cocci/../.." +try_parse="$scripts_cocci/try_parse.sh" + +exitcode=0 + +export TOR_COCCI_EXCEPTIONS_FILE="${TOR_COCCI_EXCEPTIONS_FILE:-$scripts_cocci/exceptions.txt}" + +if ! command -v spatch; then + echo "Install coccinelle's spatch to check cocci C parsing!" + exit "$exitcode" +fi + +if test $# -ge 1 ; then + "$try_parse" "$@" + exitcode=$? +else + # This is the layout in 0.3.5 + "$try_parse" \ + src/lib/*/*.[ch] \ + src/core/*/*.[ch] \ + src/feature/*/*.[ch] \ + src/app/*/*.[ch] \ + src/test/*.[ch] \ + src/test/*/*.[ch] \ + src/tools/*.[ch] + exitcode=$? +fi + +if test "$exitcode" != 0 ; then + echo "Please fix these cocci parsing errors in the above files" + echo "Set VERBOSE=1 for more details" + echo "Try running test-operator-cleanup or 'make autostyle-operators'" + echo "As a last resort, you can modify scripts/coccinelle/exceptions.txt" +fi + +exit "$exitcode" diff --git a/scripts/coccinelle/exceptions.txt b/scripts/coccinelle/exceptions.txt new file mode 100644 index 0000000000..473f4b22c5 --- /dev/null +++ b/scripts/coccinelle/exceptions.txt @@ -0,0 +1,24 @@ +# A list of exception patterns for check_cocci_parse.sh +# Passed to 'grep -f' +src/lib/cc/compat_compiler.h +src/lib/container/handles.h +src/lib/container/map.c +src/lib/container/map.h +src/lib/container/order.c +src/lib/crypt_ops/crypto_rand.c +src/lib/fs/files.h +src/lib/log/util_bug.c +src/lib/pubsub/pubsub_macros.h +src/lib/smartlist_core/smartlist_foreach.h +src/lib/testsupport/testsupport.h +src/lib/tls/tortls.h +src/lib/tls/tortls_openssl.c +src/lib/tls/x509.h +src/lib/version/version.c +src/core/mainloop/connection.c +src/core/or/reasons.c +src/feature/dirclient/dirclient.c +src/feature/nodelist/networkstatus.c +src/test/test_address.c +src/test/test_hs_cache.c +src/test/test_hs_descriptor.c diff --git a/scripts/coccinelle/try_parse.sh b/scripts/coccinelle/try_parse.sh index 0f91e31702..a90e51b4aa 100755 --- a/scripts/coccinelle/try_parse.sh +++ b/scripts/coccinelle/try_parse.sh @@ -2,6 +2,11 @@ # Echo the name of every argument of this script that is not "perfect" # according to coccinelle's --parse-c. +# +# If $TOR_COCCI_EXCEPTIONS_FILE is non-empty, skip any files that match the +# patterns in the exception file, according to "grep -f" +# +# If VERBOSE is non-empty, log spatch errors and skipped files. top="$(dirname "$0")/../.." @@ -9,16 +14,28 @@ exitcode=0 for fn in "$@"; do - if spatch -macro_file_builtins "$top"/scripts/coccinelle/tor-coccinelle.h \ - -I "$top" -I "$top"/src -I "$top"/ext --defined COCCI \ + if test "${TOR_COCCI_EXCEPTIONS_FILE}" ; then + skip_fn=$(echo "$fn" | grep -f "${TOR_COCCI_EXCEPTIONS_FILE}") + if test "${skip_fn}" ; then + if test "${VERBOSE}" != ""; then + echo "Skipping '${skip_fn}'" + fi + continue + fi + fi + + if spatch --macro-file-builtins \ + "$top"/scripts/coccinelle/tor-coccinelle.h \ + --defined COCCI \ --parse-c "$fn" \ 2>/dev/null | grep "perfect = 1" > /dev/null; then : # it's perfect else echo "$fn" if test "${VERBOSE}" != ""; then - spatch -macro_file_builtins "$top"/scripts/coccinelle/tor-coccinelle.h \ - -I "$top" -I "$top"/src -I "$top"/ext --defined COCCI \ + spatch --macro-file-builtins \ + "$top"/scripts/coccinelle/tor-coccinelle.h \ + --defined COCCI \ --parse-c "$fn" fi exitcode=1 diff --git a/scripts/git/pre-commit.git-hook b/scripts/git/pre-commit.git-hook index 1c381ec60a..2e476c310e 100755 --- a/scripts/git/pre-commit.git-hook +++ b/scripts/git/pre-commit.git-hook @@ -13,30 +13,61 @@ cd "$workdir" || exit 1 set -e +if [ $# -eq 0 ]; then + # When called in pre-commit, check the files modified in this commit + CHECK_FILTER="git diff --cached --name-only --diff-filter=ACMR" + # Use the appropriate owned tor source list to filter the changed files + if [ -d src/lib ]; then + # This is the layout in 0.3.5 + CHECK_FILES="$($CHECK_FILTER \ + src/lib/*/*.[ch] \ + src/core/*/*.[ch] \ + src/feature/*/*.[ch] \ + src/app/*/*.[ch] \ + src/test/*.[ch] \ + src/test/*/*.[ch] \ + src/tools/*.[ch] \ + )" + elif [ -d src/common ]; then + # This was the layout before 0.3.5 + CHECK_FILES="$($CHECK_FILTER \ + src/common/*/*.[ch] \ + src/or/*/*.[ch] \ + src/test/*.[ch] \ + src/test/*/*.[ch] \ + src/tools/*.[ch] + )" + fi +else + # When called in pre-push, concatenate the argument array + # Fails on special characters in file names + CHECK_FILES="$*" +fi + +## General File Checks + if [ -n "$(ls ./changes/)" ]; then python scripts/maint/lintChanges.py ./changes/* fi -if [ -d src/lib ]; then - # This is the layout in 0.3.5 - perl scripts/maint/checkSpace.pl -C \ - src/lib/*/*.[ch] \ - src/core/*/*.[ch] \ - src/feature/*/*.[ch] \ - src/app/*/*.[ch] \ - src/test/*.[ch] \ - src/test/*/*.[ch] \ - src/tools/*.[ch] -elif [ -d src/common ]; then - # This was the layout before 0.3.5 - perl scripts/maint/checkSpace.pl -C \ - src/common/*/*.[ch] \ - src/or/*/*.[ch] \ - src/test/*.[ch] \ - src/test/*/*.[ch] \ - src/tools/*.[ch] +if [ -e scripts/maint/checkShellScripts.sh ]; then + scripts/maint/checkShellScripts.sh +fi + +if [ ! "$CHECK_FILES" ]; then + echo "No modified tor-owned source files, skipping further checks" + exit 0 fi +## Owned Source File Checks + +printf "Modified tor-owned source files:\n%s\n" "$CHECK_FILES" + +# We want word splitting here, because file names are space separated +# shellcheck disable=SC2086 +perl scripts/maint/checkSpace.pl -C \ + $CHECK_FILES + if test -e scripts/maint/practracker/includes.py; then python scripts/maint/practracker/includes.py fi @@ -54,6 +85,13 @@ if [ -e "${PT_DIR}/practracker.py" ]; then fi fi -if [ -e scripts/maint/checkShellScripts.sh ]; then - scripts/maint/checkShellScripts.sh +if [ -e scripts/coccinelle/check_cocci_parse.sh ]; then + + # Run a verbose cocci parse check on the changed files + # (spatch is slow, so we don't want to check all the files.) + # + # We want word splitting here, because file names are space separated + # shellcheck disable=SC2086 + VERBOSE=1 scripts/coccinelle/check_cocci_parse.sh \ + $CHECK_FILES fi diff --git a/scripts/git/pre-push.git-hook b/scripts/git/pre-push.git-hook index f4504c4215..2f3608029a 100755 --- a/scripts/git/pre-push.git-hook +++ b/scripts/git/pre-push.git-hook @@ -16,91 +16,110 @@ # The following sample script was used as starting point: # https://github.com/git/git/blob/master/templates/hooks--pre-push.sample +# Are you adding a new check to the git hooks? +# - Common checks belong in the pre-commit hook +# - Push-only checks belong in the pre-push hook + echo "Running pre-push hook" z40=0000000000000000000000000000000000000000 upstream_name=${TOR_UPSTREAM_REMOTE_NAME:-"upstream"} -# Are you adding a new check to the git hooks? -# - Common checks belong in the pre-commit hook -# - Push-only checks belong in the pre-push hook -# -# Call the pre-commit hook for the common checks, if it is executable. workdir=$(git rev-parse --show-toplevel) -if [ -x "$workdir/.git/hooks/pre-commit" ]; then - if ! "$workdir"/.git/hooks/pre-commit; then - exit 1 - fi -fi remote="$1" - remote_name=$(git remote --verbose | grep "$2" | awk '{print $1}' | head -n 1) -if [[ "$remote_name" != "$upstream_name" ]]; then - echo "Not pushing to upstream - refraining from further checks" - exit 0 -fi ref_is_upstream_branch() { - if [ "$1" == "refs/heads/master" ] || - [[ "$1" == refs/heads/release-* ]] || - [[ "$1" == refs/heads/maint-* ]] - then - return 1 - fi + if [ "$1" == "refs/heads/master" ] || + [[ "$1" == refs/heads/release-* ]] || + [[ "$1" == refs/heads/maint-* ]]; then + return 1 + fi } # shellcheck disable=SC2034 while read -r local_ref local_sha remote_ref remote_sha do - if [ "$local_sha" = $z40 ] - then - # Handle delete - : - else - if [ "$remote_sha" = $z40 ] - then - # New branch, examine all commits - range="$local_sha" - else - # Update to existing branch, examine new commits - range="$remote_sha..$local_sha" - fi - - if (ref_is_upstream_branch "$local_ref" == 0 || - ref_is_upstream_branch "$remote_ref" == 0) && - [ "$local_ref" != "$remote_ref" ] - then - if [ "$remote" == "origin" ] - then - echo >&2 "Not pushing: $local_ref to $remote_ref" - echo >&2 "If you really want to push this, use --no-verify." - exit 1 - else - continue - fi - fi - - # Check for fixup! commit - commit=$(git rev-list -n 1 --grep '^fixup!' "$range") - if [ -n "$commit" ] - then - echo >&2 "Found fixup! commit in $local_ref, not pushing" - echo >&2 "If you really want to push this, use --no-verify." - exit 1 - fi - - # Check for squash! commit - commit=$(git rev-list -n 1 --grep '^squash!' "$range") - if [ -n "$commit" ] - then - echo >&2 "Found squash! commit in $local_ref, not pushing" - echo >&2 "If you really want to push this, use --no-verify." - exit 1 - fi - fi + if [ "$local_sha" = $z40 ]; then + # Handle delete + : + else + if [ "$remote_sha" = $z40 ]; then + # New branch, examine commits not in master + range="master...$local_sha" + else + # Update to existing branch, examine new commits + range="$remote_sha..$local_sha" + fi + + # Call the pre-commit hook for the common checks, if it is executable + # Only check the files newly modified in this branch + CHECK_FILTER="git diff --name-only --diff-filter=ACMR $range" + # Use the appropriate owned tor source list to filter the changed files + if [ -d src/lib ]; then + # This is the layout in 0.3.5 + CHECK_FILES="$($CHECK_FILTER \ + src/lib/*/*.[ch] \ + src/core/*/*.[ch] \ + src/feature/*/*.[ch] \ + src/app/*/*.[ch] \ + src/test/*.[ch] \ + src/test/*/*.[ch] \ + src/tools/*.[ch] \ + )" + elif [ -d src/common ]; then + # This was the layout before 0.3.5 + CHECK_FILES="$($CHECK_FILTER \ + src/common/*/*.[ch] \ + src/or/*/*.[ch] \ + src/test/*.[ch] \ + src/test/*/*.[ch] \ + src/tools/*.[ch] + )" + fi + + # We want word splitting here, because file names are space separated + # shellcheck disable=SC2086 + if ! "$workdir/"scripts/git/pre-commit.git-hook $CHECK_FILES; then + exit 1 + fi + + if [[ "$remote_name" != "$upstream_name" ]]; then + echo "Not pushing to upstream - refraining from further checks" + continue + fi + + if (ref_is_upstream_branch "$local_ref" == 0 || + ref_is_upstream_branch "$remote_ref" == 0) && + [ "$local_ref" != "$remote_ref" ]; then + if [ "$remote" == "origin" ]; then + echo >&2 "Not pushing: $local_ref to $remote_ref" + echo >&2 "If you really want to push this, use --no-verify." + exit 1 + else + continue + fi + fi + + # Check for fixup! commit + commit=$(git rev-list -n 1 --grep '^fixup!' "$range") + if [ -n "$commit" ]; then + echo >&2 "Found fixup! commit in $local_ref, not pushing" + echo >&2 "If you really want to push this, use --no-verify." + exit 1 + fi + + # Check for squash! commit + commit=$(git rev-list -n 1 --grep '^squash!' "$range") + if [ -n "$commit" ]; then + echo >&2 "Found squash! commit in $local_ref, not pushing" + echo >&2 "If you really want to push this, use --no-verify." + exit 1 + fi + fi done exit 0 |