Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cmake-format, and a pre-commit hook for it #1755

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

pratikvn
Copy link
Member

@pratikvn pratikvn commented Dec 15, 2024

This PR adds a pre-commit hook to format CMakeLists,txt files. The actual format can still be discussed. But currently, I just formatted all CMakeLists.txt files with the default style. If necessary, we can adjust the style in the .cmake-format.json file.

Unfortunately, as develop does not have cmake-format hook, the current format job will not run the cmake-format pre-commit hook yet.

run with command
`git ls-files -- '*CMakeLists.txt' | xargs pre-commit run --files`
@pratikvn pratikvn added 1:ST:do-not-merge Please do not merge PR this yet. 1:ST:ready-for-review This PR is ready for review labels Dec 15, 2024
@pratikvn pratikvn requested a review from a team December 15, 2024 11:49
@pratikvn pratikvn self-assigned this Dec 15, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:reordering This is related to the matrix(LinOp) reordering mod:all This touches all Ginkgo modules. labels Dec 15, 2024
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Format suggestions:

.cmake-format.json Outdated Show resolved Hide resolved
.cmake-format.json Outdated Show resolved Hide resolved
Comment on lines 8 to 19
"foo": {
"flags": [
"BAR",
"BAZ"
],
"kwargs": {
"HEADERS": "*",
"SOURCES": "*",
"DEPENDS": "*"
}
}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should list the functions that use CMake argument parsing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I meant mostly uses of cmake_parse_arguments, where the formatting tool can otherwise not differentiate between flags/kwargs and regular arguments

Comment on lines +83 to +87
# \param name name for the executable to create (including type
# suffix) \param use_lib_linops Boolean indicating if linking against
# hipsparse/cusparse is necessary \param macro_def preprocessor macro name
# that will be defined during building (to compile for a specific type) All
# remaining arguments will be treated as source files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It destories the format

@@ -14,6 +14,12 @@ repos:
third_party/identify_stream_usage/.*|
include/ginkgo/ginkgo.hpp
)
- repo: https://github.com/cheshirekow/cmake-format-precommit
rev: 'v0.6.13' # The current latest release
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is kind of uncontinued project

Comment on lines +131 to +133
# \param name base-name for the executable to create \param
# use_lib_linops Boolean indicating if linking against hipsparse/cusparse is
# necessary All remaining arguments will be treated as source files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format issue

Comment on lines +7 to +10
# dashboard. The supported runs are: + With or without coverage, requires the
# gcov tool. + With or without address sanitizers. + With or without memory
# sanitizers. + With or without thread sanitizers. + With or without leak
# sanitizers. + With or without undefined behavior (UB) sanitizers. + With or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format issue

elseif((NOT CTEST_MEMORYCHECK_TYPE STREQUAL "NONE" AND NOT CTEST_MEMORYCHECK_TYPE STREQUAL "Valgrind") OR CTEST_BUILD_CONFIGURATION STREQUAL "COVERAGE")
set(GINKGO_CONFIGURE_OPTIONS "-DGINKGO_DEVEL_TOOLS=OFF;-DGINKGO_BUILD_REFERENCE=ON;-DGINKGO_BUILD_OMP=ON;-DGINKGO_BUILD_CUDA=OFF;-DGINKGO_BUILD_HIP=OFF;-DGINKGO_BUILD_SYCL=OFF;-DCMAKE_BUILD_TYPE=${CTEST_BUILD_CONFIGURATION}")
set(GINKGO_CONFIGURE_OPTIONS
"-DGINKGO_DEVEL_TOOLS=OFF;-DGINKGO_BUILD_REFERENCE=ON;-DGINKGO_BUILD_OMP=OFF;-DGINKGO_BUILD_CUDA=ON;-DGINKGO_BUILD_HIP=OFF;-DGINKGO_BUILD_SYCL=OFF;-DCMAKE_BUILD_TYPE=${CTEST_BUILD_CONFIGURATION};-DCMAKE_CUDA_FLAGS=-lineinfo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does not follow column limit

Comment on lines +178 to +180
function(ginkgo_extract_dpcpp_version DPCPP_COMPILER GINKGO_DPCPP_VERSION
MACRO_VAR
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit weird to me

Comment on lines +202 to +205
set(${GINKGO_DPCPP_VERSION}
"${FOUND_DPCPP_VERSION}"
PARENT_SCOPE
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does not exceed 80 char

Comment on lines +149 to +150
STATUS
"Skipping ${_LANG}, not supported by build_type_helpers.cmake script"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after STATUS, it has one more indentation, but it does not do that after STRING in line 160

@@ -17,102 +16,116 @@ set(GINKGO_INSTALL_MODULE_DIR "${CMAKE_INSTALL_LIBDIR}/cmake/Ginkgo/Modules")
# 3. GINKGO_INSTALL_RPATH_DEPENDENCIES : Allows adding any extra paths to the
# RPATH.
#
# @param name the name of the target
# @param ARGN any external dependencies path to be added
# @param name the name of the target @param ARGN any external dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong format

# We separate the library as a workaround to solve this issue
# To make ginkgo still be the major library, we make the original to ginkgo_core in MSVC/shared
# TODO: should think another way to solve it like dllexport or def file
# MSVC: LNK1189 issue CLANG in MSYS2 (MINGW): too many exported symbols We
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:do-not-merge Please do not merge PR this yet. 1:ST:ready-for-review This PR is ready for review mod:all This touches all Ginkgo modules. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:testing This is related to testing. type:matrix-format This is related to the Matrix formats type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants