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

fix cmake: Fix identation #638

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

idzm
Copy link
Contributor

@idzm idzm commented Jul 2, 2024

No description provided.

Copy link

github-actions bot commented Jul 2, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@idzm
Copy link
Contributor Author

idzm commented Jul 4, 2024

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/

@apolukhin
Copy link
Member

This does not seem to be a complete solution. How about adding https://github.com/cheshirekow/cmake_format to the CI and formatters?

@idzm
Copy link
Contributor Author

idzm commented Jul 16, 2024

After applying it directly (cmake_format):

cmake-format -i ../userver/CMakeLists.txt

we get following changes:

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -13,11 +13,7 @@ project(userver)
 
 set(USERVER_ROOT_DIR "${CMAKE_CURRENT_LIST_DIR}")
 
-option(
-  USERVER_INSTALL
-  "Prepare build of userver to install in system"
-  OFF
-)
+option(USERVER_INSTALL "Prepare build of userver to install in system" OFF)
 
 set(USERVER_AVAILABLE_COMPONENTS universal)
 set(USERVER_NOT_INCLUDED_AS_SUBDIR OFF)
@@ -25,7 +21,10 @@ if(CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR AND NOT USERVER_INSTALL)
   set(USERVER_NOT_INCLUDED_AS_SUBDIR ON)
 endif()
 
-option(USERVER_FEATURE_CORE "Provide a core library with coroutines, otherwise build only userver-universal" ON)
+option(
+  USERVER_FEATURE_CORE
+  "Provide a core library with coroutines, otherwise build only userver-universal"
+  ON)
 option(USERVER_FEATURE_CHAOTIC "Provide chaotic-codegen for jsonschema" ON)
 
 set(USERVER_BUILD_PLATFORM_X86 OFF)
@@ -34,24 +33,23 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "^x86")
 endif()
 
 function(_require_userver_core FEATURE)
-  if (NOT USERVER_FEATURE_CORE)
+  if(NOT USERVER_FEATURE_CORE)
     message(FATAL_ERROR "'${FEATURE}' requires 'USERVER_FEATURE_CORE=ON'")
   endif()
 endfunction()
 
-option(USERVER_FEATURE_UTEST "Provide 'utest' and 'ubench' for gtest and gbenchmark integration" ON)
-if (USERVER_FEATURE_UTEST)
+option(USERVER_FEATURE_UTEST
+       "Provide 'utest' and 'ubench' for gtest and gbenchmark integration" ON)
+if(USERVER_FEATURE_UTEST)
   message(STATUS "Building utest with gtest and ubench with gbench")
 endif()
 
-option(
-  USERVER_IS_THE_ROOT_PROJECT
-  "Contributor mode: build userver tests, samples and helper tools"
-  "${USERVER_NOT_INCLUDED_AS_SUBDIR}"
-)
-if (USERVER_IS_THE_ROOT_PROJECT)
+option(USERVER_IS_THE_ROOT_PROJECT
+       "Contributor mode: build userver tests, samples and helper tools"
+       "${USERVER_NOT_INCLUDED_AS_SUBDIR}")
+if(USERVER_IS_THE_ROOT_PROJECT)
   message(STATUS "Building userver as a primary project")
-  if (NOT USERVER_FEATURE_UTEST)
+  if(NOT USERVER_FEATURE_UTEST)
     message(FATAL_ERROR "Cannot build tests without utest")
   endif()
 else()
@@ -65,7 +63,9 @@ endif()
 
 set(USERVER_MONGODB_DEFAULT OFF)
 set(USERVER_CLICKHOUSE_DEFAULT OFF)
-if(USERVER_FEATURE_CORE AND USERVER_IS_THE_ROOT_PROJECT AND USERVER_BUILD_PLATFORM_X86)
+if(USERVER_FEATURE_CORE
+   AND USERVER_IS_THE_ROOT_PROJECT
+   AND USERVER_BUILD_PLATFORM_X86)
   if(NOT CMAKE_SYSTEM_NAME MATCHES "BSD")
     set(USERVER_MONGODB_DEFAULT ON)
   endif()
