From a3f6610df2147e1702600526e69529373dd95102 Mon Sep 17 00:00:00 2001 From: Mikhail Goncharov Date: Thu, 23 Jan 2020 11:17:23 +0100 Subject: [PATCH] Update bot report - add links to join beta and report issue - add link "not useful" to clang-tidy warning - clang-tidy comment in report now tells how many inline comments were added --- docs/clang_tidy.md | 29 +++++++++++++++++++++++++++++ docs/user_doc.md | 6 ++++-- scripts/phabtalk/phabtalk.py | 25 +++++++++++++++++++++---- 3 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 docs/clang_tidy.md diff --git a/docs/clang_tidy.md b/docs/clang_tidy.md new file mode 100644 index 0000000..fc2714d --- /dev/null +++ b/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) \ No newline at end of file diff --git a/docs/user_doc.md b/docs/user_doc.md index 75adc63..041c399 100644 --- a/docs/user_doc.md +++ b/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). diff --git a/scripts/phabtalk/phabtalk.py b/scripts/phabtalk/phabtalk.py index a34e559..4829b15 100755 --- a/scripts/phabtalk/phabtalk.py +++ b/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]].'\ - .format(results_url, clang_tidy_file, errors_count, warn_count) + comment += "clang-tidy found [[ {}/{} | {} errors and {} warnings]]. {} of them are added as review comments " \ + "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)