You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/02/21 19:45:48 UTC
[GitHub] [arrow] kou commented on a diff in pull request #34259: GH-34258: [C++] System flatbuffers
kou commented on code in PR #34259:
URL: https://github.com/apache/arrow/pull/34259#discussion_r1113489602
##########
cpp/build-support/update-flatbuffers.sh:
##########
@@ -18,30 +18,22 @@
# under the License.
#
-# Run this from cpp/ directory. flatc is expected to be in your path
+# Run this from cpp/ directory. flatc is expected to be in your path.
+# The output directory can be passed as a positional argumenet,
+# it defaults to /cpp/src/generated.
set -euo pipefail
CWD="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)"
SOURCE_DIR="$CWD/../src"
-PYTHON_SOURCE_DIR="$CWD/../../python"
FORMAT_DIR="$CWD/../../format"
-TOP="$FORMAT_DIR/.."
FLATC="flatc"
Review Comment:
How about running `flatc` from CMake directly instead of using this shell script?
Because:
* We need to use `flatbuffers::flatc` instead of `flatc`.
* If we use shell script, we can't use system FlatBuffers on Windows.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -313,15 +320,23 @@ endmacro()
set(THIRDPARTY_DIR "${arrow_SOURCE_DIR}/thirdparty")
-add_library(arrow::flatbuffers INTERFACE IMPORTED)
-if(CMAKE_VERSION VERSION_LESS 3.11)
- set_target_properties(arrow::flatbuffers
- PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
- "${THIRDPARTY_DIR}/flatbuffers/include")
-else()
- target_include_directories(arrow::flatbuffers
- INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include")
-endif()
+macro(build_flatbuffers)
+ add_library(flatbuffers::flatbuffers INTERFACE IMPORTED)
+ if(CMAKE_VERSION VERSION_LESS 3.11)
+ set_target_properties(flatbuffers::flatbuffers
+ PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
+ "${THIRDPARTY_DIR}/flatbuffers/include")
+ else()
+ target_include_directories(flatbuffers::flatbuffers
+ INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include")
+ endif()
+endmacro()
+
+resolve_dependency(flatbuffers
+ IS_RUNTIME_DEPENDENCY
+ FALSE
Review Comment:
Is this correct?
It seems that `flatbuffers::flatbuffers` depends on `libflatbuffers.a` and `flatbuffers::flatbuffers_shared` depends on `libflatbuffers.so`.
##########
cpp/CMakeLists.txt:
##########
@@ -541,7 +541,20 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
include_directories(src)
# Compiled flatbuffers files
-include_directories(src/generated)
+if("${flatbuffers_SOURCE}" STREQUAL "SYSTEM")
+ file(GLOB _flatbuffers_files "${CMAKE_CURRENT_SOURCE_DIR}/../format/*.fbs")
+ add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/src/generated
+ COMMAND "${BUILD_SUPPORT_DIR}/update-flatbuffers.sh"
+ "${CMAKE_CURRENT_BINARY_DIR}/src/generated"
+ DEPENDS ${_flatbuffers_files}
+ VERBATIM)
+ unset(_flatbuffers_files)
+ add_custom_target(generate-flatbuffers
+ DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/src/generated)
Review Comment:
How about one more custom target that updates bundled generated files in `src/generated/` like `update-generated-flatbuffers`?
If we provide it, we can remove `cpp/build-support/update-flatbuffers.sh`.
##########
cpp/CMakeLists.txt:
##########
@@ -541,7 +541,20 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
include_directories(src)
# Compiled flatbuffers files
-include_directories(src/generated)
+if("${flatbuffers_SOURCE}" STREQUAL "SYSTEM")
+ file(GLOB _flatbuffers_files "${CMAKE_CURRENT_SOURCE_DIR}/../format/*.fbs")
+ add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/src/generated
+ COMMAND "${BUILD_SUPPORT_DIR}/update-flatbuffers.sh"
+ "${CMAKE_CURRENT_BINARY_DIR}/src/generated"
+ DEPENDS ${_flatbuffers_files}
+ VERBATIM)
+ unset(_flatbuffers_files)
+ add_custom_target(generate-flatbuffers
+ DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/src/generated)
+ include_directories(${CMAKE_CURRENT_BINARY_DIR}/src/generated)
+else()
+ include_directories(src/generated)
Review Comment:
Could you attach them to `arrow::flatbuffers` instead of using `include_directories()`?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -313,15 +320,23 @@ endmacro()
set(THIRDPARTY_DIR "${arrow_SOURCE_DIR}/thirdparty")
-add_library(arrow::flatbuffers INTERFACE IMPORTED)
-if(CMAKE_VERSION VERSION_LESS 3.11)
- set_target_properties(arrow::flatbuffers
- PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
- "${THIRDPARTY_DIR}/flatbuffers/include")
-else()
- target_include_directories(arrow::flatbuffers
- INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include")
-endif()
+macro(build_flatbuffers)
+ add_library(flatbuffers::flatbuffers INTERFACE IMPORTED)
+ if(CMAKE_VERSION VERSION_LESS 3.11)
+ set_target_properties(flatbuffers::flatbuffers
+ PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
+ "${THIRDPARTY_DIR}/flatbuffers/include")
+ else()
+ target_include_directories(flatbuffers::flatbuffers
+ INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include")
+ endif()
+endmacro()
+
+resolve_dependency(flatbuffers
+ IS_RUNTIME_DEPENDENCY
+ FALSE
+ USE_CONFIG
+ TRUE)
Review Comment:
Can we remove this?
In general, `Findflatbuffers.cmake` doesn't exist. So we don't need this.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -313,15 +320,23 @@ endmacro()
set(THIRDPARTY_DIR "${arrow_SOURCE_DIR}/thirdparty")
-add_library(arrow::flatbuffers INTERFACE IMPORTED)
-if(CMAKE_VERSION VERSION_LESS 3.11)
- set_target_properties(arrow::flatbuffers
- PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
- "${THIRDPARTY_DIR}/flatbuffers/include")
-else()
- target_include_directories(arrow::flatbuffers
- INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include")
-endif()
+macro(build_flatbuffers)
+ add_library(flatbuffers::flatbuffers INTERFACE IMPORTED)
+ if(CMAKE_VERSION VERSION_LESS 3.11)
+ set_target_properties(flatbuffers::flatbuffers
+ PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
+ "${THIRDPARTY_DIR}/flatbuffers/include")
+ else()
+ target_include_directories(flatbuffers::flatbuffers
+ INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include")
+ endif()
+endmacro()
+
+resolve_dependency(flatbuffers
+ IS_RUNTIME_DEPENDENCY
+ FALSE
+ USE_CONFIG
+ TRUE)
Review Comment:
Could you add a new `ARROW_FLATBUFFERS_USE_SHARED` CMake option?
If it's true, we use `flatbuffers::flatbuffers_shared` instead of `flatbuffers::flatbuffers`.
How about keeping the current `arrow::flatbuffers` target? It refers the bundled FlatBuffers with `flatbuffers_SOURCE == TRUE`. It refers `flatbuffers::flatbuffers_shared` with `ARROW_FLATBUFFERS_USE_SHARED == TRUE`. It refers `flatbuffers::flatbuffers` otherwise.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org