@@ -73,31 +73,34 @@ if(USERVER_FEATURE_CORE AND USERVER_IS_THE_ROOT_PROJECT AND USERVER_BUILD_PLATFO
 endif()
 
 set(USERVER_YDB_DEFAULT OFF)
-if(USERVER_FEATURE_CORE AND USERVER_IS_THE_ROOT_PROJECT AND
-  DEFINED CMAKE_CXX_STANDARD AND CMAKE_CXX_STANDARD GREATER_EQUAL 20)
+if(USERVER_FEATURE_CORE
+   AND USERVER_IS_THE_ROOT_PROJECT
+   AND DEFINED CMAKE_CXX_STANDARD
+   AND CMAKE_CXX_STANDARD GREATER_EQUAL 20)
   set(USERVER_YDB_DEFAULT ON)
 endif()
 
 option(USERVER_CONAN "Build with Conan packages" ${CONAN_EXPORTED})
 
-option(
-  USERVER_DOWNLOAD_PACKAGES
-  "Download missing third party packages and use the downloaded versions"
-  ON
-)
+option(USERVER_DOWNLOAD_PACKAGES
+       "Download missing third party packages and use the downloaded versions"
+       ON)
 option(
   USERVER_FORCE_DOWNLOAD_PACKAGES
   "Download all possible third party packages even if a system package is available"
-  OFF
-)
+  OFF)
 
-option(USERVER_FEATURE_CRYPTOPP_BLAKE2 "Provide wrappers for blake2 algorithms of crypto++" ON)
-if (NOT USERVER_FEATURE_CRYPTOPP_BLAKE2)
+option(USERVER_FEATURE_CRYPTOPP_BLAKE2
+       "Provide wrappers for blake2 algorithms of crypto++" ON)
+if(NOT USERVER_FEATURE_CRYPTOPP_BLAKE2)
   add_compile_definitions("USERVER_NO_CRYPTOPP_BLAKE2=1")
 endif()
 
-option(USERVER_FEATURE_CRYPTOPP_BASE64_URL "Provide wrappers for Base64 URL decoding and encoding algorithms of crypto++" ON)
-if (NOT USERVER_FEATURE_CRYPTOPP_BASE64_URL)
+option(
+  USERVER_FEATURE_CRYPTOPP_BASE64_URL
+  "Provide wrappers for Base64 URL decoding and encoding algorithms of crypto++"
+  ON)
+if(NOT USERVER_FEATURE_CRYPTOPP_BASE64_URL)
   add_compile_definitions("USERVER_NO_CRYPTOPP_BASE64_URL=1")
 endif()
 
@@ -106,29 +109,43 @@ if(CMAKE_SYSTEM_NAME MATCHES "BSD")
 else()
   set(JEMALLOC_DEFAULT ON)
 endif()
-option(USERVER_FEATURE_JEMALLOC "Enable linkage with jemalloc memory allocator" ${JEMALLOC_DEFAULT})
+option(USERVER_FEATURE_JEMALLOC "Enable linkage with jemalloc memory allocator"
+       ${JEMALLOC_DEFAULT})
 
-option(USERVER_DISABLE_PHDR_CACHE "Disable caching of dl_phdr_info items, which interferes with dlopen" OFF)
+option(USERVER_DISABLE_PHDR_CACHE
+       "Disable caching of dl_phdr_info items, which interferes with dlopen"
+       OFF)
 
 set(USERVER_DISABLE_RSEQ_DEFAULT ON)
-if (USERVER_BUILD_PLATFORM_X86 AND CMAKE_SYSTEM_NAME MATCHES "Linux")
+if(USERVER_BUILD_PLATFORM_X86 AND CMAKE_SYSTEM_NAME MATCHES "Linux")
   set(USERVER_DISABLE_RSEQ_DEFAULT OFF)
   message(STATUS "rseq-based acceleration is enabled by default")
 endif()
-option(USERVER_DISABLE_RSEQ_ACCELERATION "Disable rseq-based optimizations" ${USERVER_DISABLE_RSEQ_DEFAULT})
+option(USERVER_DISABLE_RSEQ_ACCELERATION "Disable rseq-based optimizations"
+       ${USERVER_DISABLE_RSEQ_DEFAULT})
 
 option(USERVER_CHECK_PACKAGE_VERSIONS "Check package versions" ON)
 
