paleolimbot commented on code in PR #677:
URL: https://github.com/apache/arrow-nanoarrow/pull/677#discussion_r1839338625
##########
CMakeLists.txt:
##########
@@ -422,26 +423,28 @@ if(NANOARROW_BUILD_TESTS)
)
include(CTest)
- find_package(Arrow REQUIRED)
- message(STATUS "Arrow version: ${ARROW_VERSION}")
- message(STATUS "Arrow SO version: ${ARROW_FULL_SO_VERSION}")
+ find_package(Arrow)
Review Comment:
```suggestion
if(NANOARROW_BUILD_TESTS_WITH_ARROW)
find_package(Arrow REQUIRED)
endif()
```
...maybe?
##########
CMakePresets.json:
##########
@@ -21,6 +21,7 @@
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"NANOARROW_BUILD_TESTS": "ON"
+ "NANOARROW_BUILD_TESTS_WITH_ARROW": "ON"
Review Comment:
I think we should probably leave this out of the defaults (I would keep it
in my own CMakeUserPresets, but most people wouldn't need this when making
their first contribution).
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -1719,7 +1812,8 @@ TEST(ArrayTest, ArrayTestAppendToRunEndEncodedArray) {
EXPECT_EQ(ArrowArrayFinishBuilding(&array, NANOARROW_VALIDATION_LEVEL_FULL,
&error),
NANOARROW_OK);
-#if !defined(ARROW_VERSION_MAJOR) || ARROW_VERSION_MAJOR < 12
+#if !defined(NANOARROW_BUILD_TESTS_WITH_ARROW) ||
!defined(ARROW_VERSION_MAJOR) || \
Review Comment:
Either is OK by me! I think it makes sense that if the version isn't defined
that we didn't build with arrow C++ (but also it might make searching for all
the Arrow C++ conditional test blocks easier to include the extra check).
##########
dev/release/verify-release-candidate.sh:
##########
@@ -243,12 +243,14 @@ test_cmake_project() {
test_c() {
show_header "Build and test C library"
- test_cmake_project build . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON
+ test_cmake_project build . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON \
+ -DNANOARROW_BUILD_TESTS_WITH_ARROW=ON
Review Comment:
For verification it would be useful to default this to OFF (because it
significantly improves the instructions required for verifying). It can always
be turned on (and perhaps we should turn it on in
.github/workflows/verify.yaml) via the extra cmake args.
--
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]
