From 26da69f60ad45be185bdd30c3959676e582dcdc4 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Mon, 9 Dec 2019 10:46:33 +0100 Subject: [PATCH 01/12] Fix playbooks link --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 1fc817c..1e42332 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ This repository contains the configuration files for the pre-merge checks for the LLVM project. This github project contains the documentation and the server configuration cluster of build machines that are used to check all incoming commits to the LLVM project. # User documentation -see [docs/user_doc.md](docs/user_doc.md) +See [docs/user_doc.md](docs/user_doc.md) # Pre-merge check vision for end of 2019 Maria is a developer working on a new idea for the LLVM project. When she submits a new diff to Phabricator (or updates an existing diff), the pre-merge checks are triggered automatically in the cloud. The pre-merge checks run in one configuration (amd64, Debian Testing, clang8) and comprise these steps: @@ -59,7 +59,7 @@ On the Jenkins side: There is no backup of the credentials. If you need to change it, generate a new one and update it in Jenkins and Phabricator. # Additional Information -* [Playbooks](docs/playbook.yaml) for installing/upgrading +* [Playbooks](docs/playbooks.md) for installing/upgrading * [User documentation](docs/user_doc.md) # License From 650ed9c2747f785d50bfeabf481c0b3bff772e65 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Mon, 9 Dec 2019 10:59:08 +0100 Subject: [PATCH 02/12] Add section about local changes (#73) For now only describing how to get an conduit API token --- docs/playbooks.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/playbooks.md b/docs/playbooks.md index 1920146..3e486ce 100644 --- a/docs/playbooks.md +++ b/docs/playbooks.md @@ -105,3 +105,8 @@ powershell Invoke-WebRequest -uri 'https://raw.githubusercontent.com/google/llvm-premerge-checks/master/kubernetes/windows_agent_bootstrap.ps1' -OutFile windows_agent_bootstrap.ps1 .\windows_agent_bootstrap.ps1 ``` + +## 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//page/apitokens/ . From 7faaec98e752b3987f33fd0cb49042c5dd2de0e9 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Mon, 9 Dec 2019 12:09:53 +0100 Subject: [PATCH 03/12] 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. --- .gitignore | 4 +- Jenkins/Phabricator-pipeline/Jenkinsfile | 22 +++-- Jenkins/master-linux-pipeline/Jenkinsfile | 2 +- Jenkins/master-windows-pipeline/Jenkinsfile | 1 - containers/build_run.sh | 3 + docs/playbooks.md | 7 +- scripts/lint.sh | 37 +++++++++ scripts/phabtalk/README.md | 6 +- scripts/phabtalk/phabtalk.py | 92 ++++++++++++++++++--- scripts/run_cmake.sh | 22 +++-- scripts/run_ninja.sh | 15 ++-- 11 files changed, 168 insertions(+), 43 deletions(-) create mode 100755 scripts/lint.sh diff --git a/.gitignore b/.gitignore index dbe9c82..238546f 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,3 @@ -.vscode/ \ No newline at end of file +.vscode/ +.idea/ +*.iml diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index 3ee53a6..7d38ae7 100644 --- 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,8 +45,10 @@ 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'){ steps { @@ -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,8 +105,9 @@ 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" """ } } -} \ No newline at end of file +} diff --git a/Jenkins/master-linux-pipeline/Jenkinsfile b/Jenkins/master-linux-pipeline/Jenkinsfile index f7a505a..3410a8e 100644 --- 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' diff --git a/Jenkins/master-windows-pipeline/Jenkinsfile b/Jenkins/master-windows-pipeline/Jenkinsfile index a5ebee3..47ddc66 100644 --- 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"){ diff --git a/containers/build_run.sh b/containers/build_run.sh index 6d6772b..f8463a0 100755 --- 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 )" diff --git a/docs/playbooks.md b/docs/playbooks.md index 3e486ce..65931e0 100644 --- 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//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//page/apitokens/`. diff --git a/scripts/lint.sh b/scripts/lint.sh new file mode 100755 index 0000000..0448b19 --- /dev/null +++ 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 ======================================" diff --git a/scripts/phabtalk/README.md b/scripts/phabtalk/README.md index 8005744..6155d25 100644 --- a/scripts/phabtalk/README.md +++ b/scripts/phabtalk/README.md @@ -1,2 +1,4 @@ -This folder contains Python scripts that to Phabricator. -They require a few libraries listed in `requirements.txt`. \ No newline at end of file +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`. \ No newline at end of file diff --git a/scripts/phabtalk/phabtalk.py b/scripts/phabtalk/phabtalk.py index 19e4831..13cca57 100755 --- 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: @@ -165,9 +174,64 @@ class PhabTalk: 'details' : failure.text } result.result_type = 'fail' - result.unit.append(test_result) + 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() diff --git a/scripts/run_cmake.sh b/scripts/run_cmake.sh index 59050b7..bcc9488 100755 --- a/scripts/run_cmake.sh +++ b/scripts/run_cmake.sh @@ -14,29 +14,27 @@ # 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" \ - -D LLVM_CCACHE_BUILD=ON -D LLVM_CCACHE_DIR="${CCACHE_PATH}" -D LLVM_CCACHE_MAXSIZE=20G \ + -D LLVM_CCACHE_BUILD=ON -D LLVM_CCACHE_DIR="${CCACHE_PATH}" -D LLVM_CCACHE_MAXSIZE=20G \ -D LLVM_ENABLE_ASSERTIONS=ON -DCMAKE_CXX_FLAGS=-gmlt \ - -DLLVM_LIT_ARGS="-v --xunit-xml-output ${WORKSPACE}/build/test-results.xml" + -DLLVM_LIT_ARGS="-v --xunit-xml-output ${WORKSPACE}/build/test-results.xml" 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} \ No newline at end of file +exit "${RETURN_CODE}" diff --git a/scripts/run_ninja.sh b/scripts/run_ninja.sh index 75068c4..0f0c3e1 100755 --- 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 From 164610f2849c36c75ddb2686a08aaff6b4d2549f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20K=C3=BChnel?= Date: Wed, 11 Dec 2019 13:04:36 +0100 Subject: [PATCH 04/12] fixed duplicate variable declaration to fix failing build: https://jenkins.llvm-merge-guard.org/job/amd64_debian_testing_clang8/432/console --- Jenkins/Phabricator-pipeline/Jenkinsfile | 1 - 1 file changed, 1 deletion(-) diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index 7d38ae7..1ce4b40 100644 --- a/Jenkins/Phabricator-pipeline/Jenkinsfile +++ b/Jenkins/Phabricator-pipeline/Jenkinsfile @@ -28,7 +28,6 @@ pipeline { 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"){ From 87bf3c2601aa0297068cf2289734349977c8e643 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20K=C3=BChnel?= Date: Wed, 11 Dec 2019 13:07:49 +0100 Subject: [PATCH 05/12] added missing quote --- Jenkins/Phabricator-pipeline/Jenkinsfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index 1ce4b40..01d5c34 100644 --- a/Jenkins/Phabricator-pipeline/Jenkinsfile +++ b/Jenkins/Phabricator-pipeline/Jenkinsfile @@ -46,7 +46,7 @@ pipeline { } sh 'rm -rf build || true' sh 'mkdir -p build' - sh 'mkdir -p "${TARGET_DIR}' + sh 'mkdir -p "${TARGET_DIR}"' } } stage('arc patch'){ From 4577fa81cd4a58b30d7b7f5fea708801500f92f1 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Wed, 11 Dec 2019 13:27:36 +0100 Subject: [PATCH 06/12] fix call to phabtalk.py --- Jenkins/Phabricator-pipeline/Jenkinsfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index 01d5c34..4f69f69 100644 --- a/Jenkins/Phabricator-pipeline/Jenkinsfile +++ b/Jenkins/Phabricator-pipeline/Jenkinsfile @@ -105,7 +105,7 @@ EOF --comment-file "${PHAB_LOG}" \ --host "${PHABRICATOR_HOST}/api/" \ --buildresult ${currentBuild.result} \ - ---clang-format-patch="${TARGET_DIR}/clang-format.patch" + ---clang-format-patch "${TARGET_DIR}/clang-format.patch" """ } } From a3cb3b11d33716e2416f20cf2404bd63cf557a10 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Wed, 11 Dec 2019 13:45:25 +0100 Subject: [PATCH 07/12] fix call to phabtalk.py --- Jenkins/Phabricator-pipeline/Jenkinsfile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index 4f69f69..eac7490 100644 --- a/Jenkins/Phabricator-pipeline/Jenkinsfile +++ b/Jenkins/Phabricator-pipeline/Jenkinsfile @@ -33,6 +33,7 @@ pipeline { stage("build info"){ steps { echo "Building diff ${DIFF_ID} with PHID ${PHID}" + currentBuild.description = "diff ${DIFF_ID}" } } stage("git checkout"){ @@ -76,7 +77,8 @@ pipeline { } } post { - always { + always { + echo "Console log is available at ${RESULT_URL}" dir("${TARGET_DIR}") { // copy console log to result folder @@ -105,7 +107,7 @@ EOF --comment-file "${PHAB_LOG}" \ --host "${PHABRICATOR_HOST}/api/" \ --buildresult ${currentBuild.result} \ - ---clang-format-patch "${TARGET_DIR}/clang-format.patch" + --clang-format-patch "${TARGET_DIR}/clang-format.patch" """ } } From e06b9c2dcd52e0bc9010b5f7c8df9c45675c87fc Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Wed, 11 Dec 2019 13:49:50 +0100 Subject: [PATCH 08/12] add diff id to build name --- Jenkins/Phabricator-pipeline/Jenkinsfile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index eac7490..08b8836 100644 --- a/Jenkins/Phabricator-pipeline/Jenkinsfile +++ b/Jenkins/Phabricator-pipeline/Jenkinsfile @@ -33,7 +33,9 @@ pipeline { stage("build info"){ steps { echo "Building diff ${DIFF_ID} with PHID ${PHID}" - currentBuild.description = "diff ${DIFF_ID}" + script { + currentBuild.displayName = "diff ${DIFF_ID}" + } } } stage("git checkout"){ From 8d390ff37010c2595197e1f4f7f50a19f6746735 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Mon, 9 Dec 2019 12:09:53 +0100 Subject: [PATCH 09/12] Refactor Phabricator report construction Now report is constructed by adding more data from several stages and represents internal structure rather what API expects. + Made all function that don't interact with phabricator free standing + Now links list all files in results directory + Status icons Example comment: https://reviews.llvm.org/D71197#1779176 (links don't work). --- Jenkins/Phabricator-pipeline/Jenkinsfile | 16 +- Jenkins/master-windows-pipeline/Jenkinsfile | 1 + docs/playbooks.md | 2 +- scripts/phabtalk/phabtalk.py | 420 ++++++++++---------- scripts/run_ninja.sh | 2 +- 5 files changed, 226 insertions(+), 215 deletions(-) diff --git a/Jenkins/Phabricator-pipeline/Jenkinsfile b/Jenkins/Phabricator-pipeline/Jenkinsfile index 08b8836..6bb71ae 100644 --- a/Jenkins/Phabricator-pipeline/Jenkinsfile +++ b/Jenkins/Phabricator-pipeline/Jenkinsfile @@ -26,7 +26,6 @@ pipeline { 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" } stages { @@ -95,21 +94,14 @@ pipeline { """ } /// send results to Phabricator - // TODO: list all files from results directory instead - sh """ -set +x -cat <<-EOF>> ${PHAB_LOG} - -Log files: [[${RESULT_URL}/console-log.txt | console-log.txt]], [[${RESULT_URL}/CMakeCache.txt | CMakeCache.txt]] -EOF -""" sh """${SCRIPT_DIR}/phabtalk/phabtalk.py "${PHID}" "${DIFF_ID}" \ --conduit-token "${CONDUIT_TOKEN}" \ - --test-result-file "${TEST_REPORT}" \ - --comment-file "${PHAB_LOG}" \ + --test-result-file "test-results.xml" \ --host "${PHABRICATOR_HOST}/api/" \ --buildresult ${currentBuild.result} \ - --clang-format-patch "${TARGET_DIR}/clang-format.patch" + --clang-format-patch "clang-format.patch" \ + --results-dir "${TARGET_DIR}" \ + --results-url "${RESULT_URL}" """ } } diff --git a/Jenkins/master-windows-pipeline/Jenkinsfile b/Jenkins/master-windows-pipeline/Jenkinsfile index 47ddc66..95b7585 100644 --- a/Jenkins/master-windows-pipeline/Jenkinsfile +++ b/Jenkins/master-windows-pipeline/Jenkinsfile @@ -28,6 +28,7 @@ pipeline { git url: 'https://github.com/llvm/llvm-project.git' powershell 'git clean -fdx' powershell 'New-Item -ItemType Directory -Force -Path llvm-premerge-checks' + powershell 'New-Item -ItemType Directory -Force -Path ${TARGET_DIR}' dir("llvm-premerge-checks") { git url: 'https://github.com/google/llvm-premerge-checks.git' diff --git a/docs/playbooks.md b/docs/playbooks.md index 65931e0..46268e6 100644 --- a/docs/playbooks.md +++ b/docs/playbooks.md @@ -112,4 +112,4 @@ Build and run agent docker image `sudo build_run.sh agent-debian-testing-clang8- 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//page/apitokens/`. +Additionally set `WORKSPACE`, `PHID` and `DIFF_ID` parameters. Set `CONDUIT_TOKEN` with your personal one from `https://reviews.llvm.org/settings/user//page/apitokens/`. \ No newline at end of file diff --git a/scripts/phabtalk/phabtalk.py b/scripts/phabtalk/phabtalk.py index 13cca57..ea27214 100755 --- a/scripts/phabtalk/phabtalk.py +++ b/scripts/phabtalk/phabtalk.py @@ -28,16 +28,18 @@ from lxml import etree from phabricator import Phabricator -class TestResults: +class BuildReport: def __init__(self): - self.result_type = None # type: str - self.unit = [] #type: List + self.comments = [] + self.success = True + self.working = False + self.unit = [] # type: List self.lint = [] self.test_stats = { - 'pass':0, - 'fail':0, - 'skip':0 + 'pass': 0, + 'fail': 0, + 'skip': 0 } # type: Dict[str, int] @@ -51,7 +53,7 @@ class PhabTalk: if not dryrun: self._phab = Phabricator(token=token, host=host) self._phab.update_interfaces() - + @property def dryrun(self): return self._phab is None @@ -64,244 +66,260 @@ class PhabTalk: result = self._phab.differential.querydiffs(ids=[diff]) return 'D' + result[diff]['revisionID'] - def _comment_on_diff(self, diff: str, text: str): + def _comment_on_diff(self, diff_id: str, text: str): """Add a comment to a differential based on the diff_id""" - print('Sending comment to diff {}:'.format(diff)) + print('Sending comment to diff {}:'.format(diff_id)) print(text) - self._comment_on_revision(self._get_revision_id(diff), text) + self._comment_on_revision(self._get_revision_id(diff_id), text) def _comment_on_revision(self, revision: str, text: str): """Add comment on a differential based on the revision id.""" transactions = [{ - 'type' : 'comment', - 'value' : text + 'type': 'comment', + 'value': text }] if self.dryrun: print('differential.revision.edit =================') print('Transactions: {}'.format(transactions)) return - + # API details at # https://secure.phabricator.com/conduit/method/differential.revision.edit/ - self._phab.differential.revision.edit(objectIdentifier=revision, transactions=transactions) + self._phab.differential.revision.edit(objectIdentifier=revision, + transactions=transactions) - def _comment_on_diff_from_file(self, diff: str, text_file_path: str, test_results: TestResults, buildresult:str): - """Comment on a diff, read text from file.""" - header = '' - if test_results.result_type is None: - # do this if there are no test results - header = 'Build result: {} - '.format(buildresult) - else: - header = 'Build result: {} - '.format(test_results.result_type) - header += '{} tests passed, {} failed and {} were skipped.\n'.format( - test_results.test_stats['pass'], - test_results.test_stats['fail'], - test_results.test_stats['skip'], - ) - for test_case in test_results.unit: - if test_case['result'] == 'fail': - header += ' failed: {}/{}\n'.format(test_case['namespace'], test_case['name']) - - text = '' - if text_file_path is not None and os.path.exists(text_file_path): - with open(text_file_path) as input_file: - text = input_file.read() - - if len(header+text) == 0: - print('Comment for Phabricator would be empty. Not posting it.') - return - - self._comment_on_diff(diff, header + text) - - def _report_test_results(self, phid: str, test_results: TestResults, build_result: str): - """Report failed tests to phabricator. - - Only reporting failed tests as the full test suite is too large to upload. + def submit_report(self, diff_id: str, phid: str, report: BuildReport, build_result: str): + """Submit collected report to Phabricator. """ - # use jenkins build status if possible - result = self._translate_jenkins_status(build_result) - # fall back to test results if Jenkins status is not availble - if result is None: - result = test_results.result_type - # If we do not have a proper status: fail the build. - if result is None: - result = 'fail' + result_type = 'pass' + if report.working: + result_type = 'working' + elif not report.success: + result_type = 'fail' if self.dryrun: print('harbormaster.sendmessage =================') - print('type: {}'.format(result)) - print('unit: {}'.format(test_results.unit)) - print('lint: {}'.format(test_results.lint)) - return + print('type: {}'.format(result_type)) + print('unit: {}'.format(report.unit)) + print('lint: {}'.format(report.lint)) + else: + _try_call(lambda: self._phab.harbormaster.sendmessage( + buildTargetPHID=phid, + type=result_type, + unit=report.unit, + lint=report.lint)) - # API details at - # https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/ - self._phab.harbormaster.sendmessage( - buildTargetPHID=phid, - type=result, - unit=test_results.unit, - lint=test_results.lint) + if len(report.comments) > 0: + _try_call(lambda: self._comment_on_diff(diff_id, '\n\n'.join(report.comments))) - def _compute_test_results(self, build_result_file: str, clang_format_patch: str) -> TestResults: - result = TestResults() - if build_result_file is None: - # If no result file is specified: assume this is intentional - result.result_type = None - return result - if not os.path.exists(build_result_file): - print('Warning: Could not find test results file: {}'.format(build_result_file)) - result.result_type = None - return result - - root_node = etree.parse(build_result_file) - result.result_type = 'pass' - - for test_case in root_node.xpath('//testcase'): - test_result = self._test_case_status(test_case) - result.test_stats[test_result] += 1 +def _parse_patch(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 - if test_result == 'fail': - failure = test_case.find('failure') - test_result = { - 'name' : test_case.attrib['name'], - 'namespace' : test_case.attrib['classname'], - 'result' : test_result, - 'duration' : float(test_case.attrib['time']), - 'details' : failure.text - } - 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 +def _add_clang_format(report: BuildReport, clang_format_patch: str, results_dir: str, + results_url: str): + """Populates results from diff produced by clang format.""" + if clang_format_patch is None: + return + p = os.path.join(results_dir, clang_format_patch) + ok = True + if os.path.exists(p): + ok = False + diffs = _parse_patch(open(p, '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```', + } + report.lint.append(lint_message) + comment = section_title('clang-format', ok) + if not ok: + comment += 'Please format your changes with clang-format by running `git-clang-format HEAD^` or apply ' \ + 'this [[ {}/{} | patch ]].'.format(results_url, clang_format_patch) + report.comments.append(comment) - @staticmethod - def _test_case_status(test_case) -> str: - """Get the status of a test case based on an etree node.""" - if test_case.find('failure') is not None: - return 'fail' - if test_case.find('skipped') is not None: - return 'skip' - return 'pass' - 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.') - - @staticmethod - def _translate_jenkins_status(jenkins_status: str) -> str: - """ - Translate the build status form Jenkins to Phabricator. - - Jenkins semantics: https://jenkins.llvm-merge-guard.org/pipeline-syntax/globals#currentBuild - Phabricator semantics: https://reviews.llvm.org/conduit/method/harbormaster.sendmessage/ - """ - if jenkins_status.lower() == 'success': - return 'pass' - if jenkins_status.lower() == 'null': - return 'working' - return 'fail' - -def main(): - args = _parse_args() - errorcount = 0 +def _try_call(call): + """Tries to call function several times retrying on socked.timeout.""" + c = 0 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, - args.clang_format_patch) + call() except socket.timeout as e: - errorcount += 1 - if errorcount > 5: + c += 1 + if c > 5: print('Connection to Pharicator failed, giving up: {}'.format(e)) raise print('Connection to Pharicator failed, retrying: {}'.format(e)) - time.sleep(errorcount*10) + time.sleep(c * 10) break +def _add_test_results(report: BuildReport, build_result_file: str): + """Populates results from build test results XML. + + Only reporting failed tests as the full test suite is too large to upload. + """ + if build_result_file is None: + return + if not os.path.exists(build_result_file): + print('Warning: Could not find test results file: {}'.format( + build_result_file)) + return + + root_node = etree.parse(build_result_file) + + ok = True + for test_case in root_node.xpath('//testcase'): + test_result = _test_case_status(test_case) + report.test_stats[test_result] += 1 + + if test_result == 'fail': + ok = False + failure = test_case.find('failure') + test_result = { + 'name': test_case.attrib['name'], + 'namespace': test_case.attrib['classname'], + 'result': test_result, + 'duration': float(test_case.attrib['time']), + 'details': failure.text + } + report.unit.append(test_result) + + report.success = ok and report.success + comment = section_title('Unit tests', ok) + comment += '{} tests passed, {} failed and {} were skipped.\n'.format( + report.test_stats['pass'], + report.test_stats['fail'], + report.test_stats['skip'], + ) + for test_case in report.unit: + if test_case['result'] == 'fail': + comment += ' failed: {}/{}\n'.format(test_case['namespace'], test_case['name']) + report.comments.append(comment) + + +def _add_links_to_artifacts(report: BuildReport, results_dir: str, results_url: str): + """Comment on a diff, read text from file.""" + file_links = [] + for f in os.listdir(results_dir): + if not os.path.isfile(os.path.join(results_dir, f)): + continue + file_links.append('[[{0}/{1} | {1}]]'.format(results_url, f)) + if len(file_links) > 0: + report.comments.append('[[ {} | Build artifacts ]]: '.format(results_url) + ', '.join(file_links)) + + +def _test_case_status(test_case) -> str: + """Get the status of a test case based on an etree node.""" + if test_case.find('failure') is not None: + return 'fail' + if test_case.find('skipped') is not None: + return 'skip' + return 'pass' + + +def section_title(title: str, ok: bool) -> str: + return '{} {}: {}. '.format( + '{icon check-circle color=green}' if ok else '{icon times-circle color=red}', + title, + 'pass' if ok else 'fail') + + +def main(): + args = _parse_args() + report = BuildReport() + + if args.buildresult is not None: + print('Jenkins result: {}'.format(args.buildresult)) + if args.buildresult.lower() == 'success': + pass + elif args.buildresult.lower() == 'null': + report.working = True + else: + report.success = False + + _add_test_results(report, os.path.join(args.results_dir, args.test_result_file)) + _add_clang_format(report, args.clang_format_patch, args.results_dir, args.results_url) + _add_links_to_artifacts(report, args.results_dir, args.results_url) + p = PhabTalk(args.conduit_token, args.host, args.dryrun) + p.submit_report(args.diff_id, args.ph_id, report, args.buildresult) + + def _parse_args(): - parser = argparse.ArgumentParser(description='Write build status back to Phabricator.') + parser = argparse.ArgumentParser( + description='Write build status back to Phabricator.') parser.add_argument('ph_id', type=str) - parser.add_argument('diff_id', type=str) - parser.add_argument('--comment-file', type=str, dest='comment_file', default=None) + parser.add_argument('diff_id', type=str) parser.add_argument('--test-result-file', type=str, dest='test_result_file', - default=os.path.join(os.path.curdir,'test-results.xml')) - parser.add_argument('--conduit-token', type=str, dest='conduit_token', default=None) - parser.add_argument('--host', type=str, dest='host', default="None", - help="full URL to API with trailing slash, e.g. https://reviews.llvm.org/api/") - parser.add_argument('--dryrun', action='store_true',help="output results to the console, do not report back to the server") + default='test-results.xml') + parser.add_argument('--conduit-token', type=str, dest='conduit_token', + default=None) + parser.add_argument('--host', type=str, dest='host', default="None", + help="full URL to API with trailing slash, e.g. https://reviews.llvm.org/api/") + 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() + help="path to diff produced by git-clang-format, relative to results-dir") + parser.add_argument('--results-dir', type=str, default=None, + dest='results_dir', + help="directory of all build artifacts") + parser.add_argument('--results-url', type=str, default=None, + dest='results_url', + help="public URL to access results directory") + return parser.parse_args() if __name__ == '__main__': - main() \ No newline at end of file + main() diff --git a/scripts/run_ninja.sh b/scripts/run_ninja.sh index 0f0c3e1..de2649d 100755 --- a/scripts/run_ninja.sh +++ b/scripts/run_ninja.sh @@ -31,7 +31,7 @@ set -e echo "ninja ${CMD} completed ======================================" if test -f "test-results.xml" ; then - cp test-results.xml ${TARGET_DIR} + cp test-results.xml "${TARGET_DIR}" fi exit ${RETURN_CODE} \ No newline at end of file From 6755122c57600bf92afdb1593297f5a9e11676c4 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Wed, 11 Dec 2019 16:05:34 +0100 Subject: [PATCH 10/12] Update issue templates --- .github/ISSUE_TEMPLATE/bug_report.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/bug_report.md diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md new file mode 100644 index 0000000..99016b4 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -0,0 +1,23 @@ +--- +name: Bug report +about: Create a report to help us improve llvm-premerge-checks +title: "[phabricator]" +labels: bug +assignees: '' + +--- + +**Describe the bug** +A clear and concise description of what the bug is. +Relevant links: + +**To Reproduce** +Steps to reproduce the behavior: +1. Submit +2. + +**Expected behavior** +A clear and concise description of what you expected to happen. + +**Screenshots** +If applicable, add screenshots to help explain your problem. From e69c0e41303d2f5b41c838c7f8ca30c9f5f1d982 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Wed, 11 Dec 2019 16:06:13 +0100 Subject: [PATCH 11/12] Update bug_report.md --- .github/ISSUE_TEMPLATE/bug_report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 99016b4..2fbb08d 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -1,7 +1,7 @@ --- name: Bug report about: Create a report to help us improve llvm-premerge-checks -title: "[phabricator]" +title: "" labels: bug assignees: '' From c04a10e9143f7fc27498a654132fb268611691ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20K=C3=BChnel?= Date: Wed, 11 Dec 2019 17:23:42 +0100 Subject: [PATCH 12/12] merged changes from other Jenkinsfile --- Jenkins/master-linux-pipeline/Jenkinsfile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Jenkins/master-linux-pipeline/Jenkinsfile b/Jenkins/master-linux-pipeline/Jenkinsfile index 3410a8e..941d7ae 100644 --- a/Jenkins/master-linux-pipeline/Jenkinsfile +++ b/Jenkins/master-linux-pipeline/Jenkinsfile @@ -28,12 +28,14 @@ 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' } - } + sh 'rm -rf build || true' + sh 'mkdir -p build' + sh 'mkdir -p "${TARGET_DIR}"' + } } stage('CMake') { steps {