Merge pull request #104 from google/bot-text

Update bot report

docs/clang_tidy.md

@@ -0,0 +1,29 @@
+# clang-tidy checks
+## warning is not useful
+If you found that a warning produced by clang-tidy is not useful:
+  
+- If clang-tidy must not run for some files at all (e.g. lit test), please
+[add files to blacklist](scripts/clang-tidy.ignore).
+
+- Consider fixing or [suppressing diagnostic](https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics)
+  if there is a good reason.
+  
+- [File a bug](issues/new?assignees=&labels=bug&template=bug_report.md&title=)
+  if build process should be improved. 
+
+- If you believe that you found a clang-tidy bug then please keep in mind that clang-tidy version run by bot
+  might not be most recent. Please reproduce your issue on current version before submitting a bug to clang-tidy.
+
+## Review comments
+
+Build bot leaves inline comments only for a small subset of files that are not blacklisted for analysis (see above) *and*
+specifically whitelisted for comments.
+
+That is done to avoid potential noise when a file already contains a number of warnings.
+
+If your are confident that some files are in good shape already, please
+[whitelist them](scripts/clang-tidy-comments.ignore).
+
+----
+
+[about pre-merge checks](docs/user_doc.md)

docs/user_doc.md

@@ -44,17 +44,19 @@ Only then can the build server apply the patch locally and run the builds and te
 Once you're signed up, Phabricator will automatically trigger a build for every new patch you upload or every existing patch you modify. Phabricator shows the build results at the top of the entry:
 ![build status](images/diff_detail.png)
 
+Bot will compile and run tests, run clang-format and [clang-tidy](docs/clang_tidy.md) on lines changed. 
+
 If a unit test failed, this is shown below the build status. You can also expand the unit test to see the details:
 ![unit test results](images/unit_tests.png)
 
-After every build the build server will comment on your latest patch, so that you can also see the results for previous changes. The comment also contains a link to the log files:
+After every build the build server will comment on your latest patch, so that you can also see the results for previous changes.
+The comment also contains a link to the log files:
 ![bot comment](images/bot_comment.png)
 
 The build logs are stored for 90 days and automatically deleted after that.
 
 You can also trigger a build manually by using the "Run Plan Manually" link on the [Harbormaster page](https://reviews.llvm.org/harbormaster/plan/3/) and entering a revision ID in the pop-up window.
 
-
 # Reporting issues
 
 If you notice any bugs, please create a [new issue](https://github.com/google/llvm-premerge-checks/issues/new).

scripts/phabtalk/phabtalk.py

@@ -22,6 +22,7 @@ import os
 import re
 import socket
 import time
+import urllib
 from typing import Optional
 
 import pathspec
@@ -65,7 +66,7 @@ class PhabTalk:
     def dryrun(self):
         return self._phab is None
 
-    def _get_revision_id(self, diff: str):
+    def get_revision_id(self, diff: str):
         """Get the revision ID for a diff from Phabricator."""
         if self.dryrun:
             return None
@@ -77,7 +78,7 @@ class PhabTalk:
         """Add a comment to a differential based on the diff_id"""
         print('Sending comment to diff {}:'.format(diff_id))
         print(text)
-        self._comment_on_revision(self._get_revision_id(diff_id), 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."""
@@ -222,6 +223,7 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
     pattern = '^{}/([^:]*):(\\d+):(\\d+): (.*): (.*)'.format(workspace)
     errors_count = 0
     warn_count = 0
+    inline_comments = 0
     present = (clang_tidy_file is not None) and os.path.exists(os.path.join(results_dir, clang_tidy_file))
     if not present:
         print('clang-tidy result {} is not found'.format(clang_tidy_file))
@@ -243,6 +245,8 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
             char_pos = match.group(3)
             severity = match.group(4)
             text = match.group(5)
+            text += '\n[[https://github.com/google/llvm-premerge-checks/blob/master/docs/clang_tidy.md#warning-is-not' \
+                    '-useful || not useful]] '
             if severity in ['warning', 'error']:
                 if severity == 'warning':
                     warn_count += 1
@@ -251,6 +255,7 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
                 if ignore.match_file(file_name):
                     print('{} is ignored by pattern and no comment will be added')
                 else:
+                    inline_comments += 1
                     report.addLint({
                         'name': 'clang-tidy',
                         'severity': 'warning',
@@ -263,8 +268,11 @@ def _add_clang_tidy(report: BuildReport, results_dir: str, results_url: str, wor
     success = errors_count + warn_count == 0
     comment = section_title('clang-tidy', success, present)
     if not success:
-        comment += 'clang-tidy found [[ {}/{} | {} errors and {} warnings]].'\
+        comment += "clang-tidy found [[ {}/{} | {} errors and {} warnings]]. {} of them are added as review comments " \
-            .format(results_url, clang_tidy_file, errors_count, warn_count)
+                   "below ([[https://github.com/google/llvm-premerge-checks/blob/master/docs/clang_tidy.md#review" \
+                   "-comments || why?]])." \
+            .format(results_url, clang_tidy_file, errors_count, warn_count, inline_comments)
+
     report.comments.append(comment)
     report.success = success and report.success
 
@@ -375,7 +383,16 @@ def main():
                     args.clang_tidy_ignore)
     _add_clang_format(report, args.results_dir, args.results_url, args.clang_format_patch)
     _add_links_to_artifacts(report, args.results_dir, args.results_url)
+
     p = PhabTalk(args.conduit_token, args.host, args.dryrun)
+    title = 'Issue with build for {} ({})'.format(p.get_revision_id(args.diff_id), args.diff_id)
+    report.comments.append(
+        '//Pre-merge checks is in beta. [[ https://github.com/google/llvm-premerge-checks/issues/new?assignees'
+        '=&labels=bug&template=bug_report.md&title={} | Report issue]]. '
+        'Please [[ https://reviews.llvm.org/project/update/78/join/ || join beta ]] or '
+        '[[ https://github.com/google/llvm-premerge-checks/issues/new?assignees=&labels=enhancement&template'
+        '=&title=enable%20checks%20for%20{{PATH}} || enable it for your project ]].//'.format(
+            urllib.parse.quote(title)))
     p.submit_report(args.diff_id, args.ph_id, report, args.buildresult)