Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Make Arrow C++ dependency optional #677

Merged
merged 9 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build-and-test-device.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ jobs:
cd build
export VERBOSE=1
cmake .. -DCMAKE_BUILD_TYPE=Debug -DNANOARROW_DEVICE=ON \
-DNANOARROW_BUILD_TESTS=ON -DCMAKE_PREFIX_PATH="${ARROW_PATH}" \
${{ matrix.config.cmake_args }}
-DNANOARROW_BUILD_TESTS=ON -DNANOARROW_BUILD_TESTS_WITH_ARROW=ON \
-DCMAKE_PREFIX_PATH="${ARROW_PATH}" ${{ matrix.config.cmake_args }}

cmake --build .

Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/build-and-test-ipc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ jobs:
cd build

cmake .. -DCMAKE_BUILD_TYPE=Debug -DNANOARROW_BUILD_TESTS=ON \
-DNANOARROW_IPC=ON -DCMAKE_PREFIX_PATH="${ARROW_PATH}" \
${{ matrix.config.cmake_args }}
-DNANOARROW_BUILD_TESTS_WITH_ARROW=ON -DNANOARROW_IPC=ON \
-DCMAKE_PREFIX_PATH="${ARROW_PATH}" ${{ matrix.config.cmake_args }}

cmake --build .

Expand Down
21 changes: 20 additions & 1 deletion .github/workflows/build-and-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ jobs:
cd build

cmake .. -DNANOARROW_BUILD_TESTS=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON \
-DCMAKE_PREFIX_PATH="${ARROW_PATH}" ${{ matrix.config.cmake_args }}
-DNANOARROW_BUILD_TESTS_WITH_ARROW=ON -DCMAKE_PREFIX_PATH="${ARROW_PATH}" \
${{ matrix.config.cmake_args }}

cmake --build .

Expand Down Expand Up @@ -158,3 +159,21 @@ jobs:
- name: Run meson testing script
run: |
PKG_CONFIG_PATH="$(pwd)/arrow/lib/pkgconfig" ci/scripts/build-with-meson.sh

test-no-arrow:
name: test-no-arrow
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- name: Build nanoarrow
run: |
cmake -S . -B build -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_BUILD_TESTS_WITH_ARROW=OFF \
-DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_PREFIX_PATH="${ARROW_PATH}"

cmake --build build

- name: Run tests
run: |
cd build
ctest -T test --output-on-failure .
6 changes: 3 additions & 3 deletions .github/workflows/clang-tidy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ jobs:
cd build

cmake .. -DNANOARROW_DEVICE=ON -DNANOARROW_IPC=ON \
-DNANOARROW_BUILD_TESTS=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON \
-DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DCMAKE_PREFIX_PATH="${ARROW_PATH}"
-DNANOARROW_BUILD_TESTS=ON -DNANOARROW_BUILD_TESTS_WITH_ARROW=ON \
-DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_PREFIX_PATH="${ARROW_PATH}"

cmake --build .

Expand Down
78 changes: 61 additions & 17 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ option(NANOARROW_DEVICE_WITH_CUDA "Build CUDA libraries" OFF)
# Development options
option(NANOARROW_BUILD_APPS "Build utility applications" OFF)
option(NANOARROW_BUILD_TESTS "Build tests" OFF)
option(NANOARROW_BUILD_TESTS_WITH_ARROW "Build tests that require Arrow" OFF)
option(NANOARROW_BUILD_BENCHMARKS "Build benchmarks" OFF)
option(NANOARROW_BUILD_INTEGRATION_TESTS
"Build cross-implementation Arrow integration tests" OFF)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
find_package(Arrow)
if(NANOARROW_BUILD_TESTS_WITH_ARROW)
find_package(Arrow REQUIRED)
endif()

...maybe?

if(Arrow_FOUND)
message(STATUS "Arrow version: ${ARROW_VERSION}")
message(STATUS "Arrow SO version: ${ARROW_FULL_SO_VERSION}")

