Add CMake-format pre-commit hooks and linters #771
Conversation
facebook-github-bot
added
the
CLA Signed
|
Also wasn't sure if I should standardize to tabs of space 2 or tabs space size 4, thoughts? I noticed @mosra cmake seem to use tabs of space 4, so I went with that. |
|
Since I expect this to be controversial, we can set up a configuration like so. Suggestions welcome: https://cmake-format.readthedocs.io/en/latest/index.html |
|
This is a very interesting PR. I like it. It makes the CMake related files nice and clean. Any thoughts? @other reviewers. |
src/cmake/dependencies.cmake
Outdated
| # for slightly faster builds. If you need any of these, simply delete a line. | ||
| set(WITH_INTERCONNECT | ||
| OFF | ||
| CACHE BOOL "" FORCE) |
There was a problem hiding this comment.
I wonder what's the logic behind these aggressive line breaks for set arguments.
There was a problem hiding this comment.
@eundersander explained in the docs: https://cmake-format.readthedocs.io/en/latest/format-case_studies.html
There was a problem hiding this comment.
It looks like this style is to accommodate comments on each argument, which makes sense.
There was a problem hiding this comment.
I could also give it control over formatting the cmake comments, but have opted not to for the first run.
src/esp/sensor/CMakeLists.txt
Outdated
| gfx | ||
| scene | ||
| ) | ||
| target_link_libraries(sensor PUBLIC core gfx scene) |
There was a problem hiding this comment.
I'm not a fan of packing these onto one line. It's easier to notice additions/deletions in diffs when each list item is its own line.
There was a problem hiding this comment.
They only get packed onto one line if they are three or less arguments. I already lowered the threshold from the default of six.
There was a problem hiding this comment.
Is there some middle ground between "aggressively packing everything with < 4 arguments onto a single line" and "aggressively expanding everything with >= 4 arguments onto several lines"? Something like "always expand target_link_libraries()" or "preserve whatever format was there for certain commands"?
Like, in this case I would prefer to have the args separated on multiple lines because it's very likely more libs will get added later, however with the set(thing CACHE BOOL "bla" OFF) it'll never happen that there will be more arguments and spreading it out like that just adds unnecessary congitive load as the WITH_ options etc are no longer nicely aligned under each other.
There was a problem hiding this comment.
@mosra There is, is there anything we should expand besides target_link_libraries?
There was a problem hiding this comment.
I thought about doing something similar to the set(bla_SOURCES many things here) commands but most of those lists could be put directly into add_library() / add_executable() ... and conditional sources later added with target_sources().
So maybe add add_executable() and target_sources() to the list as well, even though the latter is probably not used anywhere yet.
There was a problem hiding this comment.
We can just make a note to add those later once we use those features.
src/cmake/dependencies.cmake
Outdated
| set(WITH_GLFWAPPLICATION OFF CACHE BOOL "WITH_GLFWAPPLICATION" FORCE) | ||
| set(WITH_EIGEN ON CACHE BOOL "WITH_EIGEN" FORCE) # Eigen integration | ||
| set(WITH_IMGUI ON CACHE BOOL "WITH_IMGUI" FORCE) # ImGui integration | ||
| set(WITH_TINYGLTFIMPORTER |
There was a problem hiding this comment.
@erikwijmans It's expressions like this that make prefer the default limit of 6 args per line instead of 3.
There was a problem hiding this comment.
General comment: I personally do not like autoformatters because the tool is never smart enough to actually preserve readability instead of just applying rigid rules (and making things worse in many cases).
But this is not my code, so please do not take my opinion too seriously ;)
|
@mosra It can be disabled at any point if it really impacts readability. :) |
* Make all ipynb work locally * Add path classifiers to .lgtm * Remove unncessary glob * Update black and lint repo * Sort yaml * Enable flake8-bugbear plugin * Fix a few more flake8-bugbear errors * Attempt to add pre-commit hooks to .circleci * Add job to .circleci * Add system dependencies for pre-commit * Skip system hooks * fix typo * Fix .circleci * Black notebooks * Remove unused variable and fix clang-format * Remove another unused variable * Fix bugs in AttributeManager * Fix set comprehensions * Fix EOF * revert sim bindings * Fix true false errors * Ignore C408 * Ignore C402 and C403 * Meant C401 and C402 * Add cmake-format pre-commit hook and cicheck * Commit cmake-format config * Fix typos and add missing cmake-formats * Update cmake-format to ignore Find CMake files * Adjust cmake-format settings * Simplify cmake-format to better match our preferences. * Clean up .pre-commit and cmake-format * enable autosort cmake-format feature * Attempt to fix pre-commit hooks * Changed config to mosra's specification * always wrap add_library too * reduce min indent to two * always wrap find_package * revert find_package * final config tweaks
Motivation and Context
It ensures we enforce a consistent style across the cmake files in our repo. For the first round, I am only having it handle the code, and not handle any of the formatting of comments. This will also add .circleci check that ensures cmake edits confirm thanks to .circleci. It would also automatically format cmakefiles during commit. Let me know if I should further customize the style file any further.
How Has This Been Tested
I reformatted the code and all the build configs seemed to build just fine.
Types of changes
It adds a new linter. I am also going to do a big spellchecker PR that should fix all the misspellings in comments and code.
Checklist