Commit b2c0d072 authored by Vihang Mehta's avatar Vihang Mehta
Browse files

Covert proto break check detector into a linter

Summary:
We currently run prototool as a "unit test" for arcanist and run it
over the entire repo. Instead, let's run it as a linter. This means the checker
will run only on `.proto` files and one at a time instead of the entire repo.
This will also make lint messages show up inline in the diff.

Downgrade from error to warnings since we do sometimes make what would be
considered a breaking change (such as renaming a field) or removing an unused
field. So these are warnings and should be treated as such.

Test Plan:
Made some breaking changes and ran `arc lint`
{F127032}

Reviewers: zasgar, michelle

Reviewed By: michelle

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

GitOrigin-RevId: 70c4a66a0f233f39a14b2a8b177236dc157f5818
parent 9c491f7e
main snyk-fix-7eb526f73c75336b865418bda117e40b snyk-fix-a916784efb9d8870d42eeba63409bf85 release/vizier/vv0.9.8-pre-test.27 release/vizier/v0.11.7 release/vizier/v0.11.6 release/vizier/v0.11.5 release/vizier/v0.11.4 release/vizier/v0.11.3 release/vizier/v0.11.2 release/vizier/v0.11.1 release/vizier/v0.11.0 release/vizier/v0.10.22 release/vizier/v0.10.21 release/vizier/v0.10.20 release/vizier/v0.10.19 release/vizier/v0.10.18 release/vizier/v0.10.17 release/vizier/v0.10.16 release/vizier/v0.10.15 release/vizier/v0.10.14 release/vizier/v0.10.13 release/vizier/v0.10.12 release/vizier/v0.10.11 release/vizier/v0.10.10 release/vizier/v0.10.9 release/vizier/v0.10.8 release/vizier/v0.10.7 release/vizier/v0.10.6 release/vizier/v0.10.5 release/vizier/v0.10.4 release/vizier/v0.10.3 release/vizier/v0.10.2 release/vizier/v0.10.1 release/vizier/v0.10.0 release/vizier/v0.9.16 release/vizier/v0.9.14 release/vizier/v0.9.14-pre-main.5 release/vizier/v0.9.13 release/vizier/v0.9.13-pre-r0.17 release/vizier/v0.9.12 release/vizier/v0.9.12-pre-r0.1 release/vizier/v0.9.11 release/vizier/v0.9.11-pre-r0.32 release/vizier/v0.9.11-pre-r0.29 release/vizier/v0.9.11-pre-main.9 release/vizier/v0.9.8 release/vizier/v0.9.8-pre-test.27 release/vizier/v0.9.8-pre-test2.28 release/vizier/v0.9.7 release/vizier/v0.9.7-pre-r0.7 release/vizier/v0.9.7-pre-main.27 release/vizier/v0.9.6 release/vizier/v0.9.5 release/vizier/v0.9.5-pre-r0.13 release/vizier/v0.9.5-pre-main.18 release/vizier/v0.9.5-pre-main.17 release/vizier/v0.9.5-pre-main.14 release/vizier/v0.9.5-pre-etcdConfig.15 release/vizier/v0.9.4 release/vizier/v0.9.3 release/vizier/v0.9.2 release/vizier/v0.9.1 release/vizier/v0.9.0 release/vizier/v0.8.10 release/vizier/v0.8.9 release/vizier/v0.8.8 release/vizier/v0.8.7 release/vizier/v0.8.6 release/vizier/v0.8.5 release/vizier/v0.8.3 release/vizier/v0.8.2 release/vizier/v0.8.1 release/vizier/v0.8.0 release/vizier/v0.7.19 release/vizier/v0.7.18 release/vizier/v0.7.17 release/vizier/v0.7.16 release/vizier/v0.7.15 release/vizier/v0.7.14 release/vizier/v0.7.13 release/vizier/v0.7.12 release/vizier/v0.7.11 release/vizier/v0.7.10 release/vizier/v0.7.9 release/vizier/v0.7.8 release/vizier/v0.7.7 release/vizier/v0.7.6 release/vizier/v0.7.5 release/vizier/v0.7.4 release/vizier/v0.7.3 release/vizier/v0.7.2 release/vizier/v0.7.1 release/operator/v0.0.30 release/operator/v0.0.29 release/operator/v0.0.28 release/operator/v0.0.27 release/operator/v0.0.26 release/operator/v0.0.25 release/operator/v0.0.24 release/operator/v0.0.23 release/operator/v0.0.22 release/operator/v0.0.21 release/operator/v0.0.20 release/operator/v0.0.19 release/operator/v0.0.18 release/operator/v0.0.17 release/operator/v0.0.16 release/operator/v0.0.15 release/operator/v0.0.15-pre-r0.6 release/operator/v0.0.14 release/operator/v0.0.14-pre-zOperator3.8 release/operator/v0.0.14-pre-r0.8 release/operator/v0.0.13 release/operator/v0.0.13-pre-test.5 release/operator/v0.0.13-pre-main.5 release/operator/v0.0.12 release/operator/v0.0.12-pre-main.6 release/operator/v0.0.11 release/operator/v0.0.10 release/operator/v0.0.9 release/operator/v0.0.7 release/operator/v0.0.5 release/operator/v0.0.2 release/cloud/staging/1637355758 release/cloud/staging/1637266321 release/cloud/staging/1637083942 release/cloud/staging/1636658412 release/cloud/staging/1636655115 release/cloud/staging/1636566009 release/cloud/staging/1636486580 release/cloud/staging/1635273164 release/cloud/staging/1634664761 release/cloud/staging/1634662582 release/cloud/staging/1634426539 release/cloud/staging/1634255213 release/cloud/staging/1633889214 release/cloud/staging/1633467640 release/cloud/staging/1633377421 release/cloud/staging/1632931048 release/cloud/staging/1632880554 release/cloud/staging/1631813710 release/cloud/staging/1631205641 release/cloud/prod/1658198111 release/cloud/prod/1658185818 release/cloud/prod/1658183222 release/cloud/prod/1657740688 release/cloud/prod/1657049209 release/cloud/prod/1656629056 release/cloud/prod/1656527373 release/cloud/prod/1656452950 release/cloud/prod/1655997138 release/cloud/prod/1655226092 release/cloud/prod/1654806360 release/cloud/prod/1654144074 release/cloud/prod/1654133791 release/cloud/prod/1652313416 release/cloud/prod/1652304483 release/cloud/prod/1652214656 release/cloud/prod/1651864223 release/cloud/prod/1651799821 release/cloud/prod/1651704659 release/cloud/prod/1651616922 release/cloud/prod/1650645384 release/cloud/prod/1650480744 release/cloud/prod/1650306041 release/cloud/prod/1650056868 release/cloud/prod/1650039340 release/cloud/prod/1649978499 release/cloud/prod/1649797942 release/cloud/prod/1649787581 release/cloud/prod/1649269698 release/cloud/prod/1649107437 release/cloud/prod/1648586238 release/cloud/prod/1647992139 release/cloud/prod/1647379907 release/cloud/prod/1646182041 release/cloud/prod/1644961014 release/cloud/prod/1644348245 release/cloud/prod/1643849214 release/cloud/prod/1643826488 release/cloud/prod/1643153852 release/cloud/prod/1643056106 release/cloud/prod/1643052598 release/cloud/prod/1642705917 release/cloud/prod/1642632551 release/cloud/prod/1642205277 release/cloud/prod/1642145141 release/cloud/prod/1642141551 release/cloud/prod/1642139120 release/cloud/prod/1642134238 release/cloud/prod/1642130337 release/cloud/prod/1642126826 release/cloud/prod/1642124521 release/cloud/prod/1642109235 release/cloud/prod/1641941995 release/cloud/prod/1641420513 release/cloud/prod/1641254216 release/cloud/prod/1638917470 release/cloud/prod/1637096190 release/cloud/prod/1636492829 release/cloud/prod/1635286066 release/cloud/prod/1634668183 release/cloud/prod/1634663695 release/cloud/prod/1634282223 release/cloud/prod/1633893408 release/cloud/prod/1633710125 release/cloud/prod/1633560085 release/cloud/prod/1633495949 release/cloud/prod/1633474704 release/cloud/prod/1633379527 release/cloud/prod/1632935904 release/cloud/prod/1631826310 release/cloud/prod/1630737620 release/cloud/prod/1630714187 release/cloud/prod/1630622583 release/cloud/prod/1630433362 release/cloud/prod/1630110767 release/cloud/prod/1629952882 release/cloud/prod/1629920564 release/cloud/prod/1629851112 release/cloud/prod/1629494063 release/cloud/prod/1629339917 release/cloud/prod/1629337848 release/cloud/prod/1629335332 release/cloud/prod/1629311730 release/cloud/prod/1629166688 release/cloud/prod/1629166023 release/cloud/prod/1629165964 release/cloud/prod/1629151138 release/cloud/prod/1628025334 release/cloud/prod/1627971857 release/cloud/prod/1627943140 release/cloud/prod/1627935195 release/cloud/prod/1627644793 release/cloud/prod/1627601637 release/cloud/prod/1627445520 release/cloud/prod/1627421364 release/cloud/prod/1626909202 release/cloud/prod/1626824610 release/cloud/prod/1626391194 release/cloud/prod/1626321970 release/cloud/prod/1626073778 release/cloud/prod/1625699466 release/cloud/prod/1625253333 release/cloud/prod/1625249910 release/cloud/prod/1625013333 release/cloud/prod/1625003953 release/cloud/prod/1625001388 release/cloud/prod/1624993505 release/cloud/prod/1624989299 release/cloud/prod/1623456844 release/cloud/prod/1622779032 release/cloud/prod/1621989349 release/cloud/prod/1621986389 release/cloud/prod/1621964672 release/cloud/prod/1621920686 release/cloud/prod/1621917276 release/cloud/prod/1621882022 release/cloud/prod/1620958944 release/cloud/prod/1620945558 release/cloud/prod/1620327546 release/cloud/prod/1620273211 release/cloud/prod/1620271073 release/cloud/prod/1620090826 release/cloud/prod/1619828773 release/cloud/prod/1619741770 release/cloud/prod/1619126029 release/cloud/prod/1618639797 release/cloud/prod/1618522037 release/cli/v0.7.16 release/cli/v0.7.15 release/cli/v0.7.14 release/cli/v0.7.13 release/cli/v0.7.12 release/cli/v0.7.11 release/cli/v0.7.10 release/cli/v0.7.9 release/cli/v0.7.8 release/cli/v0.7.7 release/cli/v0.7.6 release/cli/v0.7.5 release/cli/v0.7.4 release/cli/v0.7.3 release/cli/v0.7.2 release/cli/v0.7.1 release/cli/v0.7.1-pre-r0.5 release/cli/v0.7.0 release/cli/v0.7.0-pre-main.11 release/cli/v0.6.8-pre-main.9 release/cli/v0.6.7 release/cli/v0.6.6 release/cli/v0.6.5 release/cli/v0.6.4 release/cli/v0.6.3 release/cli/v0.6.2 release/cli/v0.6.1 release/cli/v0.5.14 release/cli/v0.5.13 release/cli/v0.5.12 release/cli/v0.5.11 release/cli/v0.5.9 release/cli/v0.5.8 release/cli/v0.5.7 release/cli/v0.5.6 release/cli/v0.5.5 release/cli/v0.5.4 release/cli/v0.5.3 release/cli/v0.5.2 release/cli/v0.5.1
No related merge requests found
Showing with 78 additions and 53 deletions
+78 -53
......@@ -138,6 +138,15 @@
"(^arc_addons/.*\\.php$)"
]
},
"proto-break-check": {
"type": "proto-break-check",
"include": [
"(.*\\.proto$)"
],
"exclude": [
"(^src/stirling/source_connectors/dynamic_tracer/dynamic_tracing/ir/physicalpb/physical\\.proto$)"
]
},
"spelling": {
"type": "spelling",
"exclude": [
......
......@@ -15,6 +15,7 @@ phutil_register_library_map(array(
'ArcanistGoImportsLinter' => 'pixielabs/lint/ArcanistGoImportsLinter.php',
'ArcanistGoVetLinter' => 'golang/lint/ArcanistGoVetLinter.php',
'ArcanistGolangCiLinter' => 'pixielabs/lint/ArcanistGolangCiLinter.php',
'ArcanistProtoBreakCheckLinter' => 'pixielabs/lint/ArcanistProtoBreakCheckLinter.php',
'ArcanistShellCheckLinter' => 'shellcheck/lint/ArcanistShellCheckLinter.php',
'ArcanistShellCheckLinterTestCase' => 'shellcheck/lint/__tests__/ArcanistShellCheckLinterTestCase.php',
'ExpCheckerTestEngine' => 'pixielabs/unit/ExpCheckerTestEngine.php',
......@@ -22,7 +23,6 @@ phutil_register_library_map(array(
'GazelleCheckerTestEngine' => 'pixielabs/unit/GazelleCheckerTestEngine.php',
'GoGenerateCheckerTestEngine' => 'pixielabs/unit/GoGenerateCheckerTestEngine.php',
'PLTestEngine' => 'pixielabs/unit/PLTestEngine.php',
'PrototoolCheckerTestEngine' => 'pixielabs/unit/PrototoolCheckerTestEngine.php',
),
'function' => array(),
'xmap' => array(
......@@ -32,6 +32,7 @@ phutil_register_library_map(array(
'ArcanistGoImportsLinter' => 'ArcanistExternalLinter',
'ArcanistGoVetLinter' => 'ArcanistExternalLinter',
'ArcanistGolangCiLinter' => 'ArcanistExternalLinter',
'ArcanistProtoBreakCheckLinter' => 'ArcanistExternalLinter',
'ArcanistShellCheckLinter' => 'ArcanistExternalLinter',
'ArcanistShellCheckLinterTestCase' => 'PhutilTestCase',
'PLTestEngine' => 'ArcanistUnitTestEngine',
......
......@@ -11,17 +11,18 @@ phutil_register_library_map(array(
'class' => array(
'ArcanistGoImportsLinter' => 'lint/ArcanistGoImportsLinter.php',
'ArcanistGolangCiLinter' => 'lint/ArcanistGolangCiLinter.php',
'ArcanistProtoBreakCheckLinter' => 'lint/ArcanistProtoBreakCheckLinter.php',
'ExpCheckerTestEngine' => 'unit/ExpCheckerTestEngine.php',
'FileCheckerTestEngine' => 'unit/FileCheckerTestEngine.php',
'GazelleCheckerTestEngine' => 'unit/GazelleCheckerTestEngine.php',
'GoGenerateCheckerTestEngine' => 'unit/GoGenerateCheckerTestEngine.php',
'PLTestEngine' => 'unit/PLTestEngine.php',
'PrototoolCheckerTestEngine' => 'unit/PrototoolCheckerTestEngine.php',
),
'function' => array(),
'xmap' => array(
'ArcanistGoImportsLinter' => 'ArcanistExternalLinter',
'ArcanistGolangCiLinter' => 'ArcanistExternalLinter',
'ArcanistProtoBreakCheckLinter' => 'ArcanistExternalLinter',
'PLTestEngine' => 'ArcanistUnitTestEngine',
),
));
<?php
final class ArcanistProtoBreakCheckLinter extends ArcanistExternalLinter {
public function getInfoName() {
return 'proto-break-check';
}
public function getInfoURI() {
return 'https://github.com/uber/prototool#prototool-break-check';
}
public function getInfoDescription() {
return 'proto-break-check looks for backward incompatible protobuf changes';
}
public function getLinterName() {
return 'proto-break-check';
}
public function getLinterConfigurationName() {
return 'proto-break-check';
}
public function getDefaultBinary() {
return 'prototool';
}
public function getInstallInstructions() {
return 'Install from github releases '.
'https://github.com/uber/prototool/releases';
}
protected function getMandatoryFlags() {
return array('--git-branch=main');
}
protected function buildFutures(array $paths) {
$executable = $this->getExecutableCommand();
$flags = $this->getCommandFlags();
$futures = array();
foreach ($paths as $path) {
$future = new ExecFuture('%C break check %Ls %s', $executable, $flags, $path);
$futures[$path] = $future;
}
return $futures;
}
protected function parseLinterOutput($path, $err, $stdout, $stderr) {
$messages = array();
if ($err !== 0) {
$message = id(new ArcanistLintMessage())
->setPath($path)
->setCode('breaking-change')
->setName('prototool')
->setDescription($stdout)
->setSeverity(ArcanistLintSeverity::SEVERITY_WARNING);
$messages[] = $message;
}
return $messages;
}
}
......@@ -4,7 +4,6 @@ include 'FileCheckerTestEngine.php';
include 'GazelleCheckerTestEngine.php';
include 'ExpCheckerTestEngine.php';
include 'GoGenerateCheckerTestEngine.php';
include 'PrototoolCheckerTestEngine.php';
final class PLTestEngine extends ArcanistUnitTestEngine {
private $project_root;
......@@ -30,9 +29,6 @@ final class PLTestEngine extends ArcanistUnitTestEngine {
// $go_generate_checker = new GoGenerateCheckerTestEngine($this->project_root, $this->files);
// $test_results = array_merge($test_results, $go_generate_checker->run());
$prototool_checker = new PrototoolCheckerTestEngine($this->project_root, $this->files);
$test_results = array_merge($test_results, $prototool_checker->run());
return $test_results;
}
}
<?php
final class PrototoolCheckerTestEngine {
private $project_root;
private $files;
public function __construct($project_root, $files) {
$this->project_root = $project_root;
$this->files = $files;
}
public function run() {
$test_results = array();
chdir($this->project_root);
// Skip prototool if no .proto files are present, since prototool is time-consuming.
$protoFiles = array_filter($this->files, function($f) {
return substr($f, -6) == '.proto';
});
if (count($protoFiles) == 0) {
return $test_results;
}
$output = array();
exec('prototool break check . --git-branch main --walk-timeout 10s', $output, $return_var);
$updatedRes = new ArcanistUnitTestResult();
$updatedRes->setName('Protobuf Breaking API check');
if ($return_var == 0) {
$updatedRes->setResult(ArcanistUnitTestResult::RESULT_PASS);
} else {
$updatedRes->setUserData(implode("\n", $output));
$updatedRes->setResult(ArcanistUnitTestResult::RESULT_FAIL);
}
$test_results[] = $updatedRes;
return $test_results;
}
}
---
excludes:
- vendor
- third_party
- chef
- bazel-bin
- bazel-out
- bazel-pixielabs
- bazel-testlogs
- docs
- src/ui
# Exclude stirling's dynamic tracing physical IR, since it is not a public proto.
- src/stirling/source_connectors/dynamic_tracer/dynamic_tracing/ir/physicalpb
- third_party
protoc:
version: 3.8.0
includes:
......
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