WillAyd commented on code in PR #905:
URL: https://github.com/apache/arrow-nanoarrow/pull/905#discussion_r3589758793
##########
meson.build:
##########
@@ -263,13 +263,18 @@ if get_option('tests').enabled()
# Similarly code coverage has a built in option users should use instead
# https://mesonbuild.com/Unit-tests.html#coverage
- arrow_dep = dependency('arrow', include_type: 'system', required: false)
if get_option('tests_with_arrow').enabled()
- if arrow_dep.found()
- test_cpp_args += ['-DNANOARROW_BUILD_TESTS_WITH_ARROW']
- else
- warning('tests_with_arrow option enabled but could not find Arrow')
- endif
+ # When tests_with_arrow is enabled, Arrow is required. Use pkg-config
first,
+ # then fall back to building from the wrapdb subproject if not found.
+ arrow_dep = dependency(
Review Comment:
```suggestion
arrow_dep = dependency('arrow')
```
Your comment is in essence what Meson does by default - no need to add all
the extra arguments.
Note that the arrow-cpp wrap already decalres the name of one of its
dependencies as `arrow` which maps to its internal `arrow_dep` variable:
https://github.com/mesonbuild/wrapdb/blob/1d2d25f4206f4d10856c889fd10e360f2225086c/subprojects/arrow-cpp.wrap#L10
##########
meson_options.txt:
##########
@@ -16,7 +16,7 @@
# under the License.
option('tests', type: 'feature', description: 'Build tests')
-option('tests_with_arrow', type: 'feature', description: 'Build tests with
Arrow')
+option('tests_with_arrow', type: 'feature', value: 'disabled', description:
'Build tests with Arrow')
Review Comment:
I actually don't know what this does - is this just making it so that
setting `-Dauto_features=enabled` will not set this any longer? By default
features are disabled
##########
meson.build:
##########
@@ -263,13 +263,18 @@ if get_option('tests').enabled()
# Similarly code coverage has a built in option users should use instead
# https://mesonbuild.com/Unit-tests.html#coverage
- arrow_dep = dependency('arrow', include_type: 'system', required: false)
if get_option('tests_with_arrow').enabled()
- if arrow_dep.found()
- test_cpp_args += ['-DNANOARROW_BUILD_TESTS_WITH_ARROW']
- else
- warning('tests_with_arrow option enabled but could not find Arrow')
- endif
+ # When tests_with_arrow is enabled, Arrow is required. Use pkg-config
first,
+ # then fall back to building from the wrapdb subproject if not found.
+ arrow_dep = dependency(
+ 'arrow',
+ include_type: 'system',
+ fallback: ['arrow-cpp', 'arrow_dep'],
+ )
+ test_cpp_args += ['-DNANOARROW_BUILD_TESTS_WITH_ARROW']
+ else
+ # When tests_with_arrow is disabled, Arrow is optional (for roundtrip
tests)
+ arrow_dep = dependency('arrow', include_type: 'system', required:
false)
Review Comment:
```suggestion
arrow_dep = dependency('arrow', required: false)
```
##########
ci/scripts/build-with-meson.sh:
##########
@@ -64,7 +64,8 @@ function main() {
meson setup "${SANDBOX_DIR}" \
--pkg-config-path $PKG_CONFIG_PATH \
-Dwerror=true \
- -Dbuildtype=release
+ -Dbuildtype=release \
+ -Dtests_with_arrow=enabled
Review Comment:
Not saying it needs to be solved here, but from a usability perspective I
think it would be best if we just had a tests option that was set, and the
configuration could figure out what to do to solve the arrow dependency issue
(or not). Having a configured setting like `tests_with_arrow` inherits a lot
from CMake :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
