ARROW-15709: [C++] Compilation of ARROW_ENGINE fails if doing an "inline" build

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[jvanstraten]

This should fix:

• inline builds in general (ARROW-15709);
• weird stuff with inline builds causing non-tracked files to be deleted that the previous fix for the above caused
• dependencies on git for downloading dependencies (ARROW-15760);
• the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. ThirdpartyToolchain.cmake)

---

Initial attempt at making something functional to solve this issue properly.

The use of add_arrow_lib in ThirdpartyToolchain.cmake is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues:

• The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the externalproject_add over to ThirdpartyToolchain.cmake resulted in the add_arrow_lib in src/arrow/engine failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using add_arrow_lib.
• Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so Substrait_SOURCE=SYSTEM makes no sense. In the end I just decided to override it, but that's probably not ideal.
• Substrait doesn't have releases yet, so I had to resort to a git hash instead.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15709

[westonpace]

@kou@pitrou We could use input on this approach. The Substrait integration is a little unique because the .proto files themselves are not owned by us. We could fork a copy of the proto files but that would make keeping up with updated versions a bit tricky.

Protobuf is also a bit tricky because the version of the code generation tool (protoc) must match the version of the linked protobuf library exactly (this is different from flatbuffers which doesn't require a runtime dependency).

Previously we were downloading the proto files using git and running protoc within the engine directory. This had some negative side effects (#12454) mainly due to our use of git. Even beyond git however it was rather strange that this was resulting in a call to externalproject_add outside of ThirdPartyToolchain.cmake so @jvanstraten and I agreed it might be clear to treat Substrait as a "build-from-source only" third party dependency. This PR is @jvanstraten 's attempt to make that work but it is also odd as it requires a call to add_arrow_lib inside of ThirdPartyToolchain.cmake.

Any suggestions on a better approach or feedback on our current approach would be very much welcomed.

[kou]

How about using just an OBJECT library https://cmake.org/cmake/help/latest/command/add_library.html#object-lib instead of arrow_add_lib()?

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..3afecfc562 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 $<TARGET_OBJECTS:${LIB_NAME}_objlib>)
+ # We need to specify $<TARGET_OBJECTS:XXX> explicitly to SHARED+ # library and STATIC library because $<TARGET_OBJECTS:XXX> in+ # OBJECT isn't propagated to SHARED library and STATIC library.+ foreach(ARG_SOURCE ${ARG_SOURCES})+ if(ARG_SOURCE MATCHES "^\\$<TARGET_OBJECTS:")+ list(APPEND LIB_DEPS ${ARG_SOURCE})+ endif()+ endforeach()
set(LIB_INCLUDES)
set(EXTRA_DEPS)
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 8b53272452..9143550df9 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake@@ -1687,22 +1687,13 @@ macro(build_substrait)
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}- PRIVATE_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})+ add_library(substrait OBJECT ${SUBSTRAIT_SOURCES})+ set_target_properties(substrait+ PROPERTIES+ INCLUDE_DIRECTORIES ${SUBSTRAIT_CPP_DIR}+ POSITION_INDEPENDENT_CODE ON)+ add_dependencies(substrait substrait_gen)+ get_target_property(SUBSTRAIT_INCLUDES substrait INCLUDE_DIRECTORIES)
endmacro()
if(ARROW_WITH_SUBSTRAIT)
diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt
index 50a84464c0..ff88f47e4a 100644
--- a/cpp/src/arrow/engine/CMakeLists.txt+++ b/cpp/src/arrow/engine/CMakeLists.txt@@ -36,9 +36,10 @@ add_arrow_lib(arrow_engine
OUTPUTS
ARROW_ENGINE_LIBRARIES
DEPENDENCIES
- ${SUBSTRAIT_DEPENDENCIES}+ substrait
SOURCES
${ARROW_ENGINE_SRCS}
+ $<TARGET_OBJECTS:substrait>
PRECOMPILED_HEADERS
"$<$<COMPILE_LANGUAGE:CXX>:arrow/engine/pch.h>"
SHARED_LINK_FLAGS
@@ -46,11 +47,9 @@ add_arrow_lib(arrow_engine
SHARED_LINK_LIBS
arrow_shared
arrow_dataset_shared
- ${SUBSTRAIT_SHARED}
STATIC_LINK_LIBS
arrow_static
arrow_dataset_static
- ${SUBSTRAIT_STATIC}
PRIVATE_INCLUDES
${SUBSTRAIT_INCLUDES})

[jvanstraten]

@kou Thanks for the input! It's certainly a lot nicer to do it that way, but unfortunately, we're not quite there yet... I got it working in most build environments using your diff with some minor changes (had to add protobuf to the include list for the object library, and add its runtime library to arrow_engine), but it fails here:

CMake Error at cmake_modules/BuildUtils.cmake:287 (add_library):
OBJECT library "arrow_engine_objlib" contains:
capabilities.pb.cc.o
expression.pb.cc.o
extensions.pb.cc.o
function.pb.cc.o
parameterized_types.pb.cc.o
plan.pb.cc.o
relations.pb.cc.o
type.pb.cc.o
type_expressions.pb.cc.o
but may contain only sources that compile, header files, and other files
that would not affect linking of a normal library.

I feel like the addition to BuildUtils.cmake is either causing this or supposed to resolve this (and failing to do so in this particular case), but other than that, my skill level for CMake is insufficient to see what exactly is going on here.

[pitrou]

+1, this will be much better than involving some git commands.

[pitrou]

Hmm... is there a particular reason for this? Should we instead create a static library containing these objects? Or perhaps I'm entirely misunderstanding this?

[kou]

We can use a static library instead of an object library without using add_arrow_lib(). We can just use raw add_library().

[jvanstraten]

Looks like that's working, 577979b. I wasn't sure how generally-applicable linking static libraries with -fPIC into shared libraries would be. I'm also not sure why I still need to specify PRIVATE_INCLUDES ${SUBSTRAIT_INCLUDES} for the add_arrow_lib call (shouldn't CMake be propagating public include directories to dependent libraries?), but I couldn't get it to work without it.

[kou]

Can we remove this file?
It seems that this file is never used.

[jvanstraten]

b108e3e

[kou]

Can we remove this?
This will be automatically added by using substrait target.

[jvanstraten]

It doesn't build (multithreaded) without it. I think it ensures that substrait is built completely prior to compilation of arrow_engine, rather than just prior to linking, which matters because arrow_engine uses generated code. Making it depend on substrait_gen would also work, if you prefer that option (IMO, the less arrow_engine depends on implementation details of ThirdpartyToolchain.cmake the better, so I set it to just substrait).

[kou]

It seems that this is a problem of our add_arrow_lib():

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index 391c43e0ac..7517e4e806 100644
--- a/cpp/cmake_modules/BuildUtils.cmake+++ b/cpp/cmake_modules/BuildUtils.cmake@@ -374,6 +374,13 @@ function(ADD_ARROW_LIB LIB_NAME)
"$<INSTALL_INTERFACE:${ARG_SHARED_INSTALL_INTERFACE_LIBS}>"
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)
@@ -449,6 +456,13 @@ function(ADD_ARROW_LIB LIB_NAME)
if(ARG_STATIC_LINK_LIBS)
target_link_libraries(${LIB_NAME}_static LINK_PRIVATE
"$<BUILD_INTERFACE:${ARG_STATIC_LINK_LIBS}>")
+ 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 13d50b4244..edb878939f 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

[jvanstraten]

Okay, that fixes it on my end, too. fffa65c

[kou]

Could you add substrait to ARROW_BUNDLED_STATIC_LIBS?

Suggested change

	add_dependencies(substraitsubstrait_gen)
	list(APPEND ARROW_BUNDLED_STATIC_LIBS substrait)

If we don't add this, users can't link to libarrow_engine.a statically with libarrow_bundled_dependencies.a.

[jvanstraten]

Added in 3d7bac9

The removal of add_dependencies(substrait substrait_gen) seems unrelated. I suppose it's technically redundant, since CMake will generate the .cc files by way of needing them as input for compiling substrait, and the .h files just so happen to be generated by the same command. If for whatever reason only the generated .h files are removed though, CMake won't know to recreate them without that rule.

[kou]

Ah, sorry. It's my fault.

[jvanstraten]

Failure of AMD64 Ubuntu 20.04 GLib & Ruby for
fffa65c seems to be unrelated (zlib download failure, I think). It builds correctly when I run it with archery locally.

[kou]

@github-actions crossbow submit python-sdist

[github-actions]

Revision: fffa65c

Submitted crossbow builds: ursacomputing/crossbow @ actions-1676

Task	Status
python-sdist

[jvanstraten]

@kou@pitrou@westonpace I believe this is ready to be merged, or am I still missing something?

[pitrou]

Can you add a comment explaining this snippet?

[jvanstraten]

Added something in c2380ee. @kou, please verify if what I wrote for it is correct and complete.

[kou]

Right.

[pitrou]

Minor, but naming ARG_SOMETHING a variable that is not one of the original function arguments is confusing.

[jvanstraten]

eb88cfd

[pitrou]

It seems like it compiles each proto file twice (once per extension)? Is that desired?

[jvanstraten]

No, probably not. I noticed it too when I inherited this code, but incorrectly assumed it was written like that because add_custom_command only supported a single output file. 7ad2e1f

[pitrou]

Perhaps @kou can also double-check the CMake changes.

[westonpace]

I have nothing further to add. I think this looks much nicer than what we had before. We might try and take some snippets from here and push them upstream into the Substrait project's documentation in some kind of "getting started with C++" section.

[kou]

+1

[kou]

Right.

[ursabot]

Benchmark runs are scheduled for baseline = acfd1d2 and contender = 89cc6b3. 89cc6b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.21% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java