Copilot commented on code in PR #904:
URL: https://github.com/apache/incubator-graphar/pull/904#discussion_r2911484503
##########
cpp/CMakeLists.txt:
##########
@@ -284,57 +298,62 @@ endfunction()
include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
include_directories(src)
-if (BUILD_ARROW_FROM_SOURCE)
- # the necessary dependencies for building arrow from source
- find_package(OpenSSL REQUIRED)
- if(OPENSSL_FOUND)
- if(OPENSSL_VERSION LESS "1.1.0")
- message(ERROR "The OpenSSL must be greater than or equal to 1.1.0,
current version is ${OPENSSL_VERSION}")
- endif()
+if(BUILD_ARROW_FROM_SOURCE)
+ # the necessary dependencies for building arrow from source
+ find_package(OpenSSL REQUIRED)
+ if(OPENSSL_FOUND)
+ if(OPENSSL_VERSION LESS "1.1.0")
+ message(ERROR
+ "The OpenSSL must be greater than or equal to 1.1.0, current
version is ${OPENSSL_VERSION}"
+ )
Review 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).
##########
.pre-commit-config.yaml:
##########
@@ -47,6 +47,11 @@ repos:
types_or: [c++]
args: [--style=file, --verbose]
exclude: ^cpp/thirdparty/
+
+ - repo: https://github.com/cheshirekow/cmake-format-precommit
+ rev: v0.6.10
+ hooks:
+ - id: cmake-format
Review 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`).
##########
cpp/cmake/apache-arrow.cmake:
##########
@@ -35,139 +35,162 @@
# This cmake file is referred and derived from
# https://github.com/apache/arrow/blob/master/matlab/CMakeLists.txt
-
# Build the Arrow C++ libraries.
function(build_arrow)
- set(one_value_args)
- set(multi_value_args)
+ set(one_value_args)
+ set(multi_value_args)
+
+ cmake_parse_arguments(ARG
+ "${options}"
+ "${one_value_args}"
+ "${multi_value_args}"
+ ${ARGN})
+ if(ARG_UNPARSED_ARGUMENTS)
+ message(SEND_ERROR "Error: unrecognized arguments:
${ARG_UNPARSED_ARGUMENTS}")
+ endif()
- cmake_parse_arguments(ARG
- "${options}"
- "${one_value_args}"
- "${multi_value_args}"
- ${ARGN})
- if (ARG_UNPARSED_ARGUMENTS)
- message(SEND_ERROR "Error: unrecognized arguments:
${ARG_UNPARSED_ARGUMENTS}")
- endif ()
+ # If Arrow needs to be built, the default location will be within the build
tree.
+ set(GAR_ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
- # If Arrow needs to be built, the default location will be within the
build tree.
- set(GAR_ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(GAR_ARROW_STATIC_LIBRARY_DIR "${GAR_ARROW_PREFIX}/lib")
- set(GAR_ARROW_STATIC_LIBRARY_DIR "${GAR_ARROW_PREFIX}/lib")
+ set(GAR_ARROW_STATIC_LIB_FILENAME
+ "${CMAKE_STATIC_LIBRARY_PREFIX}arrow${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_ARROW_STATIC_LIB
+ "${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "arrow lib")
+ set(GAR_PARQUET_STATIC_LIB_FILENAME
+ "${CMAKE_STATIC_LIBRARY_PREFIX}parquet${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_PARQUET_STATIC_LIB
+ "${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_PARQUET_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "parquet lib")
+ set(GAR_DATASET_STATIC_LIB_FILENAME
+
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_dataset${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_DATASET_STATIC_LIB
+ "${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_DATASET_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "arrow dataset lib")
+ set(GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME
+
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_compute${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_ARROW_COMPUTE_STATIC_LIB
+
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "arrow compute lib")
+ set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME
+
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}"
+ )
+ set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB
+
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "bundled deps lib")
- set(GAR_ARROW_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_ARROW_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_STATIC_LIB_FILENAME}" CACHE
INTERNAL "arrow lib")
- set(GAR_PARQUET_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}parquet${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_PARQUET_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_PARQUET_STATIC_LIB_FILENAME}" CACHE
INTERNAL "parquet lib")
- set(GAR_DATASET_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_dataset${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_DATASET_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_DATASET_STATIC_LIB_FILENAME}" CACHE
INTERNAL "arrow dataset lib")
- set(GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_compute${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_ARROW_COMPUTE_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME}"
CACHE INTERNAL "arrow compute lib")
- set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB
-
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME}"
CACHE INTERNAL "bundled deps lib")
+ set(GAR_ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-build")
+ set(GAR_ARROW_CMAKE_ARGS
+ "-DCMAKE_INSTALL_PREFIX=${GAR_ARROW_PREFIX}"
+ "-DARROW_BUILD_STATIC=ON"
+ "-DARROW_BUILD_SHARED=OFF"
+ "-DARROW_DEPENDENCY_SOURCE=BUNDLED"
+ "-DARROW_DEPENDENCY_USE_SHARED=OFF"
+ "-DCMAKE_INSTALL_LIBDIR=lib"
+ "-Dxsimd_SOURCE=BUNDLED"
+ "-DARROW_PARQUET=ON"
+ "-DARROW_WITH_RE2=OFF"
+ "-DARROW_WITH_UTF8PROC=OFF"
+ "-DARROW_WITH_RE2=OFF"
+ "-DARROW_FILESYSTEM=ON"
+ "-DARROW_CSV=ON"
+ "-DARROW_JSON=ON"
+ "-DARROW_PYTHON=OFF"
+ "-DARROW_BUILD_BENCHMAKRS=OFF"
+ "-DARROW_BUILD_TESTS=OFF"
Review 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.
##########
cpp/cmake/apache-arrow.cmake:
##########
@@ -35,139 +35,162 @@
# This cmake file is referred and derived from
# https://github.com/apache/arrow/blob/master/matlab/CMakeLists.txt
-
# Build the Arrow C++ libraries.
function(build_arrow)
- set(one_value_args)
- set(multi_value_args)
+ set(one_value_args)
+ set(multi_value_args)
+
+ cmake_parse_arguments(ARG
+ "${options}"
+ "${one_value_args}"
+ "${multi_value_args}"
+ ${ARGN})
+ if(ARG_UNPARSED_ARGUMENTS)
+ message(SEND_ERROR "Error: unrecognized arguments:
${ARG_UNPARSED_ARGUMENTS}")
+ endif()
- cmake_parse_arguments(ARG
- "${options}"
- "${one_value_args}"
- "${multi_value_args}"
- ${ARGN})
- if (ARG_UNPARSED_ARGUMENTS)
- message(SEND_ERROR "Error: unrecognized arguments:
${ARG_UNPARSED_ARGUMENTS}")
- endif ()
+ # If Arrow needs to be built, the default location will be within the build
tree.
+ set(GAR_ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
- # If Arrow needs to be built, the default location will be within the
build tree.
- set(GAR_ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(GAR_ARROW_STATIC_LIBRARY_DIR "${GAR_ARROW_PREFIX}/lib")
- set(GAR_ARROW_STATIC_LIBRARY_DIR "${GAR_ARROW_PREFIX}/lib")
+ set(GAR_ARROW_STATIC_LIB_FILENAME
+ "${CMAKE_STATIC_LIBRARY_PREFIX}arrow${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_ARROW_STATIC_LIB
+ "${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "arrow lib")
+ set(GAR_PARQUET_STATIC_LIB_FILENAME
+ "${CMAKE_STATIC_LIBRARY_PREFIX}parquet${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_PARQUET_STATIC_LIB
+ "${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_PARQUET_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "parquet lib")
+ set(GAR_DATASET_STATIC_LIB_FILENAME
+
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_dataset${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_DATASET_STATIC_LIB
+ "${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_DATASET_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "arrow dataset lib")
+ set(GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME
+
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_compute${CMAKE_STATIC_LIBRARY_SUFFIX}")
+ set(GAR_ARROW_COMPUTE_STATIC_LIB
+
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "arrow compute lib")
+ set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME
+
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}"
+ )
+ set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB
+
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME}"
+ CACHE INTERNAL "bundled deps lib")
- set(GAR_ARROW_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_ARROW_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_STATIC_LIB_FILENAME}" CACHE
INTERNAL "arrow lib")
- set(GAR_PARQUET_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}parquet${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_PARQUET_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_PARQUET_STATIC_LIB_FILENAME}" CACHE
INTERNAL "parquet lib")
- set(GAR_DATASET_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_dataset${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_DATASET_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_DATASET_STATIC_LIB_FILENAME}" CACHE
INTERNAL "arrow dataset lib")
- set(GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_compute${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_ARROW_COMPUTE_STATIC_LIB
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_COMPUTE_STATIC_LIB_FILENAME}"
CACHE INTERNAL "arrow compute lib")
- set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME
-
"${CMAKE_STATIC_LIBRARY_PREFIX}arrow_bundled_dependencies${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(GAR_ARROW_BUNDLED_DEPS_STATIC_LIB
-
"${GAR_ARROW_STATIC_LIBRARY_DIR}/${GAR_ARROW_BUNDLED_DEPS_STATIC_LIB_FILENAME}"
CACHE INTERNAL "bundled deps lib")
+ set(GAR_ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-build")
+ set(GAR_ARROW_CMAKE_ARGS
+ "-DCMAKE_INSTALL_PREFIX=${GAR_ARROW_PREFIX}"
+ "-DARROW_BUILD_STATIC=ON"
+ "-DARROW_BUILD_SHARED=OFF"
+ "-DARROW_DEPENDENCY_SOURCE=BUNDLED"
+ "-DARROW_DEPENDENCY_USE_SHARED=OFF"
+ "-DCMAKE_INSTALL_LIBDIR=lib"
+ "-Dxsimd_SOURCE=BUNDLED"
+ "-DARROW_PARQUET=ON"
+ "-DARROW_WITH_RE2=OFF"
+ "-DARROW_WITH_UTF8PROC=OFF"
+ "-DARROW_WITH_RE2=OFF"
+ "-DARROW_FILESYSTEM=ON"
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
