Skip to content

Commit

Permalink
Enforce clang-format version 7.x
Browse files Browse the repository at this point in the history
Summary:
`llvm` 8.0 and 7.1 have been released recently.

As an example, Ubuntu recently updated to clang-format 7.1, causing
arcanist to return an error on `arc lint` due to version conflict.

I tested our codebase against clang-format 7.1 and 8.0, there is
a minor difference in the output (some edge cases are causing
bad indents with clang-format-7, which is fixed by clang-format-8).

Therefore both versions cannot be allowed at the same time
as it would cause `arc lint` to format back and forth when
using different versions.

I would have preferred to enforce >= 7.0 and < 8 in the
`.arclint`file but this is not supported by arcanist.
As a workaround an additional check is performed at the linter
level to enforce using version 7.x only.

Test Plan:
With `clang-format` 7.0, 7.1 and 8.0:
  arc lint --everything

Reviewers: #bitcoin_abc, deadalnix, jasonbcox

Reviewed By: #bitcoin_abc, deadalnix, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D3806
  • Loading branch information
Fabcien committed Aug 8, 2019
1 parent 4391a2e commit b44d73a
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 2 deletions.
2 changes: 1 addition & 1 deletion .arclint
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
},
"clang-format": {
"type": "clang-format",
"version": "7.0",
"version": ">=7.0",
"bin": ["clang-format-7", "clang-format"],
"include": "(^src/.*\\.(h|c|cpp|mm)$)",
"exclude": [
Expand Down
17 changes: 16 additions & 1 deletion arcanist/linter/ClangFormatLinter.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,25 @@ public function getVersion() {
$matches = array();
$regex = '/^clang-format version (?P<version>\d+\.\d+)\./';
if (preg_match($regex, $stdout, $matches)) {
return $matches['version'];
$version = $matches['version'];
} else {
return false;
}

/*
* FIXME: This is a hack to only allow for clang-format version 7.x.
* The .arclint `version` field only allow to filter versions using `=`,
* `>`, `<`, `>=` or `<=`. There is no facility to define that the required
* version should be >= 7.0 and < 8.0.
*/
if ($version[0] != '7') {
throw new Exception(pht('Linter %s requires clang-format version 7.x. '.
'You have version %s.',
ClangFormatLinter::class,
$version));
}

return $version;
}

public function getInstallInstructions() {
Expand Down
2 changes: 2 additions & 0 deletions arcanist/phpcs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
<exclude name="Generic.Commenting.DocComment.ParamNotFirst" />
<exclude name="Generic.Commenting.DocComment.SpacingBeforeTags" />
<exclude name="Generic.Commenting.DocComment.TagValueIndent" />
<exclude name="Generic.Commenting.Fixme.TaskFound" />
<exclude name="Generic.Commenting.Todo.TaskFound" />
<exclude name="Generic.Files.EndFileNoNewline.Found" />
<exclude name="Generic.Files.LowercasedFilename" />
<exclude name="Generic.Formatting.MultipleStatementAlignment" />
Expand Down

0 comments on commit b44d73a

Please sign in to comment.