refactor(cmake): CMake configuration files for improved readability#904
refactor(cmake): CMake configuration files for improved readability#904SYaoJun wants to merge 2 commits intoapache:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
=========================================
Coverage 80.60% 80.60%
Complexity 615 615
=========================================
Files 94 94
Lines 10709 10709
Branches 1055 1055
=========================================
Hits 8632 8632
Misses 1837 1837
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b9e155d to
7afcd33
Compare
7afcd33 to
a2f06df
Compare
There was a problem hiding this comment.
Pull request overview
Introduces cmake-format to standardize formatting of CMake configuration across the repository (per #903), and applies the formatter to existing CMakeLists/cmake modules to improve readability and enforce consistency via automation.
Changes:
- Add
cmake-formatas apre-commithook and run it in CI. - Reformat multiple CMakeLists.txt and
.cmakemodules across C++, Python, and Java build integrations. - Add a repository-level
cmake-formatconfiguration file.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| python/CMakeLists.txt | Reformats Python build CMakeLists for consistent indentation/wrapping. |
| maven-projects/java/cmake/graphar-cpp.cmake | Reformats Java-side ExternalProject build helper for GraphAr C++. |
| maven-projects/java/CMakeLists.txt | Reformats Java JNI CMakeLists (options/globs/custom commands). |
| cpp/test/CMakeLists.txt | Reformats test macro and link/include blocks for readability. |
| cpp/src/CMakeLists.txt | Reformats core library build rules and Arrow linking blocks. |
| cpp/graphar-config-version.in.cmake | Minor formatting adjustment in version check logic. |
| cpp/examples/CMakeLists.txt | Reformats example discovery/build loop and link logic. |
| cpp/cmake/apache-arrow.cmake | Reformats Arrow ExternalProject build configuration and target setup. |
| cpp/benchmarks/CMakeLists.txt | Reformats benchmark macro and linking logic. |
| cpp/CMakeLists.txt | Reformats top-level C++ project configuration and build options. |
| cmake-format.py | Adds a cmake-format configuration file for consistent formatting rules. |
| .pre-commit-config.yaml | Adds cmake-format hook to enforce formatting locally. |
| .github/workflows/ci.yml | Adds CI step to run cmake-format via pre-commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "-DARROW_JSON=ON" | ||
| "-DARROW_PYTHON=OFF" | ||
| "-DARROW_BUILD_BENCHMAKRS=OFF" | ||
| "-DARROW_BUILD_TESTS=OFF" |
There was a problem hiding this comment.
The Arrow CMake option name is misspelled as ARROW_BUILD_BENCHMAKRS (missing "R"). This will be ignored by Arrow and may accidentally build benchmarks. Use the correct ARROW_BUILD_BENCHMARKS option name in the CMake args list.
| "-DARROW_PARQUET=ON" | ||
| "-DARROW_WITH_RE2=OFF" | ||
| "-DARROW_WITH_UTF8PROC=OFF" | ||
| "-DARROW_WITH_RE2=OFF" | ||
| "-DARROW_FILESYSTEM=ON" |
There was a problem hiding this comment.
GAR_ARROW_CMAKE_ARGS includes -DARROW_WITH_RE2=OFF twice. This duplication is redundant and can make it harder to audit the Arrow build configuration; remove the duplicate entry to keep the option list unambiguous.
| - repo: https://github.com/cheshirekow/cmake-format-precommit | ||
| rev: v0.6.10 | ||
| hooks: | ||
| - id: cmake-format | ||
|
|
There was a problem hiding this comment.
The cmake-format hook is added, but the repository config file is named cmake-format.py and isn't referenced here. cmake-format typically auto-discovers .cmake-format.py/.cmake-format.yaml, so this setup may run with default formatting settings in CI and on contributors' machines. Rename the config to .cmake-format.py or pass the config explicitly via hook args (e.g., --config-files cmake-format.py).
There was a problem hiding this comment.
cmake-format.py also can find it.
cpp/CMakeLists.txt
Outdated
| message(ERROR | ||
| "The OpenSSL must be greater than or equal to 1.1.0, current version is ${OPENSSL_VERSION}" | ||
| ) |
There was a problem hiding this comment.
message(ERROR ...) isn't a valid CMake message mode and will itself error when this branch is hit. If the intent is to stop configuration when OpenSSL < 1.1.0 is detected, use message(FATAL_ERROR ...) (or SEND_ERROR if you want to continue processing).
dbac2b9 to
3b66a94
Compare
…ypo in build options
3b66a94 to
b651f58
Compare
Reason for this PR
fix: #903
cmake-formatin pre-commitCIcheck cmake-formatcmake-format.pyreferred to arrow cmake-format.pyWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?