GH-45796: [C++] Add integration directory to Meson configuration #45797
Conversation
|
|
|
|
| exc = executable( | ||
| 'arrow-json-integration-test', | ||
| sources: ['json_integration_test.cc'], | ||
| dependencies: [arrow_dep, rapidjson_dep, gflags_dep, gtest_dep], |
There was a problem hiding this comment.
@kou there is some inconsistency in the tests here that you alluded to before. Most tests do not define a main function and therefore use gtest_main_dep as a dependency. However, this JSON integration test does define its own main, so you have to link just gtest_dep or you end up with duplicate main symbols.
I don't know the history of this test enough to know if it is intentional that some tests define their own main function while others do not. Does it make sense to have two different dependencies or should we consider removing main from this test module?
There was a problem hiding this comment.
I think that this is intentional. The test is used for an unit test and an integration test with other implementations. If the test is used for an integration test with other implementations, we pass a custom --integration flag. So we have a custom main().
Could you use gtest_dep not gtest_main_dep here?
There was a problem hiding this comment.
Sounds good - yes this is already using gtest_dep :-)
github-actions
bot
added
awaiting committer review
cpp/src/arrow/meson.build
Outdated
| else | ||
| rapidjson_dep = disabler() | ||
| endif | ||
| rapidjson_dep = dependency('rapidjson', include_type: 'system') |
There was a problem hiding this comment.
Why do we need to remove the if needs_json check?
There was a problem hiding this comment.
The integration tests requires RapidJSON but does not require the JSON option to be set, so to get that to work without disabling the integration test I think this need to exist without branching
There was a problem hiding this comment.
OK. Then can we use if needs_json or needs_integration instead of removing this condition?
| exc = executable( | ||
| 'arrow-json-integration-test', | ||
| sources: ['json_integration_test.cc'], | ||
| dependencies: [arrow_dep, rapidjson_dep, gflags_dep, gtest_dep], |
There was a problem hiding this comment.
I think that this is intentional. The test is used for an unit test and an integration test with other implementations. If the test is used for an integration test with other implementations, we pass a custom --integration flag. So we have a custom main().
Could you use gtest_dep not gtest_main_dep here?
github-actions
bot
added
awaiting changes
WillAyd
force-pushed
the
meson-add-integration-directory
branch
from
467fc64 to
1311f6e
Compare
|
@github-actions crossbow submit *meson |
|
Revision: 1311f6e Submitted crossbow builds: ursacomputing/crossbow @ actions-d35184ccea
|
|
@github-actions crossbow submit *meson |
|
Revision: d10ffa7 Submitted crossbow builds: ursacomputing/crossbow @ actions-c950246fa9
|
Rationale for this change
This continues to add features to the Meson build configuration
What changes are included in this PR?
The integration directory has been added
Are these changes tested?
Yes
Are there any user-facing changes?
No