GH-47283: [C++] Fix flight visibility issue in Meson configuration #47298
Conversation
|
|
github-actions
bot
added
Component: C++
Component: FlightRPC
awaiting review
WillAyd
force-pushed
the
fix-flight-visibility
branch
2 times, most recently
from
a959636 to
84ec299
Compare
WillAyd
force-pushed
the
fix-flight-visibility
branch
from
84ec299 to
209c2a9
Compare
WillAyd
force-pushed
the
fix-flight-visibility
branch
from
209c2a9 to
f040de5
Compare
github-actions
bot
added
awaiting merge
| arrow_testing_dep, | ||
| rapidjson_dep, | ||
| gflags_dep, | ||
| gtest_dep, |
There was a problem hiding this comment.
Do we need gtest_dep with arrow_testing_dep?
arrow_testing_dep includes gtest_main_dep.
There was a problem hiding this comment.
Its a bit confusing and there might be a better way to word things, but with this PR we now have a few dependencies being declared:
- arrow_testing_dep, which just exposes the testing library
- arrow_test_dep, which exposes multiple dependencies that are used to compile and run tests in arrow
- arrow_test_dep_no_main which is used by tests that declare their own main function
I could use arrow_test_dep_no_main here instead; it would add a few superfluous dependencies but is closer to what this test requires
There was a problem hiding this comment.
Ah, sorry. I was wrong.
Let's use arrow_test_dep_no_main here.
There was a problem hiding this comment.
OK - I think I've used that where it makes sense in this PR. Let me know what you think!
| arrow_testing_dep, | ||
| filesystem_dep, | ||
| gmock_dep, | ||
| gtest_main_dep, |
There was a problem hiding this comment.
Can we remove this with arrow_testing_dep?
| arrow_testing_dep, | ||
| filesystem_dep, | ||
| gmock_dep, | ||
| gtest_dep, |
There was a problem hiding this comment.
Can we remove this with arrow_testing_dep?
| arrow_dep, | ||
| arrow_testing_dep, | ||
| benchmark_main_dep, | ||
| gtest_dep, |
There was a problem hiding this comment.
Can we remove this with arrow_testing_dep?
cpp/src/arrow/util/meson.build
Outdated
| gtest_dep, | ||
| gmock_dep, |
There was a problem hiding this comment.
Can we remove them with arrow_testing_dep?
cpp/src/arrow/flight/meson.build
Outdated
| ], | ||
| ) | ||
|
|
||
| grpc_cpp = dependency('grpc++') |
There was a problem hiding this comment.
It seems that this isn't used in this PR.
There was a problem hiding this comment.
Surprisingly it is, because using this as a dependency will ensure that override_find_program is called for the grpc_cpp_plugin. Without this, if you don't have that installed on your system then the build will fail
There was a problem hiding this comment.
Hmm I take that back - we already have this up on L47. Let me take another look...
There was a problem hiding this comment.
Related to the comment above, this is transitively required by grpc (and in turn the grpc_cpp_plugin)
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
cpp/src/arrow/meson.build
Outdated
| arrow_testing_lib = static_library( | ||
| 'arrow_testing', | ||
| sources: arrow_testing_srcs, | ||
| dependencies: [arrow_dep, filesystem_dep, gmock_dep, gtest_main_dep], |
There was a problem hiding this comment.
Should we use gtest_dep not gtest_main_dep here because libarrow_testing doesn't need main()?
| dependencies: [arrow_dep, filesystem_dep, gmock_dep, gtest_main_dep], | |
| dependencies: [arrow_dep, filesystem_dep, gmock_dep, gtest_dep], |
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 3c460f7. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
3 participants
Rationale for this change
The meson configuration is missing some symbols for flight that appear to come from the testing library.
What changes are included in this PR?
Updates to the Meson configuration
Are these changes tested?
Yes
Are there any user-facing changes?
No