Unverified Commit 00431b92 authored by Vihang Mehta's avatar Vihang Mehta Committed by Copybara
Browse files

Improve arcanist + prototool integration

Summary:
Running prototool once per proto file causes a variety of problems,
a common one of which is the fact that it will warn on new proto files.

This causes us to lose information about what file the breaking change comes
from but should ease some of the other painpoints from this linter.

Test Plan: Ran `arc lint` after making some breaking proto changes.

Reviewers: michelle, zasgar

Reviewed By: michelle

Differential Revision: https://phab.corp.pixielabs.ai/D9042

GitOrigin-RevId: 72a08085270b7dff3da4dc96384d389be747c82e
parent 73c62ffd
Showing with 33 additions and 20 deletions
+33 -20
...@@ -19,6 +19,8 @@ ...@@ -19,6 +19,8 @@
*/ */
final class ArcanistProtoBreakCheckLinter extends ArcanistExternalLinter { final class ArcanistProtoBreakCheckLinter extends ArcanistExternalLinter {
private $future;
public function getInfoName() { public function getInfoName() {
return 'proto-break-check'; return 'proto-break-check';
} }
...@@ -49,35 +51,46 @@ final class ArcanistProtoBreakCheckLinter extends ArcanistExternalLinter { ...@@ -49,35 +51,46 @@ final class ArcanistProtoBreakCheckLinter extends ArcanistExternalLinter {
} }
protected function getMandatoryFlags() { protected function getMandatoryFlags() {
return array('--git-branch=main'); return array('--git-branch=main', '--json');
} }
protected function buildFutures(array $paths) { // Running prototool once per file causes spurious errors around new proto files
// or moving protos between files while maintaining the package name etc.
// Instead we want to run prototool once over the entire repo.
// So we override the arcanist default behavior of running a linter once per file
// to trigger a repo wide prototool lint only once if any proto files change.
public function willLintPaths(array $paths) {
if ($this->future) {
return;
}
$executable = $this->getExecutableCommand(); $executable = $this->getExecutableCommand();
$flags = $this->getCommandFlags(); $flags = $this->getCommandFlags();
$this->future = new ExecFuture('%C break check %Ls', $executable, $flags);
}
$futures = array(); public function didLintPaths(array $paths) {
foreach ($paths as $path) { if (!$this->future) {
$future = new ExecFuture('%C break check %Ls %s', $executable, $flags, $path); return;
$futures[$path] = $future;
$future->setCWD($this->getProjectRoot());
} }
return $futures;
}
protected function parseLinterOutput($path, $err, $stdout, $stderr) { list($err, $stdout, $stderr) = $this->future->resolve();
$messages = array();
if ($err !== 0) { if ($err !== 0) {
$message = id(new ArcanistLintMessage()) $lines = phutil_split_lines($stdout, false);
->setPath($path) foreach ($lines as $line) {
->setCode('breaking-change') $json = phutil_json_decode($line);
->setName('prototool') $this->addLintMessage(id(new ArcanistLintMessage())
->setDescription($stdout) ->setPath(isset($json['filename']) ? $json['filename'] : '')
->setSeverity(ArcanistLintSeverity::SEVERITY_WARNING); ->setCode(isset($json['lint_id']) ? $json['lint_id'] : '')
->setName('prototool')
$messages[] = $message; ->setDescription(isset($json['message']) ? $json['message'] : $line)
->setSeverity(ArcanistLintSeverity::SEVERITY_WARNING));
}
} }
return $messages;
$this->future = null;
} }
public function parseLinterOutput($path, $err, $stdout, $stderr) {
return;
}
} }
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment