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
Created attachment 177977 [details] Patch
Created attachment 177981 [details] Patch
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 ?
Created attachment 178187 [details] Patch
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.
(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.
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.
Created attachment 178313 [details] Patch
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?
Created attachment 178353 [details] Patch
(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)
Comment on attachment 178353 [details] Patch This seems like a totally reasonable.
Comment on attachment 178353 [details] Patch Clearing flags on attachment: 178353 Committed r137057: <http://trac.webkit.org/changeset/137057>
All reviewed patches have been landed. Closing bug.
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.