rnp
rnp copied to clipboard
rnpgp
difference in clang-format versions pre-commit script and CI
The CI currently seems check the code format with clang-format 11.0.0. But in the pre-commit Hook in current master, version 11.1.0 is mandated. I experienced already differences in two files, rather likely because of this difference in versions.
Which version of clang-format is the correct one and should be used for RNP development?
We should probably migrate all the pre-commit hooks into CI hooks.
Any thoughts @ni4 @antonsviridenko @maxirmx ?
Version 11.0 should be used, pre-commit hook should be fixed to v11.0.
Could you please also specify the patch version? It seems to me that even v11.0.0 and v11.0.1 are showing differences. Will we use 11.0.0 as configured in the CI?
We should probably migrate all the pre-commit hooks into CI hooks.
To be honest I never used pre-commit hook and run clang-format manually via VS Code build tasks. Probably just fixing clang-format version to 11.0.0, or moving it to some include so it may be reused by CI and pre-commit hook, would be enough in this case.
@falko-strenzke correct version is one from the CI - 11.0.0, I'll make a PR for consolidating these things now.
I feel we should just get rid of the pre-commit hooks and change them into CI jobs... Personally I dislike the scripts from a repo assuming some environment setup locally.
Since we already have the clang-format lint job in CI, I don't see why we need another one for pre-commit.
We only have one pre-commit hook which is for clang-format:
- https://github.com/rnpgp/rnp/blob/main/git-hooks/pre-commit.sh
@ni4 can we just remove the pre-commit hook scripts from the repository entirely in #2093 ?
I think it would still be useful to provide some means for a developer to quickly check and fix the code formatting. But the problem is indeed the version of clang-format affecting the formatting. I also observed that the pre-commit hook provided in RNP did not work reliable. At first at worked, then the next day falsely formatted code slipped through.
We are working now with a pre-commit hook based on this script. The scripts are checked into this branch in our fork. This is not an optimal solution since it always checks all files in src/ and include/, irrespectively of whether they are part of the commit. But it's simple and (hopefully) reliable.
Another approch would be to optionally integrate the format check into the build system as build target. We have done that with CMake in one of our own projects and it works fine. Should that be interesting to you, we can provide the CMake code for that.
Another approch would be to optionally integrate the format check into the build system as build target. We have done that with CMake in one of our own projects and it works fine. Should that be interesting to you, we can provide the CMake code for that.
sounds interesting, I think it would be nice to have
Is it normal that minor versions of clang-format have changes in enforced code style? Maybe there is a way to impose some long-term stable code formatting?
@falko-strenzke @antonsviridenko I think CMake integration could be a good solution.
There are people who tried using clang-format and clang-tidy with CMake:
- https://itnext.io/save-your-sanity-and-time-beyond-clang-format-2b929b9120b8
- https://www.danielsieger.com/blog/2021/12/21/clang-tidy-cmake.html
- https://discourse.cmake.org/t/clang-format-integration/3358/7
Another approch would be to optionally integrate the format check into the build system as build target. We have done that with CMake in one of our own projects and it works fine. Should that be interesting to you, we can provide the CMake code for that.
sounds interesting, I think it would be nice to have
This is the code for adding one formatting target for manual invocation and one check target for automatic invocation after all other build targets:
SET(CLANG_FORMAT_BINARY "${CMAKE_SOURCE_DIR}/misc/tools/clang-format" CACHE STRING "Path to clang binary")
option(DISABLE_CLANG_FORMAT_CHECK "disable clang format target" OFF)
# get all project files
file(GLOB_RECURSE ALL_SOURCE_FILES *.cpp *.h)
foreach (SOURCE_FILE ${ALL_SOURCE_FILES})
string(FIND ${SOURCE_FILE} "ext_lib" PROJECT_TRDPARTY_DIR_FOUND) # search for substring in SOURCE_FILE, last arg is result set to -1 if not found
string(FIND ${SOURCE_FILE} "${CMAKE_BINARY_DIR}/CMakeFiles/" BUILD_DIR_FOUND) # exclude also code under the build directory
if (NOT ${PROJECT_TRDPARTY_DIR_FOUND} EQUAL -1 OR NOT ${BUILD_DIR_FOUND} EQUAL -1)
MESSAGE("removing item from code formatting list" ${SOURCE_FILE})
list(REMOVE_ITEM ALL_SOURCE_FILES ${SOURCE_FILE})
endif ()
endforeach ()
foreach (SOURCE_FILE ${ALL_SOURCE_FILES})
MESSAGE("source file for fomatting= " ${SOURCE_FILE})
endforeach ()
# add a target for formatting all source files
add_custom_target(
clang-format-do
COMMAND ${CLANG_FORMAT_BINARY}
-style=file
-i
${ALL_SOURCE_FILES}
)
if(DISABLE_CLANG_FORMAT_CHECK STREQUAL OFF)
# add a target that is executed after all other targets
add_custom_target(
clang-format-check
ALL
DEPENDS <... all other relevant default build targets ...>
COMMAND ${CLANG_FORMAT_BINARY}
--dry-run
--Werror
-style=file
${ALL_SOURCE_FILES}
)
endif()
And yest another approach is to have clang formatting implemented at GHA
Something like this:
clang-format:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: DoozyX/clang-format-lint-action@master
with:
source: '.'
extensions: 'h,cpp,c'
clangFormatVersion: 14
inplace: True
- name: Commit changes
uses: EndBug/add-and-commit@v9
with:
message: clang-format fixes
committer_name: GitHub Actions
committer_email: [email protected]
@maxirmx in my workflow it was easier and more clean (no clang-format dedicated commits) to configure clang-format job in VS Code, and then, in case of need, run it and rebase.