HADOOP-11866. increase readability and reliability of checkstyle, shellcheck, and whitespace reports (aw)

dev-support/test-patch.d/checkstyle.sh

@@ -29,8 +29,39 @@ function checkstyle_filefilter
   fi
 }
 
+function checkstyle_mvnrunner
+{
+  local logfile=$1
+  local output=$2
+  local tmp=${PATCH_DIR}/$$.${RANDOM}
+  local j
+
+  "${MVN}" clean test checkstyle:checkstyle -DskipTests \
+    -Dcheckstyle.consoleOutput=true \
+    "-D${PROJECT_NAME}PatchProcess" 2>&1 \
+      | tee "${logfile}" \
+      | ${GREP} ^/ \
+      | ${SED} -e "s,${BASEDIR},.,g" \
+          > "${tmp}"
+
+  # the checkstyle output files are massive, so
+  # let's reduce the work by filtering out files
+  # that weren't changed.  Some modules are
+  # MASSIVE and this can cut the output down to
+  # by orders of magnitude!!
+  for j in ${CHANGED_FILES}; do
+    ${GREP} "${j}" "${tmp}" >> "${output}"
+  done
+
+  rm "${tmp}" 2>/dev/null
+}
+
 function checkstyle_preapply
 {
+  local module_suffix
+  local modules=${CHANGED_MODULES}
+  local module
+
   verify_needed_test checkstyle
 
   if [[ $? == 0 ]]; then
@@ -40,23 +71,78 @@ function checkstyle_preapply
   big_console_header "checkstyle plugin: prepatch"
 
   start_clock
-  echo_and_redirect "${PATCH_DIR}/${PATCH_BRANCH}checkstyle.txt" "${MVN}" test checkstyle:checkstyle-aggregate -DskipTests "-D${PROJECT_NAME}PatchProcess"
-  if [[ $? != 0 ]] ; then
-    echo "Pre-patch ${PATCH_BRANCH} checkstyle compilation is broken?"
-    add_jira_table -1 checkstyle "Pre-patch ${PATCH_BRANCH} checkstyle compilation may be broken."
-    return 1
-  fi
 
-  cp -p "${BASEDIR}/target/checkstyle-result.xml" \
-    "${PATCH_DIR}/checkstyle-result-${PATCH_BRANCH}.xml"
+  for module in ${modules}
+  do
+    pushd "${module}" >/dev/null
+    echo "  Running checkstyle in ${module}"
+    module_suffix=$(basename "${module}")
+
+    checkstyle_mvnrunner \
+      "${PATCH_DIR}/maven-${PATCH_BRANCH}checkstyle-${module_suffix}.txt" \
+      "${PATCH_DIR}/${PATCH_BRANCH}checkstyle${module_suffix}.txt"
+
+    if [[ $? != 0 ]] ; then
+      echo "Pre-patch ${PATCH_BRANCH} checkstyle compilation is broken?"
+      add_jira_table -1 checkstyle "Pre-patch ${PATCH_BRANCH} ${module} checkstyle compilation may be broken."
+    fi
+    popd >/dev/null
+  done
 
   # keep track of how much as elapsed for us already
   CHECKSTYLE_TIMER=$(stop_clock)
   return 0
 }
 
