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

@@ -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}" \
+                --test-result-file "test-results.xml" \
-                --comment-file "${PHAB_LOG}" \
                 --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}"
                 """
         }
     }    

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'

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.comments = []
-        self.unit = []  #type: List
+        self.success = True
+        self.working = False
+        self.unit = []  # type: List
         self.lint = []
         self.test_stats = {
-            'pass':0,
+            'pass': 0,
-            'fail':0, 
+            'fail': 0,
-            'skip':0
+            'skip': 0
         }  # type: Dict[str, int]
 
 
@@ -64,18 +66,18 @@ 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',
+            'type': 'comment',
-            'value' : text
+            'value': text
         }]
 
         if self.dryrun:
@@ -85,221 +87,237 @@ class PhabTalk:
 
         # 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):
+    def submit_report(self, diff_id: str, phid: str, report: BuildReport, build_result: str):
-        """Comment on a diff, read text from file."""
+        """Submit collected report to Phabricator.
-        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.
         """
 
-        # use jenkins build status if possible
+        result_type = 'pass'
-        result = self._translate_jenkins_status(build_result)
+        if report.working:
-        # fall back to test results if Jenkins status is not availble
+            result_type = 'working'
-        if result is None:
+        elif not report.success:
-            result = test_results.result_type
+            result_type = 'fail'
-        # If we do not have a proper status: fail the build.
-        if result is None:
-            result = 'fail'
 
         if self.dryrun:
             print('harbormaster.sendmessage =================')
-            print('type: {}'.format(result))
+            print('type: {}'.format(result_type))
-            print('unit: {}'.format(test_results.unit))
+            print('unit: {}'.format(report.unit))
-            print('lint: {}'.format(test_results.lint))
+            print('lint: {}'.format(report.lint))
-            return
+        else:
+            _try_call(lambda: self._phab.harbormaster.sendmessage(
+                buildTargetPHID=phid,
+                type=result_type,
+                unit=report.unit,
+                lint=report.lint))
 
-        # API details at
+        if len(report.comments) > 0:
-        # https://secure.phabricator.com/conduit/method/harbormaster.sendmessage/  
+            _try_call(lambda: self._comment_on_diff(diff_id, '\n\n'.join(report.comments)))
-        self._phab.harbormaster.sendmessage(
-            buildTargetPHID=phid,
-            type=result, 
-            unit=test_results.unit,
-            lint=test_results.lint)
 
-    def _compute_test_results(self, build_result_file: str, clang_format_patch: str) -> TestResults:
-        result = TestResults()
 
-        if build_result_file is None:
+def _parse_patch(patch) -> []:
-            # If no result file is specified: assume this is intentional
+    """Extract the changed lines from `patch` file.
-            result.result_type = None
+    The return value is a list of dictionaries {filename, line, diff}.
-            return result
+    Diff must be generated with -U0 (no context lines).
-        if not os.path.exists(build_result_file):
+    """
-            print('Warning: Could not find test results file: {}'.format(build_result_file))
+    entries = []
-            result.result_type = None
+    lines = []
-            return result
+    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
 
-        root_node = etree.parse(build_result_file)
-        result.result_type = 'pass'
 
-        for test_case in root_node.xpath('//testcase'):
+def _add_clang_format(report: BuildReport, clang_format_patch: str, results_dir: str,
-            test_result = self._test_case_status(test_case)
+                      results_url: str):
-            result.test_stats[test_result] += 1
+    """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)
 
-            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) -> []:
+def _try_call(call):
-        """Extract the changed lines from `patch` file.
+    """Tries to call function several times retrying on socked.timeout."""
-        The return value is a list of dictionaries {filename, line, diff}.
+    c = 0
-        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."""
-        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
     while True:
-        # retry on connenction problems
         try:
-            # TODO: separate build of test results and sending the individual messages (to diff and test results)
+            call()
-            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)
         except socket.timeout as e:
-            errorcount += 1
+            c += 1
-            if errorcount > 5:
+            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('--test-result-file', type=str, dest='test_result_file',
-        default=os.path.join(os.path.curdir,'test-results.xml'))
+                        default='test-results.xml')
-    parser.add_argument('--conduit-token', type=str, dest='conduit_token', default=None)
+    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/")
+                        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('--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")
+                        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()
 
 

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}