Commit 7f4008c9 authored by Vihang Mehta's avatar Vihang Mehta
Browse files

Replace gazelle tester with linter

Summary:
The gazelle checker was run as an arcanist unit test. It would modify
and autofix the files underneath you but then send out the diff anyway causing
folks to have to `arc diff` again.

Instead, run gazelle in diff mode, and as an arcanist linter and send the diff
to arcanist so that `arc` will ask and apply the patch before sending the diff
out. (Sadly arcanist doesn't accept the patch format, it take a set of original
content plus new content, so generating the diff for arcanist is a bit ...
heavy handed.)

Test Plan:
Make some changes that would make gazelle unhappy. Run `arc lint`
{F127064}

Reviewers: zasgar, michelle

Reviewed By: zasgar

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

GitOrigin-RevId: b7523e14821ee959e4f1f3a24c49ea348512f711
parent 4ee03850
Showing with 44 additions and 80 deletions
+44 -80
......@@ -36,7 +36,7 @@
"(\\.bzl$)"
],
"script-and-regex.script": "./linters/buildifier.sh",
"script-and-regex.regex": "/^(?P<filename>.*):(?P<line>\\d+): (?P<error>.*): (?P<message>.*)$/m"
"script-and-regex.regex": "/^(?P<file>.*):(?P<line>\\d+): (?P<error>.*): (?P<message>.*)$/m"
},
"cpplint": {
"type": "cpplint",
......@@ -66,11 +66,20 @@
"(^experimental/.*BUILD\\.bazel$)"
],
"script-and-regex.script": "./linters/experimental-manual.sh",
"script-and-regex.regex": "/^(?P<filename>.*): (?P<message>.*)$/m"
"script-and-regex.regex": "/^(?P<file>.*): (?P<message>.*)$/m"
},
"filename": {
"type": "filename"
},
"gazelle": {
"type": "script-and-regex",
"include": [
"(BUILD\\.bazel$)",
"(\\.go$)"
],
"script-and-regex.script": "./linters/gazelle.sh",
"script-and-regex.regex": "/^(?P<severity>[[:alpha:]]+)\n(?P<file>[^\n]+)\n(?P<message>[^\n]+)\n((?P<line>\\d),(?P<char>\\d)\n<<<<<\n(?P<original>.*)=====\n(?P<replacement>.*)>>>>>\n)$/s"
},
"goimports": {
"type": "goimports",
"include": [
......
......@@ -19,7 +19,6 @@ phutil_register_library_map(array(
'ArcanistShellCheckLinter' => 'shellcheck/lint/ArcanistShellCheckLinter.php',
'ArcanistShellCheckLinterTestCase' => 'shellcheck/lint/__tests__/ArcanistShellCheckLinterTestCase.php',
'FileCheckerTestEngine' => 'pixielabs/unit/FileCheckerTestEngine.php',
'GazelleCheckerTestEngine' => 'pixielabs/unit/GazelleCheckerTestEngine.php',
'GoGenerateCheckerTestEngine' => 'pixielabs/unit/GoGenerateCheckerTestEngine.php',
'PLTestEngine' => 'pixielabs/unit/PLTestEngine.php',
),
......
......@@ -13,7 +13,6 @@ phutil_register_library_map(array(
'ArcanistGolangCiLinter' => 'lint/ArcanistGolangCiLinter.php',
'ArcanistProtoBreakCheckLinter' => 'lint/ArcanistProtoBreakCheckLinter.php',
'FileCheckerTestEngine' => 'unit/FileCheckerTestEngine.php',
'GazelleCheckerTestEngine' => 'unit/GazelleCheckerTestEngine.php',
'GoGenerateCheckerTestEngine' => 'unit/GoGenerateCheckerTestEngine.php',
'PLTestEngine' => 'unit/PLTestEngine.php',
),
......
<?php
final class GazelleCheckerTestEngine {
private $project_root;
private $files;
public function __construct($project_root, $files) {
$this->project_root = $project_root;
$this->files = $files;
}
public function run() {
chdir($this->project_root);
$test_results = array();
// Check if there are any modified go files.
$goFiles = array_filter($this->files, function($f) {
return substr($f, -3) == '.go';
});
// Check if there are any modified build files.
$buildFiles = array_filter($this->files, function($f) {
return substr($f, -11) == 'BUILD.bazel';
});
// If there are no modified go/build files, there is no need to check gazelle.
if (sizeof($goFiles) === 0 && sizeof($buildFiles) === 0) {
return $test_results;
}
$gazelle_build = new ArcanistUnitTestResult();
$gazelle_build->setName('Gazelle builds successfully');
exec('make gazelle 2>&1', $output, $return_var);
// Get the last line of the Gazelle run.
// This should say "Build completed successfully" if it built successfully.
$gazelle_status = $output[count($output) - 1];
if (strpos($gazelle_status, 'Build completed successfully') !== false) {
$gazelle_build->setResult(ArcanistUnitTestResult::RESULT_PASS);
} else {
$gazelle_build->setResult(ArcanistUnitTestResult::RESULT_FAIL);
$gazelle_build->setUserData('"make gazelle" did not build successfully: ' . $gazelle_status);
}
$test_results[] = $gazelle_build;
$gazelle_updated = new ArcanistUnitTestResult();
$gazelle_updated->setName('Gazelle updated');
// Checks to make sure there are no modified files after running gazelle.
//If there are, then the user needs to run gazelle and check in the updated files.
exec('git ls-files -m', $file_output, $return_var);
$updated = true;
foreach ($file_output as $file) {
if (strpos($file, 'BUILD.bazel') !== false) {
$updated = false;
break;
}
}
if ($updated == true) {
$gazelle_updated->setResult(ArcanistUnitTestResult::RESULT_PASS);
} else {
$gazelle_updated->setResult(ArcanistUnitTestResult::RESULT_FAIL);
$gazelle_updated->setUserData('Please run "make gazelle" and check in the updated build files.');
}
$test_results[] = $gazelle_updated;
return $test_results;
}
}
<?php
include 'FileCheckerTestEngine.php';
include 'GazelleCheckerTestEngine.php';
include 'GoGenerateCheckerTestEngine.php';
final class PLTestEngine extends ArcanistUnitTestEngine {
......@@ -18,9 +17,6 @@ final class PLTestEngine extends ArcanistUnitTestEngine {
$file_checker = new FileCheckerTestEngine($this->project_root, $this->files);
$test_results = array_merge($test_results, $file_checker->run());
$gazelle_checker = new GazelleCheckerTestEngine($this->project_root, $this->files);
$test_results = array_merge($test_results, $gazelle_checker->run());
// TODO(michelle): Determine a more robust check.
// $go_generate_checker = new GoGenerateCheckerTestEngine($this->project_root, $this->files);
// $test_results = array_merge($test_results, $go_generate_checker->run());
......
#!/bin/bash
tmpdir=$(mktemp -d)
patchFile="${tmpdir}/diff.patch"
builddir=$(dirname "$1")
if bazel run //:gazelle --noshow_progress -- --mode=diff "$builddir" 2>/dev/null 1>"${patchFile}"; then
exit 0
else
# Here be dragons, beware all who step here.
# gazelle responds with a diff in patch format, however afaik
# for some reason arcanist still doesn't accept patch formats.
# arcanist wants patches as the set of 4 pieces of info:
# {line, char, original_text, replacement_text}
# So we just patch the entire file, and hand arcanist the original, and new contents
# with line, char set to 1, 1.
fileToPatch=$(grep '^---' "$patchFile" | perl -pe 's/--- (.*)\t.*/\1/g')
correctedFileContents=$(patch "$fileToPatch" "$patchFile" -o- 2>/dev/null)
originalFileContents=$(cat "$fileToPatch")
# Careful, do not modify this output without also modifyng the matcher in .arclint
# The arcanist linter regex is sensitive to exact text and newlines.
echo "error" # severity
echo "$fileToPatch" # file
echo "Gazelle was not run on file" # message
echo "1,1" # line,char
echo "<<<<<"
echo "$originalFileContents" # original
echo "====="
echo "$correctedFileContents" # replacement
echo ">>>>>"
fi
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