Bug 104240 – [CMake] Add CMake style checker

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Halton Huo]

Refer to bug #103605, the cmake files(CMakeLists.txt and *.cmake) are unified by rules that we agreed, this bug is to add CMake style checker

[Halton Huo]

Created attachment 177977 [details]
Patch

[Halton Huo]

Created attachment 177981 [details]
Patch

[Laszlo Gombos]

Comment on attachment 177981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177981&action=review

I think the style checker would need some unittests just like the existing checkers. 

It would be great if someone else with more experience in the style checkers could help the review. Overall the direction looks good to me.

> Tools/Scripts/webkitpy/style/checkers/cmake.py:31
> +

Extra line ?

> Tools/Scripts/webkitpy/style/checkers/cmake.py:41
> +        self._no_space_cmds = self._read_file_ignore_comments(
> +                  os.path.join(basedir, "cmake-commands-no-space.txt"))
> +        self._one_space_cmds = self._read_file_ignore_comments(
> +                  os.path.join(basedir, "cmake-commands-one-space.txt"))

I think these lists should be in the cmake.py file as that is more consistent with the existent checkers.

> Tools/Scripts/webkitpy/style/checkers/cmake.py:55
> +            self._handle_style_error(line_number, 'whitespace/tailing', 5,
> +                                     'No tailing spaces')

tailing -> trailing

> Tools/Scripts/webkitpy/style/checkers/cmake.py:86
> +            if re.search('(^|\ +)' + t.upper() + '\ *\(', line_content):
> +                msg = 'Use lowercase command "' + t + '" instead of "' + t.upper() + '"'

What if someone would use mixed-case - e.g. "If" ? It seems to me that this rule would not catch it.

> Tools/Scripts/webkitpy/style/checkers/cmake.py:88
> +            if re.search('(^|\ +)' + t + '\ +\(', line_content):

Wouldn't t.lower() be better here ?

> Tools/Scripts/webkitpy/style/checkers/cmake.py:96
> +            if re.search('(^|\ +)' + t.upper() + '\ *\(', line_content):
> +                msg = 'Use lowercase command "' + t + '" instead of "' + t.upper() + '"'

Ditto - What if someone would use mixed-case - e.g. "If" ? It seems to me that this rule would not catch it.

Can this rule somehow shared, instead of repeated ?

> Tools/Scripts/webkitpy/style/checkers/cmake.py:98
> +            if re.search('(^|\ +)' + t + '(\(|\ \ +\()', line_content):

Ditto - Wouldn't t.lower() be better here ?

[Halton Huo]

Created attachment 178187 [details]
Patch

[WebKit Review Bot]

Attachment 178187 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:3:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:2:  Use lowercase command "if"  [command/lowercase] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:4:  No trailing spaces  [whitespace/trailing] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:5:  No space after "("  [whitespace/parentheses] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:6:  No space before ")"  [whitespace/parentheses] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:7:  Use lowercase command "message"  [command/lowercase] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:8:  Use lowercase command "message"  [command/lowercase] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:10:  Use lowercase command "endif"  [command/lowercase] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:12:  One space between command "if" and its parentheses, should be "if ("  [whitespace/parentheses] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:15:  No space between command "macro" and its parentheses, should be "macro("  [whitespace/parentheses] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:16:  Use lowercase command "endmacro"  [command/lowercase] [5]
Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:18:  No space between command "function" and its parentheses, should be "function("  [whitespace/parentheses] [5]
Total errors found: 12 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Halton Huo]

(In reply to comment #3)
> > Tools/Scripts/webkitpy/style/checkers/cmake.py:31
> > +
> 
> Extra line ?
Are you suggesting to leave 3 empty line here? python checker will report error. I check with python.py, it is 2 empty lines.

All issues are address in new patch #178187 except the above one.

[Laszlo Gombos]

Comment on attachment 178187 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178187&action=review

Almost there, but I think the patch itself should pass the style check; r- for that.

> Tools/Scripts/webkitpy/style/checkers/cmake_unittest_input.cmake:1
> +# This file is sample input for cmake_unittest.py and includes below problems:

It would be better to include the test input inside cmake_unittest.py and not as a separate file - not necessary as one big buffer but as a serious of unit tests like it is done for the other checkers (see for example changelog_unittest.py). This would also avoid the problem that your negative tests in the cmake_unittest*.cmake fail on the style check.

[Halton Huo]

Created attachment 178313 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 178313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178313&action=review

> Tools/Scripts/webkitpy/style/checker.py:581
> +        elif ((file_extension == _CMAKE_FILE_EXTENSION) or os.path.basename(file_path) == 'CMakeLists.txt'):

Some day we should move all this code to use the webkitpy.common.system abstractions (like FileSystem) to make this code more easily unittestable.  (This is unrelated to your patch.)

> Tools/Scripts/webkitpy/style/checkers/cmake.py:29
> +import os
> +import re
> +import sys

os and sys seem unused?

[Halton Huo]

Created attachment 178353 [details]
Patch

[Halton Huo]

(In reply to comment #10)
> Created an attachment (id=178353) [details]
> Patch
Rebase to trunk@137030 and remove unused import os and sys(Eric's comment #9)

[Eric Seidel (no email)]

Comment on attachment 178353 [details]
Patch

This seems like a totally reasonable.

[WebKit Review Bot]

Comment on attachment 178353 [details]
Patch

Clearing flags on attachment: 178353

Committed r137057: <http://trac.webkit.org/changeset/137057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Halton Huo]

Just let you know, I run check-webkit-style at trunk@137102(By adding a empty line for all CMakeLists.txt and .cmake files), no errors found now. So only one thing left is my TODO for indent checking.