Uh oh!
There was an error while loading. Please reload this page.
GH-46827: [C++] Update Meson Configuration for compute shared lib - #46830
GH-46827: [C++] Update Meson Configuration for compute shared lib#46830WillAyd wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
The Meson configuration was actually failing to register all of the compute kernels (at least locally). When looking further into this, it appears that googletest actually suggests providing your own main function when registering a custom environment like this, to ensure that initialization happens in proper order.
There was a problem hiding this comment.
Without bumping googletest, it appears that a lot of the tests end up failing with:
../subprojects/googletest-1.15.2/googletest/include/gtest/internal/gtest-port.h:1810:: pthread_key_delete(key_)failed with error 22
There's a large amount of discussion upstream in both the Meson WrapDB and googletest about this, which ultimately traces back to the ODR and how gtest is linked into an application.
I figure there's no harm in updating this, but happy to link to the further discussions if anyone is interested
There was a problem hiding this comment.
FWIW this is a minor deviation from the CMake configuration. In the CMake configuration, the bridge_test links to the arrow_compute_XXX libs, but those libs have not yet been declared because they are located in the compute/CMakeLists.txt file, which is included after c/CMakeLists.txt
Without knowing how CMake works, I'm not sure if that's a problem or not, but the Meson configuration definitely complains about arrow_compute_dep does not exist if you try to maintain the same inclusion order as CMake
3cf8596 to
1e243c8CompareWillAyd
commented
Jun 16, 2025
Hmm I'm stumped on the AppVeyor failure and not sure how to debug - if anyone has any ideas let me know! |
| 'arrow-acero', | ||
| sources: arrow_acero_srcs, | ||
| dependencies: [arrow_dep], | ||
| dependencies: [arrow_dep, arrow_compute_dep], |
There was a problem hiding this comment.
arrow_compute_dep must be placed before arrow_dep because arrow_compute_dep depends on arrow_dep:
| dependencies: [arrow_dep, arrow_compute_dep], | |
| dependencies: [arrow_compute_dep, arrow_dep], |
| filebase: 'arrow-acero', | ||
| name: 'Apache Arrow Acero Engine', | ||
| description: 'Apache Arrow\'s Acero Engine', | ||
| requires: ['arrow', 'arrow-compute'], |
There was a problem hiding this comment.
We don't need arrow here because arrow-compute has Requires: arrow:
| requires: ['arrow', 'arrow-compute'], | |
| requires: ['arrow-compute'], |
| ] + arrow_compute_testing_srcs, | ||
| dependencies: [arrow_test_dep], | ||
| ], | ||
| dependencies: [arrow_test_dep, arrow_compute_core_test_dep], |
There was a problem hiding this comment.
| dependencies: [arrow_test_dep, arrow_compute_core_test_dep], | |
| dependencies: [arrow_compute_core_test_dep, arrow_test_dep], |
| @@ -96,16 +127,14 @@ compute_tests = { | |||
| # - value_counts | |||
| # | |||
| # Also see: GH-34388, GH-34615 | |||
There was a problem hiding this comment.
I didn't step through all of the kernels but this comment still holds true, no?
There was a problem hiding this comment.
Ah, removing the if needs_compute is just by introducing arrow_compute_test_dep, right?
raulcd
commented
Jun 17, 2025
I am also really confused on the appveyor failure. I am unsure what is the difference with the rest of our C++ Windows builds and why it fails there. |
I guess the AppVeyor build does not want to use the gtest approach where the custom environment requires its own main function. All green now, although I'm not sure if the macro is the best way to go about this |
WillAyd
commented
Jun 17, 2025
Hmm github was not reflecting my updates and I can't reopen now. Haven't run into that before...will debug in a new PR. If this can be reopened let me know! |
WillAyd
commented
Jun 17, 2025
The new PR seemed equally messed up. I think github is just having an outage - will check back in a few hours and see if this can be revived |
277f081 to
c2de059Compare
Rationale for this change
The major factor of the compute sources in #46261 addressed updates to the CMake configuration but not Meson. This is a follow up to get the Meson builds working again
What changes are included in this PR?
Meson configuration files are updated to reflect new source structure. gtest has also been bumped to a new WrapDB version, which fixes some undefined behavior that was compounded by updates to the compute test structure
Are these changes tested?
Yes
Are there any user-facing changes?
No