-option(USERVER_FEATURE_MONGODB "Provide asynchronous driver for MongoDB" "${USERVER_MONGODB_DEFAULT}")
-option(USERVER_FEATURE_POSTGRESQL "Provide asynchronous driver for PostgreSQL" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_REDIS "Provide asynchronous driver for Redis" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_GRPC "Provide asynchronous driver for gRPC" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_CLICKHOUSE "Provide asynchronous driver for ClickHouse" "${USERVER_CLICKHOUSE_DEFAULT}")
-option(USERVER_FEATURE_KAFKA "Provide asynchronous driver for Apache Kafka" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_RABBITMQ "Provide asynchronous driver for RabbitMQ" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_MYSQL "Provide asynchronous driver for MariaDB/MySQL" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_ROCKS "Provide asynchronous driver for Rocks" "${USERVER_LIB_ENABLED_DEFAULT}")
-option(USERVER_FEATURE_YDB "Provide asynchronous driver for YDB" "${USERVER_YDB_DEFAULT}")
+option(USERVER_FEATURE_MONGODB "Provide asynchronous driver for MongoDB"
+       "${USERVER_MONGODB_DEFAULT}")
+option(USERVER_FEATURE_POSTGRESQL "Provide asynchronous driver for PostgreSQL"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_REDIS "Provide asynchronous driver for Redis"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_GRPC "Provide asynchronous driver for gRPC"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_CLICKHOUSE "Provide asynchronous driver for ClickHouse"
+       "${USERVER_CLICKHOUSE_DEFAULT}")
+option(USERVER_FEATURE_KAFKA "Provide asynchronous driver for Apache Kafka"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_RABBITMQ "Provide asynchronous driver for RabbitMQ"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_MYSQL "Provide asynchronous driver for MariaDB/MySQL"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_ROCKS "Provide asynchronous driver for Rocks"
+       "${USERVER_LIB_ENABLED_DEFAULT}")
+option(USERVER_FEATURE_YDB "Provide asynchronous driver for YDB"
+       "${USERVER_YDB_DEFAULT}")
 
 set(CMAKE_DEBUG_POSTFIX d)
 
@@ -151,52 +168,48 @@ include(CMakePackageConfigHelpers)
 
 message(STATUS "Generating cmake files ...")
 userver_venv_setup(
-  NAME userver-cmake
-  PYTHON_OUTPUT_VAR USERVER_CMAKE_GEN_PYTHON_BINARY
-  REQUIREMENTS "${USERVER_ROOT_DIR}/scripts/external_deps/requirements.txt"
-)
+  NAME userver-cmake PYTHON_OUTPUT_VAR USERVER_CMAKE_GEN_PYTHON_BINARY
+  REQUIREMENTS "${USERVER_ROOT_DIR}/scripts/external_deps/requirements.txt")
 execute_process(
   COMMAND
-  "${USERVER_CMAKE_GEN_PYTHON_BINARY}"
-  -u "${USERVER_ROOT_DIR}/scripts/external_deps/cmake_generator.py"
-  --repo-dir "${USERVER_ROOT_DIR}"
-  --build-dir "${CMAKE_BINARY_DIR}"
+    "${USERVER_CMAKE_GEN_PYTHON_BINARY}" -u
+    "${USERVER_ROOT_DIR}/scripts/external_deps/cmake_generator.py" --repo-dir
+    "${USERVER_ROOT_DIR}" --build-dir "${CMAKE_BINARY_DIR}"
   RESULT_VARIABLE RESULT
-  WORKING_DIRECTORY "${USERVER_ROOT_DIR}"
-)
+  WORKING_DIRECTORY "${USERVER_ROOT_DIR}")
 if(RESULT)
-  message(FATAL_ERROR
-    "Generating cmake files failed with exit code: ${RESULT}"
-  )
+  message(FATAL_ERROR "Generating cmake files failed with exit code: ${RESULT}")
 endif(RESULT)
 
-set(USERVER_THIRD_PARTY_DIRS ${USERVER_ROOT_DIR}/third_party CACHE INTERNAL "")
+set(USERVER_THIRD_PARTY_DIRS
+    ${USERVER_ROOT_DIR}/third_party
+    CACHE INTERNAL "")
 
 init_debian_depends()
 
 include(SetupGTest)
-if (USERVER_FEATURE_GRPC)
+if(USERVER_FEATURE_GRPC)
   include(SetupProtobuf)
 endif()
 
-if (USERVER_IS_THE_ROOT_PROJECT)
+if(USERVER_IS_THE_ROOT_PROJECT)
   include(testsuite/SetupUserverTestsuiteEnv.cmake)
   add_subdirectory(testsuite)
 endif()
 
 add_subdirectory(universal)
 
-if (USERVER_FEATURE_CORE)
+if(USERVER_FEATURE_CORE)
   add_subdirectory(core)
   list(APPEND USERVER_AVAILABLE_COMPONENTS core)
 endif()
 
-if (USERVER_FEATURE_CHAOTIC)
+if(USERVER_FEATURE_CHAOTIC)
   add_subdirectory(chaotic)
   list(APPEND USERVER_AVAILABLE_COMPONENTS chaotic)
 endif()
 