# Give caller the option to link a static version of Arrow C++
if(NANOARROW_ARROW_STATIC)
set(NANOARROW_ARROW_TARGET arrow_static)
else()
set(NANOARROW_ARROW_TARGET arrow_shared)
endif()
# Give caller the option to link a static version of Arrow C++
if(NANOARROW_ARROW_STATIC)
set(NANOARROW_ARROW_TARGET arrow_static)
else()
set(NANOARROW_ARROW_TARGET arrow_shared)
endif()

# Arrow >= 10.0.0 requires C++17; GTest requires C++11.
# Leave the option open to use an older version of Arrow
# to make it easier to test on old Linux (e.g., Centos7)
if(${ARROW_VERSION} VERSION_GREATER_EQUAL "10.0.0")
set(CMAKE_CXX_STANDARD 17)
else()
set(CMAKE_CXX_STANDARD 11)
# Arrow >= 10.0.0 requires C++17; GTest requires C++11.
# Leave the option open to use an older version of Arrow
# to make it easier to test on old Linux (e.g., Centos7)
if(${ARROW_VERSION} VERSION_GREATER_EQUAL "10.0.0")
set(CMAKE_CXX_STANDARD 17)
else()
set(CMAKE_CXX_STANDARD 11)
endif()
set(CMAKE_CXX_STANDARD_REQUIRED ON)
endif()
set(CMAKE_CXX_STANDARD_REQUIRED ON)

add_subdirectory("thirdparty/googletest")

Expand Down Expand Up @@ -517,6 +520,27 @@ if(NANOARROW_BUILD_TESTS)
gmock_main
nanoarrow_coverage_config)

list(APPEND
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just kept the loop scoped to adding the define for now, although there is (as you've already mentioned) potential to use this for other parts of the config

NanoarrowTests
utils_test
buffer_test
array_test
schema_test
array_stream_test
nanoarrow_testing_test
c_data_integration_test
hpp_array_stream
hpp_buffer
hpp_exception
hpp_unique
hpp_view)
if(Arrow_FOUND)
foreach(test_target ${NanoarrowTests})
target_compile_definitions(${test_target}
PRIVATE -DNANOARROW_BUILD_TESTS_WITH_ARROW)
endforeach()
endif()

include(GoogleTest)
# Some users have reported a timeout with the default value of 5
# Building with -DBUILD_SHARED_LIBS=ON may also help reduce the size of test
Expand Down Expand Up @@ -569,6 +593,11 @@ if(NANOARROW_BUILD_TESTS)
target_link_libraries(nanoarrow_ipc_${name}_test flatccrt)
endif()

if(Arrow_FOUND)
target_compile_definitions(nanoarrow_ipc_${name}_test
PRIVATE -DNANOARROW_BUILD_TESTS_WITH_ARROW)
endif()

gtest_discover_tests(nanoarrow_ipc_${name}_test)
endforeach()

Expand All @@ -593,6 +622,13 @@ if(NANOARROW_BUILD_TESTS)
gtest_main
nanoarrow_coverage_config)

if(Arrow_FOUND)
target_compile_definitions(nanoarrow_device_test
PRIVATE -DNANOARROW_BUILD_TESTS_WITH_ARROW)
target_compile_definitions(nanoarrow_device_hpp_test
PRIVATE -DNANOARROW_BUILD_TESTS_WITH_ARROW)
endif()

include(GoogleTest)
gtest_discover_tests(nanoarrow_device_test)
gtest_discover_tests(nanoarrow_device_hpp_test)
Expand All @@ -604,6 +640,10 @@ if(NANOARROW_BUILD_TESTS)
nanoarrow
gtest_main
nanoarrow_coverage_config)
if(Arrow_FOUND)
target_compile_definitions(nanoarrow_device_metal_test
PRIVATE -DNANOARROW_BUILD_TESTS_WITH_ARROW)
endif()
gtest_discover_tests(nanoarrow_device_metal_test)
endif()