+function checkstyle_calcdiffs
+{
+  local orig=$1
+  local new=$2
+  local diffout=$3
+  local tmp=${PATCH_DIR}/cs.$$.${RANDOM}
+  local count=0
+  local j
+
+  # first, pull out just the errors
+  # shellcheck disable=SC2016
+  ${AWK} -F: '{print $NF}' "${orig}" >> "${tmp}.branch"
+
+  # shellcheck disable=SC2016
+  ${AWK} -F: '{print $NF}' "${new}" >> "${tmp}.patch"
+
+  # compare the errors, generating a string of line
+  # numbers.  Sorry portability: GNU diff makes this too easy
+  ${DIFF} --unchanged-line-format="" \
+     --old-line-format="" \
+     --new-line-format="%dn " \
+     "${tmp}.branch" \
+     "${tmp}.patch" > "${tmp}.lined"
+
+  # now, pull out those lines of the raw output
+  # shellcheck disable=SC2013
+  for j in $(cat "${tmp}.lined"); do
+    # shellcheck disable=SC2086
+    head -${j} "${new}" | tail -1 >> "${diffout}"
+  done
+
+  if [[ -f "${diffout}" ]]; then
+    # shellcheck disable=SC2016
+    count=$(wc -l "${diffout}" | ${AWK} '{print $1}' )
+  fi
+  rm "${tmp}.branch" "${tmp}.patch" "${tmp}.lined" 2>/dev/null
+  echo "${count}"
+}
+
 function checkstyle_postapply
 {
+  local rc=0
+  local module
+  local modules=${CHANGED_MODULES}
+  local module_suffix
+  local numprepatch=0
+  local numpostpatch=0
+  local diffpostpatch=0
+
   verify_needed_test checkstyle
 
   if [[ $? == 0 ]]; then
@@ -71,79 +157,49 @@ function checkstyle_postapply
   # by setting the clock back
   offset_clock "${CHECKSTYLE_TIMER}"
 
-  echo_and_redirect "${PATCH_DIR}/patchcheckstyle.txt" "${MVN}" test checkstyle:checkstyle-aggregate -DskipTests "-D${PROJECT_NAME}PatchProcess"
-  if [[ $? != 0 ]] ; then
-    echo "Post-patch checkstyle compilation is broken."
-    add_jira_table -1 checkstyle "Post-patch checkstyle compilation is broken."
-    return 1
-  fi
-
-  cp -p "${BASEDIR}/target/checkstyle-result.xml" \
-    "${PATCH_DIR}/checkstyle-result-patch.xml"
-
-  checkstyle_runcomparison
-
-  # shellcheck disable=SC2016
-  CHECKSTYLE_POSTPATCH=$(wc -l "${PATCH_DIR}/checkstyle-result-diff.txt" | ${AWK} '{print $1}')
-
-  if [[ ${CHECKSTYLE_POSTPATCH} -gt 0 ]] ; then
-
-    add_jira_table -1 checkstyle "The applied patch generated "\
-      "${CHECKSTYLE_POSTPATCH}" \
-      " additional checkstyle issues."
-    add_jira_footer checkstyle "@@BASE@@/checkstyle-result-diff.txt"
-
+  for module in ${modules}
+  do
+    pushd "${module}" >/dev/null
+    echo "  Running checkstyle in ${module}"
+    module_suffix=$(basename "${module}")
+
+    checkstyle_mvnrunner \
+      "${PATCH_DIR}/maven-patchcheckstyle-${module_suffix}.txt" \
+      "${PATCH_DIR}/patchcheckstyle${module_suffix}.txt"
+
+    if [[ $? != 0 ]] ; then
+      ((rc = rc +1))
+      echo "Post-patch checkstyle compilation is broken."
+      add_jira_table -1 checkstyle "Post-patch checkstyle ${module} compilation is broken."
+      continue
+    fi
+
+    #shellcheck disable=SC2016
+    diffpostpatch=$(checkstyle_calcdiffs \
+      "${PATCH_DIR}/${PATCH_BRANCH}checkstyle${module_suffix}.txt" \
+      "${PATCH_DIR}/patchcheckstyle${module_suffix}.txt" \
+      "${PATCH_DIR}/diffcheckstyle${module_suffix}.txt" )
+
+    if [[ ${diffpostpatch} -gt 0 ]] ; then
+      ((rc = rc + 1))
+
+      # shellcheck disable=SC2016
+      numprepatch=$(wc -l "${PATCH_DIR}/${PATCH_BRANCH}checkstyle${module_suffix}.txt" | ${AWK} '{print $1}')
+      # shellcheck disable=SC2016
+      numpostpatch=$(wc -l "${PATCH_DIR}/patchcheckstyle${module_suffix}.txt" | ${AWK} '{print $1}')
+
+      add_jira_table -1 checkstyle "The applied patch generated "\
+        "${diffpostpatch} new checkstyle issues (total was ${numprepatch}, now ${numpostpatch})."
+      footer="${footer} @@BASE@@/diffcheckstyle${module_suffix}.txt"
+    fi
+
+    popd >/dev/null
+  done
+
+  if [[ ${rc} -gt 0 ]] ; then
+    add_jira_footer checkstyle "${footer}"
     return 1
   fi
   add_jira_table +1 checkstyle "There were no new checkstyle issues."
   return 0
-}
-
-
-function checkstyle_runcomparison
-{
-
-  python <(cat <<EOF
-import os
-import sys
-import xml.etree.ElementTree as etree
-from collections import defaultdict
-
-if len(sys.argv) != 3 :
-  print "usage: %s checkstyle-result-master.xml checkstyle-result-patch.xml" % sys.argv[0]
-  exit(1)
-
-def path_key(x):
-  path = x.attrib['name']
-  return path[path.find('${PROJECT_NAME}-'):]
-
-def print_row(path, master_errors, patch_errors):
-    print '%s\t%s\t%s' % (k,master_dict[k],child_errors)
-
-master = etree.parse(sys.argv[1])
-patch = etree.parse(sys.argv[2])
-
-master_dict = defaultdict(int)
-
-for child in master.getroot().getchildren():
-    if child.tag != 'file':
-        continue
-    child_errors = len(child.getchildren())
-    if child_errors == 0:
-        continue
-    master_dict[path_key(child)] = child_errors
-
-for child in patch.getroot().getchildren():
-    if child.tag != 'file':
-        continue
-    child_errors = len(child.getchildren())
-    if child_errors == 0:
-        continue
-    k = path_key(child)
-    if child_errors > master_dict[k]:
-        print_row(k, master_dict[k], child_errors)
-
-EOF
-) "${PATCH_DIR}/checkstyle-result-${PATCH_BRANCH}.xml" "${PATCH_DIR}/checkstyle-result-patch.xml" > "${PATCH_DIR}/checkstyle-result-diff.txt"
-
-}
+}