-if (USERVER_IS_THE_ROOT_PROJECT AND USERVER_FEATURE_CORE)
+if(USERVER_IS_THE_ROOT_PROJECT AND USERVER_FEATURE_CORE)
   add_subdirectory(tools/engine)
   add_subdirectory(tools/httpclient)
   add_subdirectory(tools/netcat)
@@ -204,60 +217,60 @@ if (USERVER_IS_THE_ROOT_PROJECT AND USERVER_FEATURE_CORE)
   add_subdirectory(tools/congestion_control_emulator)
 endif()
 
-if (USERVER_FEATURE_MONGODB)
+if(USERVER_FEATURE_MONGODB)
   _require_userver_core("USERVER_FEATURE_MONGODB")
   add_subdirectory(mongo)
   list(APPEND USERVER_AVAILABLE_COMPONENTS mongo)
 endif()
 
-if (USERVER_FEATURE_POSTGRESQL)
+if(USERVER_FEATURE_POSTGRESQL)
   _require_userver_core("USERVER_FEATURE_POSTGRESQL")
   add_subdirectory(postgresql)
   list(APPEND USERVER_AVAILABLE_COMPONENTS postgres)
 endif()
 
-if (USERVER_FEATURE_REDIS)
+if(USERVER_FEATURE_REDIS)
   _require_userver_core("USERVER_FEATURE_REDIS")
   add_subdirectory(redis)
   list(APPEND USERVER_AVAILABLE_COMPONENTS redis)
 endif()
 
-if (USERVER_FEATURE_GRPC)
+if(USERVER_FEATURE_GRPC)
   _require_userver_core("USERVER_FEATURE_GRPC")
   add_subdirectory(grpc)
   list(APPEND USERVER_AVAILABLE_COMPONENTS grpc)
 endif()
 
-if (USERVER_FEATURE_CLICKHOUSE)
+if(USERVER_FEATURE_CLICKHOUSE)
   _require_userver_core("USERVER_FEATURE_CLICKHOUSE")
   add_subdirectory(clickhouse)
   list(APPEND USERVER_AVAILABLE_COMPONENTS clickhouse)
 endif()
 
-if (USERVER_FEATURE_KAFKA)
+if(USERVER_FEATURE_KAFKA)
   _require_userver_core("USERVER_FEATURE_KAFKA")
   add_subdirectory(kafka)
   list(APPEND USERVER_AVAILABLE_COMPONENTS kafka)
 endif()
 
-if (USERVER_FEATURE_RABBITMQ)
+if(USERVER_FEATURE_RABBITMQ)
   _require_userver_core("USERVER_FEATURE_RABBITMQ")
   add_subdirectory(rabbitmq)
   list(APPEND USERVER_AVAILABLE_COMPONENTS rabbitmq)
 endif()
 
-if (USERVER_FEATURE_MYSQL)
+if(USERVER_FEATURE_MYSQL)
   _require_userver_core("USERVER_FEATURE_MYSQL")
   add_subdirectory(mysql)
   list(APPEND USERVER_AVAILABLE_COMPONENTS mysql)
 endif()
 
-if (USERVER_FEATURE_ROCKS)
+if(USERVER_FEATURE_ROCKS)
   _require_userver_core("USERVER_FEATURE_ROCKS")
   add_subdirectory(rocks)
 endif()
 
-if (USERVER_FEATURE_YDB)
+if(USERVER_FEATURE_YDB)
   _require_userver_core("USERVER_FEATURE_YDB")
   add_subdirectory(ydb)
   list(APPEND USERVER_AVAILABLE_COMPONENTS ydb)
@@ -266,7 +279,7 @@ endif()
 _userver_export_targets()
 _userver_make_install_config()
 
-if (USERVER_IS_THE_ROOT_PROJECT AND USERVER_FEATURE_CORE)
+if(USERVER_IS_THE_ROOT_PROJECT AND USERVER_FEATURE_CORE)
   add_subdirectory(samples)
 endif()

So I'll try to write down some rules to keep the existing style.

@idzm
Copy link
Contributor Author

idzm commented Aug 20, 2024

Unfortunately, cmake_format cannot preserve the existing style (there are no such rules). So some style changes will be made in CMakeLists.txt.

@idzm
Copy link
Contributor Author

idzm commented Oct 21, 2024

After applying the formatter (cmake_format) to the whole project.

find . -type f -name 'CMakeLists.txt' | xargs cmake-format -i

@idzm idzm requested a review from segoon as a code owner October 21, 2024 13:18
@Anton3
Copy link
Member

Anton3 commented Jan 21, 2025

The PR is meaningless without adding .cmake-format.py configuration and a GitHub CI check.

