Create bot reviewers that do code analysis and post findings as inline comments.
Open, Needs TriagePublic

Description

Hi,

I'm not sure if this is already supported, but I'd like to achieve:

  1. For each revision, run some code analysis (e.g. clang-format, clang-tidy).
  2. Have bot (as a bot user) post findings as inline review comments on the patch.
ioeric created this task.Nov 21 2016, 4:26 PM

Have you looked at the existing lint support already?

The major advantage of lint over a bot is that it happens before code goes for review, so that patches generated by beautifiers like clang-tidy are incorporated locally. Reviewers never need to see unbeautified code or deal with inlines about formatting/style issues.

(Sorry I clicked submit too early)

Possible solutions I can think of:

  1. Create bot users that will subscribe to every patch (use Herald).
  2. For each revision, bots run static analysis on the changed code.
  3. Post findings as inline comments (use Conduit API).

I'm not sure what is the best way to trigger bots to run analysis on the second step: besides notifying bots via emails, is there a way to trigger bot analysis on each revision?

Have you looked at the existing lint support already?

The major advantage of lint over a bot is that it happens before code goes for review, so that patches generated by beautifiers like clang-tidy are incorporated locally. Reviewers never need to see unbeautified code or deal with inlines about formatting/style issues.

Thank you for such prompt reply!

I did looked into Arcanist's linter support, as you suggested.

The problem with this solution is that:

  1. Some analysis like clang-tidy might be too heavy/slow to run locally with arc diff. Running clang-tidy can easily take a few minutes on patches that touch a few compilation units, and we don't want to block users for that long.
  2. Unfortunately, for our instance (https://reviews.llvm.org/), not all users use Arcanist :(
ioeric added a subscriber: klimek.Nov 21 2016, 4:40 PM

Here's the general shape of how to do this. Since we encourage synchronous pre-review lint to fill this role instead of asynchronous build-like processes, support is much rougher than support for lint.

  • Everywhere here, by "build" I mean any kind of build, unit test, integration step, static analysis, artifact production, etc.
  • Use Harbormaster to trigger builds.
    • You can have it "Make HTTP Request" to post a request to some listener, then construct an environment yourself and run static analysis.
    • Or, you can use Drydock to have Phabricator do more of the work.
  • If possible, use staging areas to handoff changes to the build pipeline.
  • Use Conduit to report results to Harbormaster.
    • See T9529 for discussion.
    • Report "lint" results.

The desired flow is:

  1. User uploads a change via arc, sending a copy to a staging area.
  2. Herald triggers a build ("Run Static Analysis") in Harbormaster.
  3. Harbormaster leases a working copy in Drydock.
  4. Drydock constructs a working copy and any support required in the environment, pulling the changes from the staging area.
  5. Drydock runs arc lint --target, reporting lint results to the build.
  6. Differential automatically displays lint results.

In the best case, the --target flag for arc lint does not actually exist. Realistically, you'll have this kind of flow:

  1. Users copy/paste changes into the web UI or whatever.
  2. Herald triggers a build in Harbormaster.
  3. Harbormaster sends a POST HTTP request to some custom server you have somewhere else.
  4. That server tries to arc patch the revision, which sometimes works, but often does not.
  5. That server runs a custom lint-like workflow, mangles it into a harbormaster.sendmessage payload, and submits it via Conduit.

See also T10246#163438 for Drydock, and the Harbormaster / Drydock documentation.

Thank you so much for the tips! I'll check the resources and try them out.