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

pratikvn · 2024-12-15T11:49:52Z

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.

upsj

LGTM! Format suggestions:

.cmake-format.json

upsj · 2024-12-15T12:17:55Z

.cmake-format.json

+      "foo": {
+        "flags": [
+          "BAR",
+          "BAZ"
+        ],
+        "kwargs": {
+          "HEADERS": "*",
+          "SOURCES": "*",
+          "DEPENDS": "*"
+        }
+      }
+    },


Here we should list the functions that use CMake argument parsing

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

yhmtsai · 2024-12-16T01:48:41Z

benchmark/CMakeLists.txt

+# \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


It destories the format

yhmtsai · 2024-12-16T01:49:05Z

.pre-commit-config.yaml

        include/ginkgo/ginkgo.hpp
      )
+- repo: https://github.com/cheshirekow/cmake-format-precommit
+  rev: 'v0.6.13' # The current latest release


I think it is kind of uncontinued project

yhmtsai · 2024-12-16T01:49:23Z

benchmark/CMakeLists.txt

+# \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


format issue

yhmtsai · 2024-12-16T01:50:04Z

cmake/CTestScript.cmake

+# 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


format issue

yhmtsai · 2024-12-16T01:51:22Z

cmake/CTestScript.cmake

-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"


it does not follow column limit

yhmtsai · 2024-12-16T01:54:10Z

cmake/build_helpers.cmake

+function(ginkgo_extract_dpcpp_version DPCPP_COMPILER GINKGO_DPCPP_VERSION
+         MACRO_VAR
+)


a bit weird to me

yhmtsai · 2024-12-16T01:54:57Z

cmake/build_helpers.cmake

+    set(${GINKGO_DPCPP_VERSION}
+        "${FOUND_DPCPP_VERSION}"
+        PARENT_SCOPE
+    )


it does not exceed 80 char

yhmtsai · 2024-12-16T01:56:40Z

cmake/build_type_helpers.cmake

+                    STATUS
+                        "Skipping ${_LANG}, not supported by build_type_helpers.cmake script"


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

yhmtsai · 2024-12-16T02:01:20Z

cmake/install_helpers.cmake

 #
-# @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


wrong format

yhmtsai · 2024-12-16T02:03:31Z

core/CMakeLists.txt

-# 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


wrong format

yhmtsai · 2025-02-13T11:45:19Z

alternative one is https://github.com/BlankSpruce/gersemi

pratikvn · 2025-02-13T14:26:52Z

Gersemi is used now, as it also seems to provide a decent setup and has pre-commit support, in addition to being active. The comments needed to be guarded with #gersemi: on/off, but otherwise, the default formatting seems to work well enough.

upsj · 2025-02-13T14:28:23Z

Is there a way to prevent that from happening? Comments are a pretty important and frequent part of the code, and other formatters manage to not mess them up that badly 😁

pratikvn · 2025-02-13T14:32:56Z

Not all comments, but only the ones where we try to use multi-line doxygen style documentation comments.

pratikvn · 2025-02-13T14:38:59Z

Turns out it is doing the right thing anyway, so disabling it with off/on is not necessary.

yhmtsai

It seems to preserve all string in ""
LGTM in general.
I still request changes just because I have some questions on that

yhmtsai · 2025-02-13T15:36:13Z

cmake/CTestScript.cmake

+        "-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}"
+    )


can't it follow the column limits?
more on the cmake question not formatter question

I think it does not break single argument into multiple lines. It was the same before as well, so I think it should be okay.

examples/external-lib-interfacing/CMakeLists.txt

third_party/identify_stream_usage/CMakeLists.txt

sonarqubecloud · 2025-02-19T02:39:54Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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 requested a review from a team December 15, 2024 11:49

pratikvn self-assigned this Dec 15, 2024

upsj approved these changes Dec 15, 2024

View reviewed changes

yhmtsai requested changes Dec 16, 2024

View reviewed changes

thoasm self-requested a review January 22, 2025 13:45

pratikvn force-pushed the cmake/format branch 2 times, most recently from 6bcf51a to bb1c362 Compare February 13, 2025 14:24

pratikvn requested a review from yhmtsai February 13, 2025 14:26

yhmtsai requested changes Feb 13, 2025

View reviewed changes

pratikvn requested a review from yhmtsai February 17, 2025 07:50

yhmtsai approved these changes Feb 17, 2025

View reviewed changes

pratikvn removed 1:ST:do-not-merge Please do not merge PR this yet. 1:ST:ready-for-review This PR is ready for review labels Feb 17, 2025

pratikvn added the 1:ST:ready-to-merge This PR is ready to merge. label Feb 17, 2025

pratikvn added 6 commits February 18, 2025 10:15

add cmake-format pre-commit setup

6bb1274

[cmake-format] add gersemi setup and pre-commit

0dde907

[cmake] format all cmake files

d4de3be

[cmake] disabling formatting not needed

bdaeaee

[cmake] format fixes

e0fd7bf

[cmake] format fixes

f37f975

pratikvn force-pushed the cmake/format branch from 9bde911 to f37f975 Compare February 18, 2025 09:22

pratikvn merged commit 289d8a4 into develop Feb 18, 2025
9 of 11 checks passed

pratikvn deleted the cmake/format branch February 18, 2025 12:00

		STATUS
		"Skipping ${_LANG}, not supported by build_type_helpers.cmake script"

		"-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}"
		)

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

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

Uh oh!

Conversation

pratikvn commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhmtsai commented Feb 13, 2025

Uh oh!

pratikvn commented Feb 13, 2025

Uh oh!

upsj commented Feb 13, 2025

Uh oh!

pratikvn commented Feb 13, 2025

Uh oh!

pratikvn commented Feb 13, 2025

Uh oh!

yhmtsai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 19, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pratikvn commented Dec 15, 2024 •

edited

Loading