You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tobim (via GitHub)" <gi...@apache.org> on 2023/02/19 08:05:18 UTC
[GitHub] [arrow] tobim opened a new pull request, #34259: GH-34258: [C++] System flatbuffers
tobim opened a new pull request, #34259:
URL: https://github.com/apache/arrow/pull/34259
### What changes are included in this PR?
* Make it possible to use `-Dflatbuffers_SOURCE=SYSTEM`.
* Change `update-flatbuffers.sh` so that the output directory can optionally be passed in.
* Generate flatbuffers headers with `update-flatbuffers.sh` when flatbuffers is provided by the system.
* Remove plasma file generation from `update-flatbuffers.sh`.
### Are these changes tested?
This change is opt-in, so I'm not sure if it makes sense to extend the build matrix for it.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #34259: GH-34258: [C++] System flatbuffers
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34259:
URL: https://github.com/apache/arrow/pull/34259#issuecomment-1435920191
* Closes: #34258
--
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
[GitHub] [arrow] amol- commented on pull request #34259: GH-34258: [C++] Add suppport for building with system flatbuffers
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #34259:
URL: https://github.com/apache/arrow/pull/34259#issuecomment-1490644003
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
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
[GitHub] [arrow] amol- closed pull request #34259: GH-34258: [C++] Add suppport for building with system flatbuffers
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #34259: GH-34258: [C++] Add suppport for building with system flatbuffers
URL: https://github.com/apache/arrow/pull/34259
--
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
[GitHub] [arrow] kou commented on a diff in pull request #34259: GH-34258: [C++] System flatbuffers
Posted by "kou (via GitHub)" <gi...@apache.org>.
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
[GitHub] [arrow] github-actions[bot] commented on pull request #34259: GH-34258: [C++] System flatbuffers
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34259:
URL: https://github.com/apache/arrow/pull/34259#issuecomment-1435920197
:warning: GitHub issue #34258 **has been automatically assigned in GitHub** to PR creator.
--
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
[GitHub] [arrow] tobim commented on pull request #34259: GH-34258: [C++] Add suppport for building with system flatbuffers
Posted by "tobim (via GitHub)" <gi...@apache.org>.
tobim commented on PR #34259:
URL: https://github.com/apache/arrow/pull/34259#issuecomment-1491397120
Sorry for not following up. I don't have capacity to work on this at the moment. I still plan to integrate the change request eventually.
--
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