Attach diffs produced by git-clang-format as lint messages.

E.g.: https://reviews.llvm.org/D71197?id=233029 Will add a link to file and code to apply the patch in the next PR. + don't create TARGET_DIR in scripts; + updated section about local build; + partially specified inputs / outputs of scripts so it's more transparent what are the results; + added symlink to compile_command.json (clang-tidy will need it); + add IDEA files to .gitignore.
2019-12-09 12:09:53 +01:00 · 2019-12-09 12:09:53 +01:00 · 7faaec98e7
commit 7faaec98e7
parent 650ed9c274
11 changed files with 168 additions and 43 deletions
--- a/.gitignore
+++ b/.gitignore
@ -1 +1,3 @@
 .vscode/
+.idea/
+*.iml
--- a/Jenkins/Phabricator-pipeline/Jenkinsfile
+++ b/Jenkins/Phabricator-pipeline/Jenkinsfile
@ -23,11 +23,12 @@ pipeline {
            PHABRICATOR_HOST = 'https://reviews.llvm.org'
            PHAB_LOG = "${WORKSPACE}/build/.phabricator-comment"
            SCRIPT_DIR = "${WORKSPACE}/llvm-premerge-checks/scripts"
-            MY_BUILD_ID = "${JOB_BASE_NAME}-${BUILD_NUMBER}"
-            TARGET_DIR = "/mnt/nfs/results/${MY_BUILD_ID}"
-            RESULT_URL = "http://results.llvm-merge-guard.org/${MY_BUILD_ID}"
+            BUILD_ID = "${JOB_BASE_NAME}-${BUILD_NUMBER}"
+            TARGET_DIR = "/mnt/nfs/results/${BUILD_ID}"
+            RESULT_URL = "http://results.llvm-merge-guard.org/${BUILD_ID}"
            TEST_REPORT = "${WORKSPACE}/build/test-results.xml"
            DIFF_JSON = "${WORKSPACE}/build/diff.json"
+            TARGET_DIR="/mnt/nfs/results/${JOB_BASE_NAME}-${BUILD_NUMBER}"
    }
    stages {
        stage("build info"){
@ -44,7 +45,9 @@ pipeline {
                {
                    git url: 'https://github.com/google/llvm-premerge-checks.git'
                }
+                sh 'rm -rf build || true'
                sh 'mkdir -p build'
+                sh 'mkdir -p "${TARGET_DIR}'
            }
        }
        stage('arc patch'){
@ -67,6 +70,11 @@ pipeline {
                sh "${SCRIPT_DIR}/run_ninja.sh check-all"
            }
        }
+        stage('linters') {
+            steps {
+                sh "${SCRIPT_DIR}/lint.sh"
+            }
+        }
    }
    post { 
        always { 
@ -84,6 +92,7 @@ pipeline {
                """
            }
            /// send results to Phabricator
+            // TODO: list all files from results directory instead
            sh """
 set +x
 cat <<-EOF>> ${PHAB_LOG}
@ -96,7 +105,8 @@ EOF
                --test-result-file "${TEST_REPORT}" \
                --comment-file "${PHAB_LOG}" \
                --host "${PHABRICATOR_HOST}/api/" \
-                --buildresult ${currentBuild.result}
+                --buildresult ${currentBuild.result} \
+                ---clang-format-patch="${TARGET_DIR}/clang-format.patch"
                """
        }
    }    
--- a/Jenkins/master-linux-pipeline/Jenkinsfile
+++ b/Jenkins/master-linux-pipeline/Jenkinsfile
@ -21,7 +21,6 @@ pipeline {
        BUILD_ID="${JOB_BASE_NAME}-${BUILD_NUMBER}"
        TARGET_DIR="/mnt/nfs/results/${BUILD_ID}"
        SCRIPT_DIR = "${WORKSPACE}/llvm-premerge-checks/scripts"
-
    }
    stages {
        stage("git checkout"){
@ -29,6 +28,7 @@ pipeline {
                git url: 'https://github.com/llvm/llvm-project.git'
                sh 'git clean -fdx'
                sh 'mkdir -p llvm-premerge-checks'
+                sh 'mkdir -p "${TARGET_DIR}"'
                dir("llvm-premerge-checks")
                {
                    git url: 'https://github.com/google/llvm-premerge-checks.git'
--- a/Jenkins/master-windows-pipeline/Jenkinsfile
+++ b/Jenkins/master-windows-pipeline/Jenkinsfile
@ -21,7 +21,6 @@ pipeline {
        BUILD_ID="${JOB_BASE_NAME}-${BUILD_NUMBER}"
        TARGET_DIR="/mnt/nfs/results/${BUILD_ID}"
        SCRIPT_DIR = "${WORKSPACE}/llvm-premerge-checks/scripts"
-
    }
    stages {
        stage("git checkout"){
--- a/containers/build_run.sh
+++ b/containers/build_run.sh
@ -13,6 +13,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

+# Starts a new instances of a docker image. Example:
+# sudo build_run.sh agent-debian-testing-clang8-ssd /bin/bash
+
 set -eux
 DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"

--- a/docs/playbooks.md
+++ b/docs/playbooks.md
@ -108,5 +108,8 @@ Invoke-WebRequest -uri 'https://raw.githubusercontent.com/google/llvm-premerge-c

 ## Testing scripts locally

-To experiment with a build scripts locally you will need a Conduit token. You can create one at
-https://reviews.llvm.org/settings/user/<USERNAME>/page/apitokens/ .
+Build and run agent docker image `sudo build_run.sh agent-debian-testing-clang8-ssd /bin/bash`.
+
+Within a container set environment variables similar to [pipeline](https://github.com/google/llvm-premerge-checks/blob/master/Jenkins/Phabricator-pipeline/Jenkinsfile).
+
+Additionally set `WORKSPACE`, `PHID` and `DIFF_ID` parameters. Set `CONDUIT_TOKEN` with your personal one from `https://reviews.llvm.org/settings/user/<USERNAME>/page/apitokens/`.
--- a/scripts/lint.sh
+++ b/scripts/lint.sh
@ -0,0 +1,37 @@
+#!/bin/bash
+# Copyright 2019 Google LLC
+#
+# Licensed under the the Apache License v2.0 with LLVM Exceptions (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     https://llvm.org/LICENSE.txt
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Runs clang-format
+# Inputs: TARGET_DIR, WORKSPACE
+# Outputs: ${TARGET_DIR}/clang-format.patch (if there are clang-format findings).
+set -eux
+
+echo "Running linters... ====================================="
+
+cd "${WORKSPACE}"
+# Let clang format apply patches --diff doesn't produces results in the format
+# we want.
+python3 /usr/bin/git-clang-format-8 --style=llvm --binary=/usr/bin/clang-format-8
+set +e
+git diff -U0 --exit-code > "${TARGET_DIR}"/clang-format.patch
+STATUS="${PIPESTATUS[0]}"
+set -e
+# Drop file if there are no findings.
+if [[ $STATUS == 0 ]]; then rm "${TARGET_DIR}"/clang-format.patch; fi
+
+# Revert changes of git-clang-format.
+git checkout -- .
+
+echo "linters completed ======================================"
--- a/scripts/phabtalk/README.md
+++ b/scripts/phabtalk/README.md
@ -1,2 +1,4 @@
-This folder contains Python scripts that to Phabricator.
+This folder contains Python scripts that talk to Phabricator.
+
 They require a few libraries listed in `requirements.txt`.
+To install the requirements locally run `pip3 install -r requirements.txt`.
--- a/scripts/phabtalk/phabtalk.py
+++ b/scripts/phabtalk/phabtalk.py
@ -19,17 +19,21 @@ build status, a summary and the test reults to Phabricator."""

 import argparse
 import os
+import re
+import socket
 import time
 from typing import Optional
-from phabricator import Phabricator
-import socket
+
 from lxml import etree
+from phabricator import Phabricator
+

 class TestResults:

    def __init__(self):
        self.result_type = None  # type: str
        self.unit = []  #type: List
+        self.lint = []
        self.test_stats = {
            'pass':0,
            'fail':0, 
@ -38,7 +42,9 @@ class TestResults:


 class PhabTalk:
-    """Talk to Phabricator to upload build results."""
+    """Talk to Phabricator to upload build results.
+       See https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/
+    """

    def __init__(self, token: Optional[str], host: Optional[str], dryrun: bool):
        self._phab = None  # type: Optional[Phabricator]
@ -128,15 +134,18 @@ class PhabTalk:
            print('harbormaster.sendmessage =================')
            print('type: {}'.format(result))
            print('unit: {}'.format(test_results.unit))
+            print('lint: {}'.format(test_results.lint))
            return

        # API details at
        # https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/  
-        self._phab.harbormaster.sendmessage(buildTargetPHID=phid, 
+        self._phab.harbormaster.sendmessage(
+            buildTargetPHID=phid,
            type=result, 
-            unit=test_results.unit)
+            unit=test_results.unit,
+            lint=test_results.lint)

-    def _compute_test_results(self, build_result_file: str) -> TestResults:
+    def _compute_test_results(self, build_result_file: str, clang_format_patch: str) -> TestResults:
        result = TestResults()

        if build_result_file is None:
@ -166,8 +175,63 @@ class PhabTalk:
                }
                result.result_type = 'fail'
                result.unit.append(test_result)
+        if os.path.exists(clang_format_patch):
+            diffs = self.parse_patch(open(clang_format_patch, 'rt'))
+            for d in diffs:
+                lint_message = {
+                    'name': 'Please fix the formatting',
+                    'severity': 'autofix',
+                    'code': 'clang-format',
+                    'path': d['filename'],
+                    'line': d['line'],
+                    'char': 1,
+                    'description': '```\n' + d['diff'] + '\n```',
+                }
+                result.lint.append(lint_message)
        return result

+    def parse_patch(self, patch) -> []:
+        """Extract the changed lines from `patch` file.
+        The return value is a list of dictionaries {filename, line, diff}.
+        Diff must be generated with -U0 (no context lines).
+        """
+        entries = []
+        lines = []
+        filename = None
+        line_number = 0
+        for line in patch:
+            match = re.search(r'^(\+\+\+|---) [^/]+/(.*)', line)
+            if match:
+                if len(lines) > 0:
+                    entries.append({
+                        'filename': filename,
+                        'diff': ''.join(lines),
+                        'line': line_number,
+                    })
+                    lines = []
+                filename = match.group(2).rstrip('\r\n')
+                continue
+            match = re.search(r'^@@ -(\d+)(,(\d+))? \+(\d+)(,(\d+))?', line)
+            if match:
+                if len(lines) > 0:
+                    entries.append({
+                        'filename': filename,
+                        'diff': ''.join(lines),
+                        'line': line_number,
+                    })
+                    lines = []
+                line_number = int(match.group(1))
+                continue
+            if line.startswith('+') or line.startswith('-'):
+                lines.append(line)
+        if len(lines) > 0:
+            entries.append({
+                'filename': filename,
+                'diff': ''.join(lines),
+                'line': line_number,
+            })
+        return entries
+
    @staticmethod
    def _test_case_status(test_case) -> str:
        """Get the status of a test case based on an etree node."""
@ -177,8 +241,10 @@ class PhabTalk:
            return 'skip'
        return 'pass'

-    def report_all(self, diff_id: str, ph_id: str, test_result_file: str, comment_file: str, build_result:str):
-        test_results = self._compute_test_results(test_result_file)
+    def report_all(self, diff_id: str, ph_id: str, test_result_file: str,
+        comment_file: str, build_result: str, clang_format_patch: str):
+        test_results = self._compute_test_results(test_result_file, clang_format_patch)
+
        self._report_test_results(ph_id, test_results, build_result)
        self._comment_on_diff_from_file(diff_id, comment_file, test_results, build_result)
        print('reporting completed.')
@ -203,8 +269,11 @@ def main():
    while True:
        # retry on connenction problems
        try:
+            # TODO: separate build of test results and sending the individual messages (to diff and test results)
            p = PhabTalk(args.conduit_token, args.host, args.dryrun)
-            p.report_all(args.diff_id, args.ph_id, args.test_result_file, args.comment_file, args.buildresult)
+            p.report_all(args.diff_id, args.ph_id, args.test_result_file,
+                         args.comment_file, args.buildresult,
+                         args.clang_format_patch)
        except socket.timeout as e:
            errorcount += 1
            if errorcount > 5:
@ -228,6 +297,9 @@ def _parse_args():
    parser.add_argument('--dryrun', action='store_true',help="output results to the console, do not report back to the server")
    parser.add_argument('--buildresult', type=str, default=None,
                        choices=['SUCCESS', 'UNSTABLE', 'FAILURE', 'null'])
+    parser.add_argument('--clang-format-patch', type=str, default=None,
+                        dest='clang_format_patch',
+                        help="patch produced by git-clang-format")
    return parser.parse_args()    


--- a/scripts/run_cmake.sh
+++ b/scripts/run_cmake.sh
@ -14,19 +14,16 @@
 # limitations under the License.
 set -eux

+# Runs Cmake.
+# Inputs: CCACHE_PATH, WORKSPACE, TARGET_DIR; $WORKSPACE/build must exist.
+# Outputs: $TARGET_DIR/CMakeCache.txt, $WORKSPACE/compile_commands.json (symlink).
+
 echo "Running CMake... ======================================"
-cd ${WORKSPACE}
-rm -rf build || true
-mkdir build
-cd build
 export CC=clang-8
 export CXX=clang++-8
 export LD=LLD

-#TODO: move this to the pipeline
-TARGET_DIR="/mnt/nfs/results/${JOB_BASE_NAME}-${BUILD_NUMBER}"
-mkdir -p ${TARGET_DIR}
-
+cd "$WORKSPACE"/build
 set +e
 cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_LLD=ON \
    -D LLVM_ENABLE_PROJECTS="clang;clang-tools-extra;libcxx;libcxxabi;lld;libunwind" \
@ -36,7 +33,8 @@ cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_LLD=ON \
 RETURN_CODE="${PIPESTATUS[0]}"
 set -e

-#TODO: move this to the Pipeline
+ln -s "$WORKSPACE"/build/compile_commands.json "$WORKSPACE"
 cp CMakeCache.txt ${TARGET_DIR}
+
 echo "CMake completed ======================================"
-exit ${RETURN_CODE}
+exit "${RETURN_CODE}"
--- a/scripts/run_ninja.sh
+++ b/scripts/run_ninja.sh
@ -14,23 +14,22 @@
 # limitations under the License.
 set -eu

+# Runs ninja
+# Inputs: TARGET_DIR, WORKSPACE.
+# Outputs: $TARGET_DIR/test_results.xml
+
 CMD=$1
-echo "Running ${CMD}... ====================================="
-cd ${WORKSPACE}
-# TODO: move copy operation to pipeline
-BUILD_ID="${JOB_BASE_NAME}-${BUILD_NUMBER}"
-TARGET_DIR="/mnt/nfs/results/${BUILD_ID}"
+echo "Running ninja ${CMD}... ====================================="

 ulimit -n 8192
-cd build
+cd "${WORKSPACE}/build"

 set +e
 ninja ${CMD} 
 RETURN_CODE="$?"
 set -e

-echo "check-all completed ======================================"
-# TODO: move copy operation to pipeline
+echo "ninja ${CMD} completed ======================================"
 if test -f "test-results.xml" ; then
 	cp test-results.xml ${TARGET_DIR}
 fi