Use pre-commit for style checks #336
Conversation
…n multiple file paths are passed.
| for cmake_file in "${@:2}"; do | ||
| cmake-format --in-place --first-comment-is-literal --config-files ${RAPIDS_CMAKE_FORMAT_FILE} ${RAPIDS_CMAKE_ROOT}/ci/checks/cmake_config_format.json -- ${cmake_file} |
There was a problem hiding this comment.
@vyasr I tried passing cmake-format ... -- ${@:2} like we do in cudf, but for some reason, it seems that cmake-format only respects the config files for the first input and not for later files.
I played with this locally and this appears to reproduce the failure:
# One file is fine
cmake-format --in-place --first-comment-is-literal --config-files cmake-format-rapids-cmake.json ci/checks/cmake_config_format.json -- rapids-cmake/rapids-cpm.cmake
# For multiple files, only the first file respects the configs
cmake-format --in-place --first-comment-is-literal --config-files cmake-format-rapids-cmake.json ci/checks/cmake_config_format.json -- rapids-cmake/rapids-cpm.cmake rapids-cmake/rapids-cuda.cmakeThis seems like a manifestation of https://github.com/cheshirekow/cmake_format/issues/284 because we're using --first-comment-is-literal in this repo but not in cudf, which handles multiple files just fine. If you have any further insight, feel free to share.
There was a problem hiding this comment.
I have not seen this before either. Your diagnosis seems reasonable, though, and as you say below switching to a direct invocation should bypass the issue.
|
|
||
| # Copyright (c) 2021-2023, NVIDIA CORPORATION. | ||
|
|
||
| # This script is a wrapper for cmakelang that may be used with pre-commit. The |
There was a problem hiding this comment.
Should we use the upstream pre-commit hook (https://github.com/cheshirekow/cmake-format-precommit) instead of copying the run-cmake-format.sh file, since this repo contains the original config file and it doesn't need to be fetched?
There was a problem hiding this comment.
I am happy to defer this to a follow up PR
There was a problem hiding this comment.
I think we should switch, yes. The fetch was the only reason I implemented the script in the first place. Hopefully switching to the hook will also address the issue you observed with --first-comment-is-literal.
There was a problem hiding this comment.
I attempted to switch to the official pre-commit hook, but failed. There is no option to require pre-commit to run one file at a time, so the use of --first-comment-is-literal means we always run into the bug mentioned above. We have to use an intermediate shell script to split the work and execute cmake-format on individual files, so I guess we're stuck with using run-cmake-format.sh.
bdice
added
improvement
|
|
||
| exit $RETVAL | ||
| # Run pre-commit checks | ||
| pre-commit run --all-files --show-diff-on-failure |
There was a problem hiding this comment.
I can't make a suggestion for this via the web interface, but can you remove the +e on line 16 of this file?
|
/merge |
Fixes rapidsai#336 Authors: - Robert Maynard (https://github.com/robertmaynard) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#158
Description
This PR adopts pre-commit for style checks in rapids-cmake. In addition to cmake-format and cmake-lint, this will help with frequently encountered PR review notes, like formatting C++ code, standardizing copyright notices, and catching typos via codespell.
For now, codespell and whitespace linting are disabled to reduce PR size.
Checklist
cmake-format.jsonis up to date with these changes.include_guard(GLOBAL))