-
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
Conversation
f58bd3a to
6f5675f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit amazon* ubuntu-18.04 ubuntu-20.04 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1f48a2d to
c7b478c
Compare
|
@github-actions crossbow submit ubuntu-18.04 |
This comment was marked as outdated.
This comment was marked as outdated.
c7b478c to
10a4387
Compare
|
@github-actions crossbow submit verify-rc-source-cpp-linux-ubuntu-18.04-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit verify-rc-source-cpp-linux-ubuntu-*-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
|
@kou Should this be updated for git master? Is there anything left to do otherwise? |
Yes.
I need to confirm that the current failures are fixed by rebasing on the master. If there are unknown failures, I need to fix them in this pull request. (I prioritized 8.0.0 release related tasks over this.) |
10a4387 to
cfd32d1
Compare
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
cfd32d1 to
be18666
Compare
be18666 to
b583fd7
Compare
|
@github-actions crossbow submit almalinux-8-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit almalinux-8-amd64 ubuntu-bionic-amd64 test-ubuntu-20.04-cpp-thread-sanitizer wheel-windows-cp310-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
This comment was marked as outdated.
This comment was marked as outdated.
|
This is ready to merge. |
| # Libraries to link statically with libarrow.so | ||
| set(ARROW_LINK_LIBS) | ||
| set(ARROW_STATIC_LINK_LIBS) | ||
| set(ARROW_LINK_LIBS arrow::flatbuffers arrow::hadoop) |
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::hadoop enabled 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.cc unconditionally.
Note that arrow::hadoop is a header only target that refers cpp/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.
cpp/CMakeLists.txt
Outdated
| set(ARROW_STATIC_LINK_LIBS arrow::flatbuffers arrow::hadoop) | ||
| set(ARROW_STATIC_INSTALL_INTERFACE_LIBS) | ||
|
|
||
| if(TARGET Boost::headers) |
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.
This looks a bit weird, shouldn't the condition be based on enabled Arrow components instead?
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.
OK. I'll use ARROW_BOOST_REQUIRED.
| elseif(ARG_SHARED_LIB) | ||
| set(AUG_LIB_NAME "${LIB_NAME}_shared") | ||
| add_library(${AUG_LIB_NAME} SHARED IMPORTED) | ||
|
|
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.
Nice simplification here!
| set(BUILD_SHARED_LIBS ON) | ||
| else() | ||
| # Find static boost headers and libs | ||
| # TODO Differentiate here between release and debug builds |
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.
Can you open a JIRA for this TODO?
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.
This is just copied from removed FindBoostAlt.cmake...
To be honest, I don't know what should be done by this TODO. We may be able to just remove this TODO.
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, let's just remove it.
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.
|
@kou Is the AppVeyor failure unrelated? |
|
@github-actions crossbow submit -g nightly-tests -g nightly-packaging -g nightly-release |
|
Revision: e643481 Submitted crossbow builds: ursacomputing/crossbow @ actions-2061 |
|
+1 |
|
Benchmark runs are scheduled for baseline = bb190bb and contender = d653b71. d653b71 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
We can remove "include_directories(SYSTEM)" by this.