From b683d0bc80f0d8a8ba0f7779031899768905aa9c Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Thu, 17 Feb 2022 21:13:18 +0100 Subject: [PATCH 01/11] First attempt at building Substrait via ThirdpartyToolchain.cmake --- cpp/cmake_modules/FindSubstrait.cmake | 19 ++++ cpp/cmake_modules/ThirdpartyToolchain.cmake | 110 +++++++++++++++++++- cpp/src/arrow/engine/CMakeLists.txt | 79 +------------- cpp/thirdparty/versions.txt | 2 + 4 files changed, 133 insertions(+), 77 deletions(-) create mode 100644 cpp/cmake_modules/FindSubstrait.cmake diff --git a/cpp/cmake_modules/FindSubstrait.cmake b/cpp/cmake_modules/FindSubstrait.cmake new file mode 100644 index 00000000000..dce8e086d1f --- /dev/null +++ b/cpp/cmake_modules/FindSubstrait.cmake @@ -0,0 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Currently, we can only build Substrait from source. +set(SUBSTRAIT_FOUND NO) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 5b560591235..60cae3b903d 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -69,6 +69,7 @@ set(ARROW_THIRDPARTY_DEPENDENCIES Protobuf RapidJSON Snappy + Substrait Thrift utf8proc xsimd @@ -173,6 +174,8 @@ macro(build_dependency DEPENDENCY_NAME) build_re2() elseif("${DEPENDENCY_NAME}" STREQUAL "Snappy") build_snappy() + elseif("${DEPENDENCY_NAME}" STREQUAL "Substrait") + build_substrait() elseif("${DEPENDENCY_NAME}" STREQUAL "Thrift") build_thrift() elseif("${DEPENDENCY_NAME}" STREQUAL "utf8proc") @@ -309,8 +312,15 @@ endif() if(ARROW_ORC OR ARROW_FLIGHT - OR ARROW_GANDIVA - OR ARROW_ENGINE) + OR ARROW_GANDIVA) + set(ARROW_WITH_PROTOBUF ON) +endif() + +if(ARROW_ENGINE) + set(ARROW_WITH_SUBSTRAIT ON) +endif() + +if(ARROW_WITH_SUBSTRAIT) set(ARROW_WITH_PROTOBUF ON) endif() @@ -610,6 +620,14 @@ else() endif() endif() +if(DEFINED ENV{ARROW_SUBSTRAIT_URL}) + set(SUBSTRAIT_SOURCE_URL "$ENV{ARROW_SUBSTRAIT_URL}") +else() + set_urls(SUBSTRAIT_SOURCE_URL + "https://github.com/substrait-io/substrait/archive/${ARROW_SUBSTRAIT_BUILD_VERSION}.tar.gz" + ) +endif() + if(DEFINED ENV{ARROW_THRIFT_URL}) set(THRIFT_SOURCE_URL "$ENV{ARROW_THRIFT_URL}") else() @@ -1421,7 +1439,7 @@ if(ARROW_WITH_THRIFT) endif() # ---------------------------------------------------------------------- -# Protocol Buffers (required for ORC and Flight and Gandiva libraries) +# Protocol Buffers (required for ORC, Flight, Gandiva and Substrait libraries) macro(build_protobuf) message("Building Protocol Buffers from source") @@ -1605,6 +1623,92 @@ if(ARROW_WITH_PROTOBUF) message(STATUS "Found protobuf headers: ${PROTOBUF_INCLUDE_DIR}") endif() +# ---------------------------------------------------------------------- +# Substrait (required by compute engine) + +macro(build_substrait) + message("Building Substrait from source") + + set(SUBSTRAIT_PROTOS + capabilities + expression + extensions/extensions + function + parameterized_types + plan + relations + type + type_expressions) + + externalproject_add(substrait_ep + CONFIGURE_COMMAND "" + BUILD_COMMAND "" + INSTALL_COMMAND "" + URL ${SUBSTRAIT_SOURCE_URL} + URL_HASH "SHA256=${ARROW_SUBSTRAIT_BUILD_SHA256_CHECKSUM}") + + externalproject_get_property(substrait_ep SOURCE_DIR) + set(SUBSTRAIT_LOCAL_DIR ${SOURCE_DIR}) + + set(SUBSTRAIT_CPP_DIR "${CMAKE_CURRENT_BINARY_DIR}/substrait_ep-generated") + + set(SUBSTRAIT_SUPPRESSED_WARNINGS) + if(MSVC) + # Protobuf generated files trigger some spurious warnings on MSVC. + + # Implicit conversion from uint64_t to uint32_t: + list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4244") + + # Missing dll-interface: + list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4251") + endif() + + set(SUBSTRAIT_SOURCES) + set(SUBSTRAIT_PROTO_GEN_ALL) + foreach(SUBSTRAIT_PROTO ${SUBSTRAIT_PROTOS}) + set(SUBSTRAIT_PROTO_GEN "${SUBSTRAIT_CPP_DIR}/substrait/${SUBSTRAIT_PROTO}.pb") + + foreach(EXT h cc) + set_source_files_properties("${SUBSTRAIT_PROTO_GEN}.${EXT}" + PROPERTIES COMPILE_OPTIONS + "${SUBSTRAIT_SUPPRESSED_WARNINGS}" + GENERATED TRUE + SKIP_UNITY_BUILD_INCLUSION TRUE) + add_custom_command(OUTPUT "${SUBSTRAIT_PROTO_GEN}.${EXT}" + COMMAND ${ARROW_PROTOBUF_PROTOC} "-I${SUBSTRAIT_LOCAL_DIR}/proto" + "--cpp_out=${SUBSTRAIT_CPP_DIR}" + "${SUBSTRAIT_LOCAL_DIR}/proto/substrait/${SUBSTRAIT_PROTO}.proto" + DEPENDS ${PROTO_DEPENDS} substrait_ep) + list(APPEND SUBSTRAIT_PROTO_GEN_ALL "${SUBSTRAIT_PROTO_GEN}.${EXT}") + endforeach() + + list(APPEND SUBSTRAIT_SOURCES "${SUBSTRAIT_PROTO_GEN}.cc") + endforeach() + + add_custom_target(substrait_gen ALL DEPENDS ${SUBSTRAIT_PROTO_GEN_ALL}) + + add_arrow_lib(arrow_substrait + SOURCES + ${SUBSTRAIT_SOURCES} + DEPENDENCIES + substrait_gen + SHARED_LINK_LIBS + ${ARROW_PROTOBUF_LIBPROTOBUF} + STATIC_LINK_LIBS + ${ARROW_PROTOBUF_LIBPROTOBUF} + EXTRA_INCLUDES + ${SUBSTRAIT_CPP_DIR}) + + set(SUBSTRAIT_DEPENDENCIES substrait_gen) + set(SUBSTRAIT_SHARED arrow_substrait_shared) + set(SUBSTRAIT_STATIC arrow_substrait_static) + set(SUBSTRAIT_INCLUDES ${SUBSTRAIT_CPP_DIR}) +endmacro() + +if(ARROW_WITH_SUBSTRAIT) + resolve_dependency(Substrait) +endif() + # ---------------------------------------------------------------------- # jemalloc - Unix-only high-performance allocator diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt index 1e66e7fb5f7..50a84464c0b 100644 --- a/cpp/src/arrow/engine/CMakeLists.txt +++ b/cpp/src/arrow/engine/CMakeLists.txt @@ -19,12 +19,6 @@ add_custom_target(arrow_engine) arrow_install_all_headers("arrow/engine") -set(ARROW_ENGINE_LINK_LIBS ${ARROW_PROTOBUF_LIBPROTOBUF}) - -#if(WIN32) -# list(APPEND ARROW_ENGINE_LINK_LIBS ws2_32.lib) -#endif() - set(ARROW_ENGINE_SRCS substrait/expression_internal.cc substrait/extension_set.cc @@ -34,71 +28,6 @@ set(ARROW_ENGINE_SRCS substrait/relation_internal.cc substrait/type_internal.cc) -set(SUBSTRAIT_LOCAL_DIR "${CMAKE_CURRENT_BINARY_DIR}/substrait") -set(SUBSTRAIT_GEN_DIR "${CMAKE_CURRENT_BINARY_DIR}/generated") -set(SUBSTRAIT_PROTOS - capabilities - expression - extensions/extensions - function - parameterized_types - plan - relations - type - type_expressions) - -externalproject_add(substrait_ep - GIT_REPOSITORY "${ARROW_SUBSTRAIT_REPO}" - GIT_TAG "${ARROW_SUBSTRAIT_TAG}" - SOURCE_DIR "${SUBSTRAIT_LOCAL_DIR}" - CONFIGURE_COMMAND "" - BUILD_COMMAND "" - INSTALL_COMMAND "") - -set(SUBSTRAIT_SUPPRESSED_WARNINGS) -if(MSVC) - # Protobuf generated files trigger some spurious warnings on MSVC. - - # Implicit conversion from uint64_t to uint32_t: - list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4244") - - # Missing dll-interface: - list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4251") -endif() - -file(MAKE_DIRECTORY "${SUBSTRAIT_GEN_DIR}/substrait") - -set(SUBSTRAIT_PROTO_GEN_ALL) -foreach(SUBSTRAIT_PROTO ${SUBSTRAIT_PROTOS}) - set(SUBSTRAIT_PROTO_GEN "${SUBSTRAIT_GEN_DIR}/substrait/${SUBSTRAIT_PROTO}.pb") - - foreach(EXT h cc) - set_source_files_properties("${SUBSTRAIT_PROTO_GEN}.${EXT}" - PROPERTIES COMPILE_OPTIONS - "${SUBSTRAIT_SUPPRESSED_WARNINGS}" - GENERATED TRUE - SKIP_UNITY_BUILD_INCLUSION TRUE) - add_custom_command(OUTPUT "${SUBSTRAIT_PROTO_GEN}.${EXT}" - COMMAND ${ARROW_PROTOBUF_PROTOC} "-I${SUBSTRAIT_LOCAL_DIR}/proto" - "--cpp_out=${SUBSTRAIT_GEN_DIR}" - "${SUBSTRAIT_LOCAL_DIR}/proto/substrait/${SUBSTRAIT_PROTO}.proto" - DEPENDS ${PROTO_DEPENDS} substrait_ep) - list(APPEND SUBSTRAIT_PROTO_GEN_ALL "${SUBSTRAIT_PROTO_GEN}.${EXT}") - endforeach() - - list(APPEND ARROW_ENGINE_SRCS "${SUBSTRAIT_PROTO_GEN}.cc") -endforeach() - -add_custom_target(substrait_gen ALL DEPENDS ${SUBSTRAIT_PROTO_GEN_ALL}) - -find_package(Git) -add_custom_target(substrait_gen_verify - COMMENT "Verifying that generated substrait accessors are consistent with \ - ARROW_SUBSTRAIT_REPO_AND_TAG='${ARROW_SUBSTRAIT_REPO_AND_TAG}'" - COMMAND ${GIT_EXECUTABLE} diff --exit-code ${SUBSTRAIT_GEN_DIR} - DEPENDS substrait_gen_clear - DEPENDS substrait_gen) - add_arrow_lib(arrow_engine CMAKE_PACKAGE_NAME ArrowEngine @@ -106,6 +35,8 @@ add_arrow_lib(arrow_engine arrow-engine OUTPUTS ARROW_ENGINE_LIBRARIES + DEPENDENCIES + ${SUBSTRAIT_DEPENDENCIES} SOURCES ${ARROW_ENGINE_SRCS} PRECOMPILED_HEADERS @@ -115,13 +46,13 @@ add_arrow_lib(arrow_engine SHARED_LINK_LIBS arrow_shared arrow_dataset_shared - ${ARROW_ENGINE_LINK_LIBS} + ${SUBSTRAIT_SHARED} STATIC_LINK_LIBS arrow_static arrow_dataset_static - ${ARROW_ENGINE_LINK_LIBS} + ${SUBSTRAIT_STATIC} PRIVATE_INCLUDES - ${SUBSTRAIT_GEN_DIR}) + ${SUBSTRAIT_INCLUDES}) foreach(LIB_TARGET ${ARROW_ENGINE_LIBRARIES}) target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_ENGINE_EXPORTING) diff --git a/cpp/thirdparty/versions.txt b/cpp/thirdparty/versions.txt index 6a86b4d4bdc..eb3b445430f 100644 --- a/cpp/thirdparty/versions.txt +++ b/cpp/thirdparty/versions.txt @@ -87,6 +87,8 @@ ARROW_SNAPPY_BUILD_SHA256_CHECKSUM=75c1fbb3d618dd3a0483bff0e26d0a92b495bbe5059c8 # There is a bug in GCC < 4.9 with Snappy 1.1.9, so revert to 1.1.8 for those (ARROW-14661) ARROW_SNAPPY_OLD_BUILD_VERSION=1.1.8 ARROW_SNAPPY_OLD_BUILD_SHA256_CHECKSUM=16b677f07832a612b0836178db7f374e414f94657c138e6993cbfc5dcc58651f +ARROW_SUBSTRAIT_BUILD_VERSION=e1b4c04a +ARROW_SUBSTRAIT_BUILD_SHA256_CHECKSUM=65f83e5f5d979ede5fc8ac9f8bbaf793e0c72d9c415f1a162ba522f6d0bb5bbe ARROW_THRIFT_BUILD_VERSION=0.13.0 ARROW_THRIFT_BUILD_SHA256_CHECKSUM=7ad348b88033af46ce49148097afe354d513c1fca7c607b59c33ebb6064b5179 ARROW_UTF8PROC_BUILD_VERSION=v2.7.0 From b40ca72fe1b51baad6f989f3e96b1fb41184de15 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Thu, 17 Feb 2022 21:35:54 +0100 Subject: [PATCH 02/11] Force Substrait build from source --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 60cae3b903d..8b532724527 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1696,7 +1696,7 @@ macro(build_substrait) ${ARROW_PROTOBUF_LIBPROTOBUF} STATIC_LINK_LIBS ${ARROW_PROTOBUF_LIBPROTOBUF} - EXTRA_INCLUDES + PRIVATE_INCLUDES ${SUBSTRAIT_CPP_DIR}) set(SUBSTRAIT_DEPENDENCIES substrait_gen) @@ -1706,6 +1706,8 @@ macro(build_substrait) endmacro() if(ARROW_WITH_SUBSTRAIT) + # Currently, we can only build Substrait from source. + set(Substrait_SOURCE "BUNDLED") resolve_dependency(Substrait) endif() From fd6e5d4369b4e3258fd2fd0cd92c5a3f3f119491 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Fri, 18 Feb 2022 11:48:12 +0100 Subject: [PATCH 03/11] Remove defunct CMake options --- cpp/cmake_modules/DefineOptions.cmake | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/cpp/cmake_modules/DefineOptions.cmake b/cpp/cmake_modules/DefineOptions.cmake index 30b1d0e075b..05fc14bbc72 100644 --- a/cpp/cmake_modules/DefineOptions.cmake +++ b/cpp/cmake_modules/DefineOptions.cmake @@ -478,16 +478,6 @@ advised that if this is enabled 'install' will fail silently on components;\ that have not been built" OFF) - set(ARROW_SUBSTRAIT_REPO_DEFAULT "https://github.com/substrait-io/substrait") - define_option_string(ARROW_SUBSTRAIT_REPO - "Custom git repository URL for downloading Substrait sources.;\ -See also ARROW_SUBSTRAIT_TAG" "${ARROW_SUBSTRAIT_REPO_DEFAULT}") - - set(ARROW_SUBSTRAIT_TAG_DEFAULT "e1b4c04a1b518912f4c4065b16a1b2c0ac8e14cf") - define_option_string(ARROW_SUBSTRAIT_TAG - "Custom git hash/tag/branch for Substrait repository.;\ -See also ARROW_SUBSTRAIT_REPO" "${ARROW_SUBSTRAIT_TAG_DEFAULT}") - option(ARROW_BUILD_CONFIG_SUMMARY_JSON "Summarize build configuration in a JSON file" ON) endif() From 66c8cc28a5bcf68c7e5bb915877d51aa11f71694 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Mon, 21 Feb 2022 10:52:33 +0100 Subject: [PATCH 04/11] Use object library to compile Substrait --- cpp/cmake_modules/BuildUtils.cmake | 8 ++++++++ cpp/cmake_modules/ThirdpartyToolchain.cmake | 22 ++++++--------------- cpp/src/arrow/engine/CMakeLists.txt | 7 ++++--- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index 391c43e0ac5..3afecfc5621 100644 --- a/cpp/cmake_modules/BuildUtils.cmake +++ b/cpp/cmake_modules/BuildUtils.cmake @@ -297,6 +297,14 @@ function(ADD_ARROW_LIB LIB_NAME) target_precompile_headers(${LIB_NAME}_objlib PRIVATE ${ARG_PRECOMPILED_HEADERS}) endif() set(LIB_DEPS $) + # We need to specify $ explicitly to SHARED + # library and STATIC library because $ in + # OBJECT isn't propagated to SHARED library and STATIC library. + foreach(ARG_SOURCE ${ARG_SOURCES}) + if(ARG_SOURCE MATCHES "^\\$ PRECOMPILED_HEADERS "$<$:arrow/engine/pch.h>" SHARED_LINK_FLAGS @@ -46,11 +47,11 @@ add_arrow_lib(arrow_engine SHARED_LINK_LIBS arrow_shared arrow_dataset_shared - ${SUBSTRAIT_SHARED} + ${ARROW_PROTOBUF_LIBPROTOBUF} STATIC_LINK_LIBS arrow_static arrow_dataset_static - ${SUBSTRAIT_STATIC} + ${ARROW_PROTOBUF_LIBPROTOBUF} PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES}) From 577979b3016f3b39d3cda5d1a5c20a6e333b4e26 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Tue, 22 Feb 2022 10:40:35 +0100 Subject: [PATCH 05/11] Try using static library instead --- cpp/cmake_modules/BuildUtils.cmake | 8 -------- cpp/cmake_modules/ThirdpartyToolchain.cmake | 9 +++++---- cpp/src/arrow/engine/CMakeLists.txt | 5 ++--- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index 3afecfc5621..391c43e0ac5 100644 --- a/cpp/cmake_modules/BuildUtils.cmake +++ b/cpp/cmake_modules/BuildUtils.cmake @@ -297,14 +297,6 @@ function(ADD_ARROW_LIB LIB_NAME) target_precompile_headers(${LIB_NAME}_objlib PRIVATE ${ARG_PRECOMPILED_HEADERS}) endif() set(LIB_DEPS $) - # We need to specify $ explicitly to SHARED - # library and STATIC library because $ in - # OBJECT isn't propagated to SHARED library and STATIC library. - foreach(ARG_SOURCE ${ARG_SOURCES}) - if(ARG_SOURCE MATCHES "^\\$ PRECOMPILED_HEADERS "$<$:arrow/engine/pch.h>" SHARED_LINK_FLAGS @@ -47,11 +46,11 @@ add_arrow_lib(arrow_engine SHARED_LINK_LIBS arrow_shared arrow_dataset_shared - ${ARROW_PROTOBUF_LIBPROTOBUF} + substrait STATIC_LINK_LIBS arrow_static arrow_dataset_static - ${ARROW_PROTOBUF_LIBPROTOBUF} + substrait PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES}) From b108e3e63d93ac192a5cd7c647ccc0822561ee18 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Tue, 22 Feb 2022 22:26:42 +0100 Subject: [PATCH 06/11] Remove FindSubstrait.cmake, since Substrait_SOURCE is forced to BUNDLED anyway --- cpp/cmake_modules/FindSubstrait.cmake | 19 ------------------- 1 file changed, 19 deletions(-) delete mode 100644 cpp/cmake_modules/FindSubstrait.cmake diff --git a/cpp/cmake_modules/FindSubstrait.cmake b/cpp/cmake_modules/FindSubstrait.cmake deleted file mode 100644 index dce8e086d1f..00000000000 --- a/cpp/cmake_modules/FindSubstrait.cmake +++ /dev/null @@ -1,19 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -# Currently, we can only build Substrait from source. -set(SUBSTRAIT_FOUND NO) From 3d7bac937bc42f275575eab37bc0e6de95803d00 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Tue, 22 Feb 2022 22:45:41 +0100 Subject: [PATCH 07/11] Add substrait to ARROW_BUNDLED_STATIC_LIBS if compiled --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 5c689e6ae86..dfd7fa37129 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1694,6 +1694,8 @@ macro(build_substrait) target_include_directories(substrait PUBLIC ${SUBSTRAIT_INCLUDES}) target_link_libraries(substrait INTERFACE ${ARROW_PROTOBUF_LIBPROTOBUF}) add_dependencies(substrait substrait_gen) + + list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait) endmacro() if(ARROW_WITH_SUBSTRAIT) From fffa65ce4540b94d062fad1a6ffa33ae45b5b20f Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Tue, 22 Feb 2022 23:06:40 +0100 Subject: [PATCH 08/11] Update dependency management in add_arrow_lib() as per @kou's suggestions --- cpp/cmake_modules/BuildUtils.cmake | 15 +++++++++++++++ cpp/src/arrow/engine/CMakeLists.txt | 2 -- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index 391c43e0ac5..7e5cfefa374 100644 --- a/cpp/cmake_modules/BuildUtils.cmake +++ b/cpp/cmake_modules/BuildUtils.cmake @@ -375,6 +375,14 @@ function(ADD_ARROW_LIB LIB_NAME) LINK_PRIVATE ${ARG_SHARED_PRIVATE_LINK_LIBS}) + if(USE_OBJLIB) + foreach(ARG_SHARED_LINK_LIB ${ARG_SHARED_LINK_LIBS}) + if(TARGET ${ARG_SHARED_LINK_LIB}) + add_dependencies(${LIB_NAME}_objlib ${ARG_SHARED_LINK_LIB}) + endif() + endforeach() + endif() + if(ARROW_RPATH_ORIGIN) if(APPLE) set(_lib_install_rpath "@loader_path") @@ -449,6 +457,13 @@ function(ADD_ARROW_LIB LIB_NAME) if(ARG_STATIC_LINK_LIBS) target_link_libraries(${LIB_NAME}_static LINK_PRIVATE "$") + if(USE_OBJLIB) + foreach(ARG_STATIC_LINK_LIB ${ARG_STATIC_LINK_LIBS}) + if(TARGET ${ARG_STATIC_LINK_LIB}) + add_dependencies(${LIB_NAME}_objlib ${ARG_STATIC_LINK_LIB}) + endif() + endforeach() + endif() endif() install(TARGETS ${LIB_NAME}_static ${INSTALL_IS_OPTIONAL} diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt index 13d50b42443..edb878939f5 100644 --- a/cpp/src/arrow/engine/CMakeLists.txt +++ b/cpp/src/arrow/engine/CMakeLists.txt @@ -35,8 +35,6 @@ add_arrow_lib(arrow_engine arrow-engine OUTPUTS ARROW_ENGINE_LIBRARIES - DEPENDENCIES - substrait SOURCES ${ARROW_ENGINE_SRCS} PRECOMPILED_HEADERS From 7ad2e1f3ba6efd179d9b79444fc484d948078f20 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Thu, 24 Feb 2022 23:11:49 +0100 Subject: [PATCH 09/11] Use single add_custom_commands for both protoc outputs --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index dfd7fa37129..82ed145b139 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1674,13 +1674,13 @@ macro(build_substrait) "${SUBSTRAIT_SUPPRESSED_WARNINGS}" GENERATED TRUE SKIP_UNITY_BUILD_INCLUSION TRUE) - add_custom_command(OUTPUT "${SUBSTRAIT_PROTO_GEN}.${EXT}" - COMMAND ${ARROW_PROTOBUF_PROTOC} "-I${SUBSTRAIT_LOCAL_DIR}/proto" - "--cpp_out=${SUBSTRAIT_CPP_DIR}" - "${SUBSTRAIT_LOCAL_DIR}/proto/substrait/${SUBSTRAIT_PROTO}.proto" - DEPENDS ${PROTO_DEPENDS} substrait_ep) list(APPEND SUBSTRAIT_PROTO_GEN_ALL "${SUBSTRAIT_PROTO_GEN}.${EXT}") endforeach() + add_custom_command(OUTPUT "${SUBSTRAIT_PROTO_GEN}.cc" "${SUBSTRAIT_PROTO_GEN}.h" + COMMAND ${ARROW_PROTOBUF_PROTOC} "-I${SUBSTRAIT_LOCAL_DIR}/proto" + "--cpp_out=${SUBSTRAIT_CPP_DIR}" + "${SUBSTRAIT_LOCAL_DIR}/proto/substrait/${SUBSTRAIT_PROTO}.proto" + DEPENDS ${PROTO_DEPENDS} substrait_ep) list(APPEND SUBSTRAIT_SOURCES "${SUBSTRAIT_PROTO_GEN}.cc") endforeach() From c2380ee8a839005b757e6063e0dee3a9d33b6282 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Thu, 24 Feb 2022 23:17:11 +0100 Subject: [PATCH 10/11] Add comments to additions in BuildUtils.cmake --- cpp/cmake_modules/BuildUtils.cmake | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index 7e5cfefa374..dc3ff8a4600 100644 --- a/cpp/cmake_modules/BuildUtils.cmake +++ b/cpp/cmake_modules/BuildUtils.cmake @@ -376,6 +376,8 @@ function(ADD_ARROW_LIB LIB_NAME) ${ARG_SHARED_PRIVATE_LINK_LIBS}) if(USE_OBJLIB) + # Ensure that dependencies are built before compilation of objects in + # object library, rather than only before the final link step foreach(ARG_SHARED_LINK_LIB ${ARG_SHARED_LINK_LIBS}) if(TARGET ${ARG_SHARED_LINK_LIB}) add_dependencies(${LIB_NAME}_objlib ${ARG_SHARED_LINK_LIB}) @@ -458,6 +460,8 @@ function(ADD_ARROW_LIB LIB_NAME) target_link_libraries(${LIB_NAME}_static LINK_PRIVATE "$") if(USE_OBJLIB) + # Ensure that dependencies are built before compilation of objects in + # object library, rather than only before the final link step foreach(ARG_STATIC_LINK_LIB ${ARG_STATIC_LINK_LIBS}) if(TARGET ${ARG_STATIC_LINK_LIB}) add_dependencies(${LIB_NAME}_objlib ${ARG_STATIC_LINK_LIB}) From eb88cfd64e7879a19bffddbe934ecffbb20eed99 Mon Sep 17 00:00:00 2001 From: Jeroen van Straten Date: Thu, 24 Feb 2022 23:18:07 +0100 Subject: [PATCH 11/11] Remove ARG_ prefix for loop variables --- cpp/cmake_modules/BuildUtils.cmake | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index dc3ff8a4600..87932ba766c 100644 --- a/cpp/cmake_modules/BuildUtils.cmake +++ b/cpp/cmake_modules/BuildUtils.cmake @@ -378,9 +378,9 @@ function(ADD_ARROW_LIB LIB_NAME) if(USE_OBJLIB) # Ensure that dependencies are built before compilation of objects in # object library, rather than only before the final link step - foreach(ARG_SHARED_LINK_LIB ${ARG_SHARED_LINK_LIBS}) - if(TARGET ${ARG_SHARED_LINK_LIB}) - add_dependencies(${LIB_NAME}_objlib ${ARG_SHARED_LINK_LIB}) + foreach(SHARED_LINK_LIB ${ARG_SHARED_LINK_LIBS}) + if(TARGET ${SHARED_LINK_LIB}) + add_dependencies(${LIB_NAME}_objlib ${SHARED_LINK_LIB}) endif() endforeach() endif() @@ -462,9 +462,9 @@ function(ADD_ARROW_LIB LIB_NAME) if(USE_OBJLIB) # Ensure that dependencies are built before compilation of objects in # object library, rather than only before the final link step - foreach(ARG_STATIC_LINK_LIB ${ARG_STATIC_LINK_LIBS}) - if(TARGET ${ARG_STATIC_LINK_LIB}) - add_dependencies(${LIB_NAME}_objlib ${ARG_STATIC_LINK_LIB}) + foreach(STATIC_LINK_LIB ${ARG_STATIC_LINK_LIBS}) + if(TARGET ${STATIC_LINK_LIB}) + add_dependencies(${LIB_NAME}_objlib ${STATIC_LINK_LIB}) endif() endforeach() endif()