The style needs tweaking, defaults are just bad and contrast with our C++ code style:

  1. We probably need to adopt uniform 4-spaces indent and 4-spaces continuation indent, just like we've recently did for our C++ code
  2. Use a width of 120 columns to match our new C++ code style
  3. In the cases of long function calls, closing paren should be placed on a separate line
  4. Can an additional indent be applied to option values in cases like this?
    function_with_lots_of_args(
        ARG_NAME
            value1
            value2
    )
    (Debatable)
  5. Wut
    target_link_libraries(${PROJECT_NAME} PUBLIC PostgreSQLInternal
                                                 userver-internal-compile-options)

For C++, we've adopted the style that formats single-line function calls as

frobnicate(bar, baz)

and multi-line function calls as

frobnicate(
    bar,
    baz
)

CMake code style should probably match that.

@idzm idzm marked this pull request as draft January 24, 2025 14:02
@idzm
Copy link
Contributor Author

idzm commented Feb 7, 2025

#638 (comment)

  • We probably need to adopt uniform 4-spaces indent and 4-spaces continuation indent, just like we've recently did for our C++ code:
if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.15")
-  cmake_policy(SET CMP0093 NEW)
+    cmake_policy(SET CMP0093 NEW)
endif()
  • Use a width of 120 columns to match our new C++ code style:
-option(
-    USERVER_DOWNLOAD_PACKAGES
-    "Download missing third party packages and use the downloaded versions"
-    ON
+option(USERVER_DOWNLOAD_PACKAGES "Download missing third party packages and use the downloaded versions" ON)
)
  • In case of long function calls closing parenthesis should be placed on a separate line:
file(GLOB_RECURSE SOURCES
    ${CMAKE_CURRENT_SOURCE_DIR}/include/*.hpp
    ${CMAKE_CURRENT_SOURCE_DIR}/src/*.hpp
-   ${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp)
+   ${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp
+)
  • Additional indent to be applied to option values in cases like this (debatable):
function_with_lots_of_args(
-    ARG_NAME value1 value2
+    ARG_NAME
+        value1
+        value2
)

Note

After format we get such style:

  userver_venv_setup(
      NAME userver-chaotic-tests
      PYTHON_OUTPUT_VAR USERVER_CHAOTIC_PYTEST_PYTHON_BINARY
-     REQUIREMENTS
-         "${USERVER_ROOT_DIR}/scripts/chaotic/requirements.txt"
-         "${USERVER_ROOT_DIR}/testsuite/requirements.txt"
+     REQUIREMENTS "${USERVER_ROOT_DIR}/scripts/chaotic/requirements.txt"
 +                 "${USERVER_ROOT_DIR}/testsuite/requirements.txt"
      UNIQUE
  )
  • For C++, we've adopted the style that formats single-line function calls as single and multi-line function calls - CMake code style should probably match that:
+frobnicate(bar, baz)

+frobnicate(
+    bar,
+    baz
+)

Note

After format we get such style (if a positional argument group contains more than 6 arguments, then force it to a vertical layout):

- userver_add_grpc_library(${PROJECT_NAME}-unittest-proto
+ userver_add_grpc_library(
+   ${PROJECT_NAME}-unittest-proto
    PROTOS
    # Absolute paths are allowed
    ${CMAKE_CURRENT_SOURCE_DIR}/proto/tests/unit_test.proto
    # As well as paths relative to CMAKE_CURRENT_SOURCE_DIR
    tests/messages.proto
    tests/protobuf.proto
    tests/same_service_and_method_name.proto
    tests/global_package.proto
    tests/repeating_word_in_package_name.proto
    tests/secret_fields.proto
    INCLUDE_DIRECTORIES ${CMAKE_CURRENT_SOURCE_DIR}/proto
)

@idzm idzm force-pushed the fix_cmake_style branch 2 times, most recently from ad4c606 to 991f53e Compare February 8, 2025 08:24
@idzm
Copy link
Contributor Author

idzm commented Feb 8, 2025

@Anton3 - need you feedback about #638 (comment).

REQUIRED
)
# Note: If userver was added via add_subdirectory(path/to/userver), then the userver_setup_environment() should be
# called here. /// [find_userver]
Copy link
Member

Choose a reason for hiding this comment

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

Comments of the form

# /// [...]

should be kept on a separate line

Perhaps add an empty line between the comment above and this technical comment

@Anton3
Copy link
Member

Anton3 commented Feb 11, 2025

@idzm That's such a great work, thank you! Any chance you'll also add a format check to .github/workflows?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants