-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-16168: [C++][CMake] Use target to add include paths #12861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
6efdfd1
ARROW-16168: [C++][CMake] Use target to add include paths
kou aa0cbfd
Suppress sign-compare warnings from g++
kou e00602b
Ensure creating xsimd's include directory
kou f538f93
Treat thirdparty library's include directory as system
kou 8ec1abd
Ensure making include directory
kou 72ee8b1
Add workaround for RapidJSON's CMake package
kou 0abf728
Define interface library for Winsock
kou b316be8
Define interface library for -lrt
kou 5e70b36
Add missing static link dependencies
kou 3baa7ae
Use target_link_libraries()
kou e9f87d5
Add missing libs
kou 4290208
Fix a typo
kou 4c62dbe
Create target for -ldl
kou c9ec064
Remove needless DEPENDENCIES
kou 7e96eb5
Don't use target for system libraries
kou 91a54e7
Remove include_directories(SYSTEM)
kou 89b1110
Add missing Boost dependencies
kou 104e8f9
Add missing Boost dependencies to Thrift
kou 3169e33
Fix indent
kou c8f2c1c
Clean Boost related targets
kou 37e5b6a
Boost::* may not be built
kou d4b6982
Add missing COMPONENTS
kou 4997123
Add missing Boost dependency
kou 984d8d5
Fix link order
kou 0ce320c
We can't use target_link_library(OBJECT_LIBRARY) with CMake < 3.12
kou 746ef88
Add support for CMake < 3.15
kou 6c3eeb0
Add missing utf8proc dependency
kou 6bcec5c
Fix dependency order
kou e3f8406
Add missing ARROW_FLIGHT_EXPORTING
kou 4362fbc
Add missing dependencies
kou 991256c
Debug
kou 048d7de
Remove duplicated entries
kou 591077d
Use static utf8proc
kou ffe7aec
Add support for CMake < 3.11 again
kou 6bee188
Fix a typo
kou 52dbe23
Fix style
kou 63c6680
Add missing RapidJSON dependency
kou b583fd7
Add support for Ubuntu 18.04
kou 9e878a0
Add missing arrow::flatbuffers dependency
kou d6284e3
Fix a typo
kou 6ed7965
Use if(VARIABLE) instead of if(TARGET)
kou d590421
Add a comment for object library and CMake < 3.12
kou 019c3f7
Add missing xsimd availability check
kou c6708e4
Use if(VARIABLE) instead of if(TARGET)
kou 8bbe58c
Add a note for arrow::hadoop
kou e643481
Remove meaningless TODO comment
kou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,25 +30,9 @@ set(ARROW_LIBRARY_PATH_SUFFIXES | |
| "Library/bin") | ||
| set(ARROW_INCLUDE_PATH_SUFFIXES "include" "Library" "Library/include") | ||
|
|
||
| set(ARROW_BOOST_PROCESS_COMPILE_DEFINITIONS) | ||
| if(WIN32 AND CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | ||
| # boost/process/detail/windows/handle_workaround.hpp doesn't work | ||
| # without BOOST_USE_WINDOWS_H with MinGW because MinGW doesn't | ||
| # provide __kernel_entry without winternl.h. | ||
| # | ||
| # See also: | ||
| # https://github.com/boostorg/process/blob/develop/include/boost/process/detail/windows/handle_workaround.hpp | ||
| # | ||
| # You can use this like the following: | ||
| # | ||
| # target_compile_definitions(target PRIVATE | ||
| # ${ARROW_BOOST_PROCESS_COMPILE_DEFINITIONS}) | ||
| list(APPEND ARROW_BOOST_PROCESS_COMPILE_DEFINITIONS "BOOST_USE_WINDOWS_H=1") | ||
| endif() | ||
|
|
||
| function(ADD_THIRDPARTY_LIB LIB_NAME) | ||
| function(add_thirdparty_lib LIB_NAME LIB_TYPE LIB) | ||
| set(options) | ||
| set(one_value_args SHARED_LIB STATIC_LIB) | ||
| set(one_value_args) | ||
| set(multi_value_args DEPS INCLUDE_DIRECTORIES) | ||
| cmake_parse_arguments(ARG | ||
| "${options}" | ||
|
|
@@ -59,78 +43,25 @@ function(ADD_THIRDPARTY_LIB LIB_NAME) | |
| message(SEND_ERROR "Error: unrecognized arguments: ${ARG_UNPARSED_ARGUMENTS}") | ||
| endif() | ||
|
|
||
| if(ARG_STATIC_LIB AND ARG_SHARED_LIB) | ||
| set(AUG_LIB_NAME "${LIB_NAME}_static") | ||
| add_library(${AUG_LIB_NAME} STATIC IMPORTED) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION | ||
| "${ARG_STATIC_LIB}") | ||
| if(ARG_DEPS) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_LINK_LIBRARIES | ||
| "${ARG_DEPS}") | ||
| endif() | ||
| message(STATUS "Added static library dependency ${AUG_LIB_NAME}: ${ARG_STATIC_LIB}") | ||
| if(ARG_INCLUDE_DIRECTORIES) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_INCLUDE_DIRECTORIES | ||
| "${ARG_INCLUDE_DIRECTORIES}") | ||
| endif() | ||
|
|
||
| set(AUG_LIB_NAME "${LIB_NAME}_shared") | ||
| add_library(${AUG_LIB_NAME} SHARED IMPORTED) | ||
|
|
||
| if(WIN32) | ||
| # Mark the ".lib" location as part of a Windows DLL | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_IMPLIB | ||
| "${ARG_SHARED_LIB}") | ||
| else() | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION | ||
| "${ARG_SHARED_LIB}") | ||
| endif() | ||
| if(ARG_DEPS) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_LINK_LIBRARIES | ||
| "${ARG_DEPS}") | ||
| endif() | ||
| message(STATUS "Added shared library dependency ${AUG_LIB_NAME}: ${ARG_SHARED_LIB}") | ||
| if(ARG_INCLUDE_DIRECTORIES) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_INCLUDE_DIRECTORIES | ||
| "${ARG_INCLUDE_DIRECTORIES}") | ||
| endif() | ||
| elseif(ARG_STATIC_LIB) | ||
| set(AUG_LIB_NAME "${LIB_NAME}_static") | ||
| add_library(${AUG_LIB_NAME} STATIC IMPORTED) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION | ||
| "${ARG_STATIC_LIB}") | ||
| if(ARG_DEPS) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_LINK_LIBRARIES | ||
| "${ARG_DEPS}") | ||
| endif() | ||
| message(STATUS "Added static library dependency ${AUG_LIB_NAME}: ${ARG_STATIC_LIB}") | ||
| if(ARG_INCLUDE_DIRECTORIES) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_INCLUDE_DIRECTORIES | ||
| "${ARG_INCLUDE_DIRECTORIES}") | ||
| endif() | ||
| elseif(ARG_SHARED_LIB) | ||
| set(AUG_LIB_NAME "${LIB_NAME}_shared") | ||
| add_library(${AUG_LIB_NAME} SHARED IMPORTED) | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice simplification here! |
||
| add_library(${LIB_NAME} ${LIB_TYPE} IMPORTED) | ||
| if(${LIB_TYPE} STREQUAL "STATIC") | ||
| set_target_properties(${LIB_NAME} PROPERTIES IMPORTED_LOCATION "${LIB}") | ||
| message(STATUS "Added static library dependency ${LIB_NAME}: ${LIB}") | ||
| else() | ||
| if(WIN32) | ||
| # Mark the ".lib" location as part of a Windows DLL | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_IMPLIB | ||
| "${ARG_SHARED_LIB}") | ||
| set_target_properties(${LIB_NAME} PROPERTIES IMPORTED_IMPLIB "${LIB}") | ||
| else() | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES IMPORTED_LOCATION | ||
| "${ARG_SHARED_LIB}") | ||
| endif() | ||
| message(STATUS "Added shared library dependency ${AUG_LIB_NAME}: ${ARG_SHARED_LIB}") | ||
| if(ARG_DEPS) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_LINK_LIBRARIES | ||
| "${ARG_DEPS}") | ||
| endif() | ||
| if(ARG_INCLUDE_DIRECTORIES) | ||
| set_target_properties(${AUG_LIB_NAME} PROPERTIES INTERFACE_INCLUDE_DIRECTORIES | ||
| "${ARG_INCLUDE_DIRECTORIES}") | ||
| set_target_properties(${LIB_NAME} PROPERTIES IMPORTED_LOCATION "${LIB}") | ||
| endif() | ||
| else() | ||
| message(FATAL_ERROR "No static or shared library provided for ${LIB_NAME}") | ||
| message(STATUS "Added shared library dependency ${LIB_NAME}: ${LIB}") | ||
| endif() | ||
| if(ARG_DEPS) | ||
| set_target_properties(${LIB_NAME} PROPERTIES INTERFACE_LINK_LIBRARIES "${ARG_DEPS}") | ||
| endif() | ||
| if(ARG_INCLUDE_DIRECTORIES) | ||
| set_target_properties(${LIB_NAME} PROPERTIES INTERFACE_INCLUDE_DIRECTORIES | ||
| "${ARG_INCLUDE_DIRECTORIES}") | ||
| endif() | ||
| endfunction() | ||
|
|
||
|
|
@@ -271,11 +202,16 @@ function(ADD_ARROW_LIB LIB_NAME) | |
| set(OUTPUT_PATH ${BUILD_OUTPUT_ROOT_DIRECTORY}) | ||
| endif() | ||
|
|
||
| if(WIN32 OR (CMAKE_GENERATOR STREQUAL Xcode)) | ||
| if(WIN32 | ||
| OR (CMAKE_GENERATOR STREQUAL Xcode) | ||
| OR CMAKE_VERSION VERSION_LESS 3.12) | ||
pitrou marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # We need to compile C++ separately for each library kind (shared and static) | ||
| # because of dllexport declarations on Windows. | ||
| # The Xcode generator doesn't reliably work with Xcode as target names are not | ||
| # guessed correctly. | ||
| # We can't use target for object library with CMake 3.11 or earlier. | ||
| # See also: Object Libraries: | ||
| # https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries | ||
| set(USE_OBJLIB OFF) | ||
| else() | ||
| set(USE_OBJLIB ON) | ||
|
|
@@ -310,6 +246,9 @@ function(ADD_ARROW_LIB LIB_NAME) | |
| if(ARG_PRIVATE_INCLUDES) | ||
| target_include_directories(${LIB_NAME}_objlib PRIVATE ${ARG_PRIVATE_INCLUDES}) | ||
| endif() | ||
| target_link_libraries(${LIB_NAME}_objlib | ||
| PRIVATE ${ARG_SHARED_LINK_LIBS} ${ARG_SHARED_PRIVATE_LINK_LIBS} | ||
| ${ARG_STATIC_LINK_LIBS}) | ||
| else() | ||
| # Prepare arguments for separate compilation of static and shared libs below | ||
| # TODO: add PCH directives | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is
arrow::hadoopenabled unconditionally??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Because we build
cpp/src/arrow/io_hdfs_internal.ccunconditionally.Note that
arrow::hadoopis a header only target that referscpp/thirdparty/hadoop/include/.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Can you add that last sentence as a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.