-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-39823: [C++] Allow building cpp/src/arrow/**/*.cc without waiting bundled libraries #39824
Conversation
@github-actions crossbow submit -g cpp |
|
This comment was marked as outdated.
This comment was marked as outdated.
3d69f11
to
6ac50a9
Compare
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
6ac50a9
to
9e0a933
Compare
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
https://github.com/apache/arrow/actions/runs/7705131604/job/20998591461?pr=39824#step:6:1465
|
0155e17
to
dcf95db
Compare
cpp/src/arrow/CMakeLists.txt
Outdated
if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | ||
set_property(SOURCE util/io_util.cc | ||
APPEND_STRING | ||
PROPERTY COMPILE_FLAGS " -Wno-unused-macros ") |
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 we perhaps use pragmas inside io_util.cc
to avoid this?
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, we can do it.
I'll add a change for 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.
I removed this and no CI weren't failed. So it seems that we don't need this now.
cpp/src/arrow/acero/CMakeLists.txt
Outdated
add_arrow_acero_test(pivot_longer_node_test SOURCES pivot_longer_node_test.cc | ||
test_nodes.cc) | ||
EXTRA_LINK_LIBS | ||
${ARROW_ACERO_TEST_LINK_LIBS} |
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.
Why can't this be added to add_arrow_acero_test
instead of repeating it in each test declaration?
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.
We can do it by:
diff --git a/cpp/src/arrow/acero/CMakeLists.txt b/cpp/src/arrow/acero/CMakeLists.txt
index 36afad4972..7f40d1bd19 100644
--- a/cpp/src/arrow/acero/CMakeLists.txt
+++ b/cpp/src/arrow/acero/CMakeLists.txt
@@ -139,10 +139,9 @@ function(add_arrow_acero_test REL_TEST_NAME)
set(PREFIX "arrow-acero")
endif()
+ set(EXTRA_LINK_LIBS ${ARROW_ACERO_TEST_LINK_LIBS})
if(ARG_EXTRA_LINK_LIBS)
- set(EXTRA_LINK_LIBS ${ARG_EXTRA_LINK_LIBS})
- else()
- set(EXTRA_LINK_LIBS ${ARROW_ACERO_TEST_LINK_LIBS})
+ list(APPEND EXTRA_LINK_LIBS ${ARG_EXTRA_LINK_LIBS})
endif()
if(ARG_LABELS)
but we use "override" instead of "append" when we specify a value explicitly in our functions.
So I used the same behavior for consistency here.
How about defining a variable to remove the duplication and keep consistency?
diff --git a/cpp/src/arrow/acero/CMakeLists.txt b/cpp/src/arrow/acero/CMakeLists.txt
index 36afad4972..2b1683b99a 100644
--- a/cpp/src/arrow/acero/CMakeLists.txt
+++ b/cpp/src/arrow/acero/CMakeLists.txt
@@ -161,51 +161,47 @@ function(add_arrow_acero_test REL_TEST_NAME)
${ARG_UNPARSED_ARGUMENTS})
endfunction()
+set(ARROW_ACERO_NODE_TEST_LINK_LIBS ${ARROW_ACERO_TEST_LINK_LIBS})
if(ARROW_TESTING)
add_library(arrow_acero_test_nodes OBJECT test_nodes.cc)
target_link_libraries(arrow_acero_test_nodes PRIVATE ${ARROW_ACERO_TEST_LINK_LIBS})
if(ARROW_WITH_OPENTELEMETRY)
target_link_libraries(arrow_acero_test_nodes PRIVATE ${ARROW_OPENTELEMETRY_LIBS})
endif()
+ list(APPEND ARROW_ACERO_NODE_TEST_LINK_LIBS arrow_acero_test_nodes)
endif()
add_arrow_acero_test(plan_test
SOURCES
plan_test.cc
test_nodes_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
add_arrow_acero_test(source_node_test
SOURCES
source_node_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
add_arrow_acero_test(fetch_node_test
SOURCES
fetch_node_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
add_arrow_acero_test(order_by_node_test
SOURCES
order_by_node_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
add_arrow_acero_test(hash_join_node_test
SOURCES
hash_join_node_test.cc
bloom_filter_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
add_arrow_acero_test(pivot_longer_node_test
SOURCES
pivot_longer_node_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
# asof_join_node and sorted_merge_node use std::thread internally
# and doesn't use ThreadPool so it will
@@ -215,14 +211,12 @@ if(ARROW_ENABLE_THREADING)
SOURCES
asof_join_node_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
add_arrow_acero_test(sorted_merge_node_test
SOURCES
sorted_merge_node_test.cc
EXTRA_LINK_LIBS
- ${ARROW_ACERO_TEST_LINK_LIBS}
- arrow_acero_test_nodes)
+ ${ARROW_ACERO_NODE_TEST_LINK_LIBS})
endif()
add_arrow_acero_test(tpch_node_test SOURCES tpch_node_test.cc)
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.
Well, can arrow_acero_test_nodes
just be linked into every Acero test?
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.
We can do it. Some tests don't need it. So it's just not used.
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.
dcf95db
to
57cde0c
Compare
a9437fc
to
811674a
Compare
Oh, I don't know why but the AppVeyor failures were fixed...: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/49291292?fullLog=true It seems that using gmock for all tests 95eb410 is related. I haven't touched |
@github-actions crossbow submit -g cpp -g r -g linux -g python -g r |
Revision: 7203b8a Submitted crossbow builds: ursacomputing/crossbow @ actions-255108a88a |
That's weird. Perhaps linking to GMock automatically enables it? |
@github-actions crossbow submit -g wheel |
Revision: 7203b8a Submitted crossbow builds: ursacomputing/crossbow @ actions-9f09457a19 |
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.
LGTM, but let's make sure CI is ok
Maybe? All wheel jobs are green. I'll merge this. |
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 909f6f9. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…iting bundled libraries (apache#39824) ### Rationale for this change If we can build most of `cpp/src/arrow/**/*.cc` before all bundled libraries are built, we can reduce build time. ### What changes are included in this PR? * Remove the `toolchain` internal CMake target * Remove `ARROW_SHARED_LINK_LIBS` * Remove `ARROW_STATIC_LINK_LIBS` * Move the following variables to `cpp/src/arrow/CMakeLists.txt` * `ARROW_SHARED_PRIVATE_LINK_LIBS` * `ARROW_SHARED_INSTALL_INTERFACE_LIBS` * `ARROW_STATIC_INSTALL_INTERFACE_LIBS` * `ARROW_TEST_LINK_TOOLCHAIN` * `ARROW_TEST_SHARED_LINK_LIBS` * `ARROW_TEST_STATIC_LINK_LIBS` * `ARROW_SYSTEM_LINK_LIBS` * Add internal `OBJECT` libraries that have minimal dependencies * Remove unused `cpp/src/arrow/util/benchmark_main.cc` ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#39823 * GitHub Issue: apache#39823 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…ic (#40433) ### Rationale for this change `libarrow.a` uses `std::mutex` and so on. So we need to link to `Threads::Threads`. But #39824 dropped it accidentally. ### What changes are included in this PR? Add unexpectedly dropped `Threads::Threads` dependency to `arrow_static` again. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: #40432 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
### Rationale for this change ```text LINK : warning LNK4217: symbol 'uriEscapeExA' defined in 'arrow_static.lib(unity_0_c.c.obj)' is imported by 'arrow_static.lib(unity_5_cxx.cxx.obj)' in function '"class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl arrow::internal::UriEscape(class std::basic_string_view<char,struct std::char_traits<char> >)" (?UriEscape@ internal@ arrow@@ YA?AV?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@ V?$basic_string_view@ DU?$char_traits@ D@ std@@@ 4@@ Z)' LINK : warning LNK4217: symbol 'uriUnescapeInPlaceA' defined in 'arrow_static.lib(unity_0_c.c.obj)' is imported by 'arrow_static.lib(unity_5_cxx.cxx.obj)' in function '"class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl arrow::internal::UriUnescape(class std::basic_string_view<char,struct std::char_traits<char> >)" (?UriUnescape@ internal@ arrow@@ YA?AV?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@ V?$basic_string_view@ DU?$char_traits@ D@ std@@@ 4@@ Z)' LINK : warning LNK4217: symbol 'uriWindowsFilenameToUriStringA' defined in 'arrow_static.lib(unity_0_c.c.obj)' is imported by 'arrow_static.lib(unity_5_cxx.cxx.obj)' in function '"class arrow::Result<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > > __cdecl arrow::internal::UriFromAbsolutePath(class std::basic_string_view<char,struct std::char_traits<char> >)" (?UriFromAbsolutePath@ internal@ arrow@@ YA?AV?$Result@ V?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@@ 2@ V?$basic_string_view@ DU?$char_traits@ D@ std@@@ std@@@ Z)' arrow_static.lib(unity_5_cxx.cxx.obj) : error LNK2019: unresolved external symbol __imp_uriParseSingleUriExA referenced in function "public: class arrow::Status __cdecl arrow::internal::Uri::Parse(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?Parse@ Uri@ internal@ arrow@@ QEAA?AVStatus@ 3@ AEBV?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@@ Z) arrow_static.lib(unity_5_cxx.cxx.obj) : error LNK2019: unresolved external symbol __imp_uriFreeUriMembersA referenced in function "public: __cdecl arrow::internal::Uri::Impl::~Impl(void)" (??1Impl@ Uri@ internal@ arrow@@ QEAA@ XZ) arrow_static.lib(unity_5_cxx.cxx.obj) : error LNK2019: unresolved external symbol __imp_uriDissectQueryMallocA referenced in function "public: class arrow::Result<class std::vector<struct std::pair<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,class std::allocator<struct std::pair<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > > > > > __cdecl arrow::internal::Uri::query_items(void)const " (?query_items@ Uri@ internal@ arrow@@ QEBA?AV?$Result@ V?$vector@ U?$pair@ V?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@ V12@@ std@@ V?$allocator@ U?$pair@ V?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@ V12@@ std@@@ 2@@ std@@@ 3@ XZ) arrow_static.lib(unity_5_cxx.cxx.obj) : error LNK2019: unresolved external symbol __imp_uriFreeQueryListA referenced in function "public: class arrow::Result<class std::vector<struct std::pair<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > >,class std::allocator<struct std::pair<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > > > > > __cdecl arrow::internal::Uri::query_items(void)const " (?query_items@ Uri@ internal@ arrow@@ QEBA?AV?$Result@ V?$vector@ U?$pair@ V?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@ V12@@ std@@ V?$allocator@ U?$pair@ V?$basic_string@ DU?$char_traits@ D@ std@@ V?$allocator@ D@ 2@@ std@@ V12@@ std@@@ 2@@ std@@@ 3@ XZ) aws-c-io.lib(secure_channel_tls_handler.c.obj) : error LNK2019: unresolved external symbol __imp_CertFreeCertificateContext referenced in function s_ctx_new aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2001: unresolved external symbol __imp_CertFreeCertificateContext aws-c-io.lib(secure_channel_tls_handler.c.obj) : error LNK2019: unresolved external symbol __imp_CertCreateCertificateChainEngine referenced in function s_manually_verify_peer_cert aws-c-io.lib(secure_channel_tls_handler.c.obj) : error LNK2019: unresolved external symbol __imp_CertFreeCertificateChainEngine referenced in function s_manually_verify_peer_cert aws-c-io.lib(secure_channel_tls_handler.c.obj) : error LNK2019: unresolved external symbol __imp_CertGetCertificateChain referenced in function s_manually_verify_peer_cert aws-c-io.lib(secure_channel_tls_handler.c.obj) : error LNK2019: unresolved external symbol __imp_CertFreeCertificateChain referenced in function s_manually_verify_peer_cert aws-c-io.lib(secure_channel_tls_handler.c.obj) : error LNK2019: unresolved external symbol __imp_CertVerifyCertificateChainPolicy referenced in function s_manually_verify_peer_cert aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CryptDecodeObjectEx referenced in function aws_import_key_pair_to_cert_context aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CertOpenStore referenced in function aws_import_key_pair_to_cert_context aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CertCloseStore referenced in function aws_close_cert_store aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CertFindCertificateInStore referenced in function aws_load_cert_from_system_cert_store aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CertSetCertificateContextProperty referenced in function aws_import_key_pair_to_cert_context aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CertAddCertificateContextToStore referenced in function aws_import_key_pair_to_cert_context aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CryptQueryObject referenced in function aws_import_key_pair_to_cert_context aws-c-io.lib(windows_pki_utils.c.obj) : error LNK2019: unresolved external symbol __imp_CryptStringToBinaryA referenced in function aws_load_cert_from_system_cert_store ``` ### What changes are included in this PR? * Add missing `URI_STATIC_BUILD` macro definition that was removed by #39824 accidentally. * Add missing `crypt32.lib` dependency to `aws-c-io`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #40445 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
If we can build most of
cpp/src/arrow/**/*.cc
before all bundled libraries are built, we can reduce build time.What changes are included in this PR?
toolchain
internal CMake targetARROW_SHARED_LINK_LIBS
ARROW_STATIC_LINK_LIBS
cpp/src/arrow/CMakeLists.txt
ARROW_SHARED_PRIVATE_LINK_LIBS
ARROW_SHARED_INSTALL_INTERFACE_LIBS
ARROW_STATIC_INSTALL_INTERFACE_LIBS
ARROW_TEST_LINK_TOOLCHAIN
ARROW_TEST_SHARED_LINK_LIBS
ARROW_TEST_STATIC_LINK_LIBS
ARROW_SYSTEM_LINK_LIBS
OBJECT
libraries that have minimal dependenciescpp/src/arrow/util/benchmark_main.cc
Are these changes tested?
Yes.
Are there any user-facing changes?
No.