From 4195b1086098fb0bc685f40d2c4de6737148ad69 Mon Sep 17 00:00:00 2001 From: Allen Wittenauer Date: Thu, 30 Apr 2015 15:16:42 -0700 Subject: [PATCH] HADOOP-11866. increase readability and reliability of checkstyle, shellcheck, and whitespace reports (aw) --- dev-support/test-patch.d/checkstyle.sh | 204 +++++++++++------- dev-support/test-patch.d/shellcheck.sh | 52 ++++- dev-support/test-patch.d/whitespace.sh | 12 +- dev-support/test-patch.sh | 73 ++++++- .../hadoop-common/CHANGES.txt | 3 + 5 files changed, 249 insertions(+), 95 deletions(-) diff --git a/dev-support/test-patch.d/checkstyle.sh b/dev-support/test-patch.d/checkstyle.sh index 460709e3111..63115842b39 100755 --- a/dev-support/test-patch.d/checkstyle.sh +++ b/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 + for module in ${modules} + do + pushd "${module}" >/dev/null + echo " Running checkstyle in ${module}" + module_suffix=$(basename "${module}") - cp -p "${BASEDIR}/target/checkstyle-result.xml" \ - "${PATCH_DIR}/checkstyle-result-patch.xml" + checkstyle_mvnrunner \ + "${PATCH_DIR}/maven-patchcheckstyle-${module_suffix}.txt" \ + "${PATCH_DIR}/patchcheckstyle${module_suffix}.txt" - checkstyle_runcomparison + 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 - CHECKSTYLE_POSTPATCH=$(wc -l "${PATCH_DIR}/checkstyle-result-diff.txt" | ${AWK} '{print $1}') + #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 [[ ${CHECKSTYLE_POSTPATCH} -gt 0 ]] ; then + if [[ ${diffpostpatch} -gt 0 ]] ; then + ((rc = rc + 1)) - add_jira_table -1 checkstyle "The applied patch generated "\ - "${CHECKSTYLE_POSTPATCH}" \ - " additional checkstyle issues." - add_jira_footer checkstyle "@@BASE@@/checkstyle-result-diff.txt" + # 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 < 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" - -} +} \ No newline at end of file diff --git a/dev-support/test-patch.d/shellcheck.sh b/dev-support/test-patch.d/shellcheck.sh index c1084a2e5bf..5f38b6a2582 100755 --- a/dev-support/test-patch.d/shellcheck.sh +++ b/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" + "${PATCH_DIR}/diffpatchshellcheck.txt" + ) - # shellcheck disable=SC2016 - diffPostpatch=$(wc -l "${PATCH_DIR}/diffpatchshellcheck.txt" | ${AWK} '{print $1}') - - 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" diff --git a/dev-support/test-patch.d/whitespace.sh b/dev-support/test-patch.d/whitespace.sh index deac654da8b..324481ca74d 100755 --- a/dev-support/test-patch.d/whitespace.sh +++ b/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 } diff --git a/dev-support/test-patch.sh b/dev-support/test-patch.sh index ae218372676..b6e1b03b212 100755 --- a/dev-support/test-patch.sh +++ b/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= The 'awk' command to use (default 'awk')" - echo "--diff-cmd= The 'diff' command to use (default 'diff')" + echo "--diff-cmd= The GNU-compatible 'diff' command to use (default 'diff')" echo "--git-cmd= The 'git' command to use (default 'git')" echo "--grep-cmd= The 'grep' command to use (default 'grep')" echo "--mvn-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 "