Expand All @@ -615,6 +655,10 @@ if(NANOARROW_BUILD_TESTS)
CUDA::cuda_driver
gtest_main
nanoarrow_coverage_config)
if(Arrow_FOUND)
target_compile_definitions(nanoarrow_device_cuda_test
PRIVATE -DNANOARROW_BUILD_TESTS_WITH_ARROW)
endif()
gtest_discover_tests(nanoarrow_device_cuda_test)
endif()
endif()
Expand Down
1 change: 1 addition & 0 deletions CMakePresets.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"NANOARROW_BUILD_TESTS": "ON"
"NANOARROW_BUILD_TESTS_WITH_ARROW": "ON"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
},
{
Expand Down
13 changes: 10 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,16 +127,23 @@ cmake ..
cmake --build .
```

Building nanoarrow with tests currently requires [Arrow C++](https://arrow.apache.org/install/).
If installed via a system package manager like `apt`, `dnf`, or `brew`, the tests can be
built with:
To build nanoarrow along with tests run:

```sh
mkdir build && cd build
cmake .. -DNANOARROW_BUILD_TESTS=ON
cmake --build .
```

If you are able to install [Arrow C++](https://arrow.apache.org/install/) you can enable
more testing:

```sh
mkdir build && cd build
cmake .. -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_BUILD_TESTS_WITH_ARROW=ON
cmake --build .
```

Tests can be run with `ctest`.

### Building with Meson
Expand Down
3 changes: 2 additions & 1 deletion ci/scripts/coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ function main() {

cmake "${TARGET_NANOARROW_DIR}" \
-DNANOARROW_DEVICE=ON -DNANOARROW_IPC=ON \
-DNANOARROW_BUILD_TESTS=ON -DNANOARROW_CODE_COVERAGE=ON
-DNANOARROW_BUILD_TESTS=ON -DNANOARROW_BUILD_TESTS_WITH_ARROW=ON \
-DNANOARROW_CODE_COVERAGE=ON
cmake --build .
CTEST_OUTPUT_ON_FAILURE=1 ctest .

Expand Down
6 changes: 4 additions & 2 deletions dev/release/verify-release-candidate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

test_c_bundled() {
show_header "Build test bundled C library"
test_cmake_project build-bundled . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON -DNANOARROW_BUNDLE=ON
test_cmake_project build-bundled . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON \
-DNANOARROW_BUILD_TESTS_WITH_ARROW=ON -DNANOARROW_BUNDLE=ON
}

test_r() {
Expand Down
12 changes: 10 additions & 2 deletions meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,16 @@ if get_option('tests')
# Similarly code coverage has a built in option users should use instead
# https://mesonbuild.com/Unit-tests.html#coverage

# The system include suppresses compilation warnings from Arrow
arrow_dep = dependency('arrow', include_type: 'system')
arrow_dep = dependency('arrow', include_type: 'system', required: false)
if get_option('tests_with_arrow') and not arrow_dep.found()
error('tests_with_arrow=true but could not find Arrow')
endif

test_cpp_args = []
if get_option('tests_with_arrow')
test_cpp_args += ['-DNANOARROW_BUILD_TESTS_WITH_ARROW']
endif

gtest_dep = dependency('gtest_main')
gmock_dep = dependency('gmock')

Expand Down
1 change: 1 addition & 0 deletions meson.options
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# under the License.

option('tests', type: 'boolean', description: 'Build tests', value: false)
option('tests_with_arrow', type: 'boolean', description: 'Build tests with Arrow', value: false)
option('benchmarks', type: 'boolean', description: 'Build benchmarks', value: false)
option('apps', type: 'boolean', description: 'Build utility applications', value: false)
option('ipc', type: 'boolean', description: 'Build IPC libraries', value: false)
Expand Down
Loading
Loading