Merge pull request #75 from google/clang-format

Display results for git-clang-format

.gitignore

@@ -1 +1,3 @@
 .vscode/
+.idea/
+*.iml

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}"
+            BUILD_ID = "${JOB_BASE_NAME}-${BUILD_NUMBER}"
-            TARGET_DIR = "/mnt/nfs/results/${MY_BUILD_ID}"
+            TARGET_DIR = "/mnt/nfs/results/${BUILD_ID}"
-            RESULT_URL = "http://results.llvm-merge-guard.org/${MY_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"
                 """
         }
     }    

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'

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"){

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 )"
 

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
+Build and run agent docker image `sudo build_run.sh agent-debian-testing-clang8-ssd /bin/bash`.
-https://reviews.llvm.org/settings/user/<USERNAME>/page/apitokens/ .
+
+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/`.

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 ======================================"

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`.

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):
+    def report_all(self, diff_id: str, ph_id: str, test_result_file: str,
-        test_results = self._compute_test_results(test_result_file)
+        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()    
 
 

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
+cd "$WORKSPACE"/build
-TARGET_DIR="/mnt/nfs/results/${JOB_BASE_NAME}-${BUILD_NUMBER}"
-mkdir -p ${TARGET_DIR}
-
 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}"

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}... ====================================="
+echo "Running ninja ${CMD}... ====================================="
-cd ${WORKSPACE}
-# TODO: move copy operation to pipeline
-BUILD_ID="${JOB_BASE_NAME}-${BUILD_NUMBER}"
-TARGET_DIR="/mnt/nfs/results/${BUILD_ID}"
 
 ulimit -n 8192
-cd build
+cd "${WORKSPACE}/build"
 
 set +e
 ninja ${CMD} 
 RETURN_CODE="$?"
 set -e
 
-echo "check-all completed ======================================"
+echo "ninja ${CMD} completed ======================================"
-# TODO: move copy operation to pipeline
 if test -f "test-results.xml" ; then
 	cp test-results.xml ${TARGET_DIR}
 fi