dev-support/test-patch.d/shellcheck.sh

@@ -87,6 +87,45 @@ function shellcheck_preapply
   return 0
 }
 
+function shellcheck_calcdiffs
+{
+  local orig=$1
+  local new=$2
+  local diffout=$3
+  local tmp=${PATCH_DIR}/sc.$$.${RANDOM}
+  local count=0
+  local j
+
+  # first, pull out just the errors
+  # shellcheck disable=SC2016
+  ${AWK} -F: '{print $NF}' "${orig}" >> "${tmp}.branch"
+
+  # shellcheck disable=SC2016
+  ${AWK} -F: '{print $NF}' "${new}" >> "${tmp}.patch"
+
+  # compare the errors, generating a string of line
+  # numbers.  Sorry portability: GNU diff makes this too easy
+  ${DIFF} --unchanged-line-format="" \
+     --old-line-format="" \
+     --new-line-format="%dn " \
+     "${tmp}.branch" \
+     "${tmp}.patch" > "${tmp}.lined"
+
+  # now, pull out those lines of the raw output
+  # shellcheck disable=SC2013
+  for j in $(cat "${tmp}.lined"); do
+    # shellcheck disable=SC2086
+    head -${j} "${new}" | tail -1 >> "${diffout}"
+  done
+
+  if [[ -f "${diffout}" ]]; then
+    # shellcheck disable=SC2016
+    count=$(wc -l "${diffout}" | ${AWK} '{print $1}' )
+  fi
+  rm "${tmp}.branch" "${tmp}.patch" "${tmp}.lined" 2>/dev/null
+  echo "${count}"
+}
+
 function shellcheck_postapply
 {
   local i
@@ -121,16 +160,13 @@ function shellcheck_postapply
   # shellcheck disable=SC2016
   numPostpatch=$(wc -l "${PATCH_DIR}/patchshellcheck-result.txt" | ${AWK} '{print $1}')
 
-  ${DIFF} -u "${PATCH_DIR}/${PATCH_BRANCH}shellcheck-result.txt" \
+  diffPostpatch=$(shellcheck_calcdiffs \
+    "${PATCH_DIR}/${PATCH_BRANCH}shellcheck-result.txt" \
     "${PATCH_DIR}/patchshellcheck-result.txt" \
-      | ${GREP} '^+\.' \
-      > "${PATCH_DIR}/diffpatchshellcheck.txt"
-
-  # shellcheck disable=SC2016
-  diffPostpatch=$(wc -l "${PATCH_DIR}/diffpatchshellcheck.txt" | ${AWK} '{print $1}')
+      "${PATCH_DIR}/diffpatchshellcheck.txt"
+    )
 
-  if [[ ${diffPostpatch} -gt 0
-    && ${numPostpatch} -gt ${numPrepatch} ]] ; then
+  if [[ ${diffPostpatch} -gt 0 ]] ; then
     add_jira_table -1 shellcheck "The applied patch generated "\
       "${diffPostpatch} new shellcheck (v${SHELLCHECK_VERSION}) issues (total was ${numPrepatch}, now ${numPostpatch})."
     add_jira_footer shellcheck "@@BASE@@/diffpatchshellcheck.txt"

dev-support/test-patch.d/whitespace.sh

@@ -16,25 +16,31 @@
 
 add_plugin whitespace
 
-function whitespace_preapply
+function whitespace_postapply
 {
   local count
+  local j
 
   big_console_header "Checking for whitespace at the end of lines"
   start_clock
 
-  ${GREP} '^+' "${PATCH_DIR}/patch" | ${GREP} '[[:blank:]]$' > "${PATCH_DIR}/whitespace.txt"
+  pushd "${BASEDIR}" >/dev/null
+  for j in ${CHANGED_FILES}; do
+    ${GREP} -nHE '[[:blank:]]$' "./${j}" | ${GREP} -f "${GITDIFFLINES}" >> "${PATCH_DIR}/whitespace.txt"
+  done
 
   # shellcheck disable=SC2016
   count=$(wc -l "${PATCH_DIR}/whitespace.txt" | ${AWK} '{print $1}')
 
   if [[ ${count} -gt 0 ]]; then
     add_jira_table -1 whitespace "The patch has ${count}"\
-      " line(s) that end in whitespace."
+      " line(s) that end in whitespace. Use git apply --whitespace=fix."
     add_jira_footer whitespace "@@BASE@@/whitespace.txt"
+    popd >/dev/null
     return 1
   fi
 
+  popd >/dev/null
   add_jira_table +1 whitespace "The patch has no lines that end in whitespace."
   return 0
 }

dev-support/test-patch.sh

@@ -67,7 +67,7 @@ function setup_defaults
       EGREP=${EGREP:-/usr/xpg4/bin/egrep}
       GREP=${GREP:-/usr/xpg4/bin/grep}
       PATCH=${PATCH:-patch}
-      DIFF=${DIFF:-diff}
+      DIFF=${DIFF:-/usr/gnu/bin/diff}
       JIRACLI=${JIRA:-jira}
     ;;
     *)
@@ -449,7 +449,7 @@ function verify_patchdir_still_exists
   if [[ ! -d ${PATCH_DIR} ]]; then
       rm "${commentfile}" 2>/dev/null
 
-      echo "(!) The patch artifact directory on has been removed! " > "${commentfile}"
+      echo "(!) The patch artifact directory has been removed! " > "${commentfile}"
       echo "This is a fatal error for test-patch.sh.  Aborting. " >> "${commentfile}"
       echo
       cat ${commentfile}
@@ -468,6 +468,49 @@ function verify_patchdir_still_exists
     fi
 }
 
+## @description generate a list of all files and line numbers that
+## @description that were added/changed in the source repo
+## @audience    private
+## @stability   stable
+## @params      filename
+## @replaceable no
+function compute_gitdiff
+{
+  local outfile=$1
+  local file
+  local line
+  local startline
+  local counter
+  local numlines
+  local actual
+
+  pushd "${BASEDIR}" >/dev/null
+  while read line; do
+    if [[ ${line} =~ ^\+\+\+ ]]; then
+      file="./"$(echo "${line}" | cut -f2- -d/)
+      continue
+    elif [[ ${line} =~ ^@@ ]]; then
+      startline=$(echo "${line}" | cut -f3 -d' ' | cut -f1 -d, | tr -d + )
+      numlines=$(echo "${line}" | cut -f3 -d' ' | cut -s -f2 -d, )
+      # if this is empty, then just this line
+      # if it is 0, then no lines were added and this part of the patch
+      # is strictly a delete
+      if [[ ${numlines} == 0 ]]; then
+        continue
+      elif [[ -z ${numlines} ]]; then
+        numlines=1
+      fi
+      counter=0
+      until [[ ${counter} -gt ${numlines} ]]; do
+          ((actual=counter+startline))
+          echo "${file}:${actual}:" >> "${outfile}"
+          ((counter=counter+1))
+      done
+    fi
+  done < <("${GIT}" diff --unified=0 --no-color)
+  popd >/dev/null
+}
+
 ## @description  Print the command to be executing to the screen. Then
 ## @description  run the command, sending stdout and stderr to the given filename
 ## @description  This will also ensure that any directories in ${BASEDIR} have
@@ -481,7 +524,7 @@ function verify_patchdir_still_exists
 ## @returns      $?
 function echo_and_redirect
 {
-  logfile=$1
+  local logfile=$1
   shift
 
   verify_patchdir_still_exists
@@ -522,7 +565,7 @@ function hadoop_usage
 
   echo "Shell binary overrides:"
   echo "--awk-cmd=<cmd>        The 'awk' command to use (default 'awk')"
-  echo "--diff-cmd=<cmd>       The 'diff' command to use (default 'diff')"
+  echo "--diff-cmd=<cmd>       The GNU-compatible 'diff' command to use (default 'diff')"
   echo "--git-cmd=<cmd>        The 'git' command to use (default 'git')"
   echo "--grep-cmd=<cmd>       The 'grep' command to use (default 'grep')"
   echo "--mvn-cmd=<cmd>        The 'mvn' command to use (default \${MAVEN_HOME}/bin/mvn, or 'mvn')"
@@ -585,6 +628,10 @@ function parse_args
       --grep-cmd=*)
         GREP=${i#*=}
       ;;
+      --help|-help|-h|help|--h|--\?|-\?|\?)
+        hadoop_usage
+        exit 0
+      ;;
       --java-home)
         JAVA_HOME=${i#*=}
       ;;
@@ -680,6 +727,8 @@ function parse_args
       cleanup_and_exit 1
     fi
   fi
+
+  GITDIFFLINES=${PATCH_DIR}/gitdifflines.txt
 }
 
 ## @description  Locate the pom.xml file for a given directory
@@ -716,12 +765,14 @@ function find_changed_files
   # get a list of all of the files that have been changed,
   # except for /dev/null (which would be present for new files).
   # Additionally, remove any a/ b/ patterns at the front
-  # of the patch filenames
+  # of the patch filenames and any revision info at the end
+  # shellcheck disable=SC2016
   CHANGED_FILES=$(${GREP} -E '^(\+\+\+|---) ' "${PATCH_DIR}/patch" \
     | ${SED} \
       -e 's,^....,,' \
       -e 's,^[ab]/,,' \
     | ${GREP} -v /dev/null \
+    | ${AWK} '{print $1}' \
     | sort -u)
 }
 
@@ -1552,7 +1603,7 @@ function check_javac
         > "${PATCH_DIR}/diffJavacWarnings.txt"
 
         add_jira_table -1 javac "The applied patch generated "\
-        "$((patchJavacWarnings-branchJavacWarnings))" \
+        "$((patchJavacWarnings-${PATCH_BRANCH}JavacWarnings))" \
         " additional warning messages."
 
         add_jira_footer javac "@@BASE@@/diffJavacWarnings.txt"
@@ -1712,6 +1763,7 @@ function check_findbugs
       "${PATCH_DIR}/patchFindbugsWarnings${module_suffix}.xml" \
       "${PATCH_DIR}/patchFindbugsWarnings${module_suffix}.xml"
 
+    #shellcheck disable=SC2016
     newFindbugsWarnings=$("${FINDBUGS_HOME}/bin/filterBugs" \
       -first "01/01/2000" "${PATCH_DIR}/patchFindbugsWarnings${module_suffix}.xml" \
       "${PATCH_DIR}/newPatchFindbugsWarnings${module_suffix}.xml" \
@@ -1887,10 +1939,12 @@ function check_unittests
       test_timeouts="${test_timeouts} ${module_test_timeouts}"
       result=1
     fi
-    #shellcheck disable=SC2026,SC2038
+
+    #shellcheck disable=SC2026,SC2038,SC2016
     module_failed_tests=$(find . -name 'TEST*.xml'\
       | xargs "${GREP}" -l -E "<failure|<error"\
       | ${AWK} -F/ '{sub("TEST-org.apache.",""); sub(".xml",""); print $NF}')
+
     if [[ -n "${module_failed_tests}" ]] ; then
       failed_tests="${failed_tests} ${module_failed_tests}"
       result=1
@@ -2054,8 +2108,6 @@ function output_to_console
     printf "%s\n" "${comment}"
     ((i=i+1))
   done
-
-
 }
 
 ## @description  Print out the finished details to the JIRA issue
@@ -2189,7 +2241,6 @@ function postcheckout
       #shellcheck disable=SC2086
       ${plugin}_postcheckout
 
-
       (( RESULT = RESULT + $? ))
       if [[ ${RESULT} != 0 ]] ; then
         output_to_console 1
@@ -2244,6 +2295,8 @@ function postapply
   local plugin
   local retval
 
+  compute_gitdiff "${GITDIFFLINES}"
+
   check_javac
   retval=$?
   if [[ ${retval} -gt 1 ]] ; then

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -582,6 +582,9 @@ Release 2.8.0 - UNRELEASED
     HADOOP-11821. Fix findbugs warnings in hadoop-sls.
     (Brahma Reddy Battula via aajisaka)
 
+    HADOOP-11866. increase readability and reliability of checkstyle,
+    shellcheck, and whitespace reports (aw)
+
 Release 2.7.1 - UNRELEASED
 
   INCOMPATIBLE CHANGES