refactor: Consolidate device extension into main project #517
Conversation
CMakeLists.txt
Outdated
| @@ -16,7 +16,7 @@ | |||
| # under the License. | |||
|
|
|||
| message(STATUS "Building using CMake version: ${CMAKE_VERSION}") | |||
| cmake_minimum_required(VERSION 3.14) | |||
| cmake_minimum_required(VERSION 3.22) | |||
There was a problem hiding this comment.
The version bump is required to support the DEPENDS condition in the dependent option:
https://cmake.org/cmake/help/v3.29/module/CMakeDependentOption.html
Might be a creative way to work around this if 3.14 is a hard requirement
There was a problem hiding this comment.
I believe 3.14 comes from centos7's cmake3. I think for now you can just ignore the option dependencies...I am doing an overhaul of the device code in the next few weeks and I'm not sure exactly what the final CMake options will be.
| @@ -493,6 +606,55 @@ if(NANOARROW_BUILD_TESTS) | |||
| gtest_discover_tests(nanoarrow_ipc_files_test) | |||
| gtest_discover_tests(nanoarrow_ipc_hpp_test) | |||
| endif() | |||
|
|
|||
| if(NANOARROW_DEVICE) | |||
| enable_testing() | |||
There was a problem hiding this comment.
There are a few things here that are duplicative of what happens in the IPC branch directly preceding it, but didn't try to solve that as part of this PR. Figured a copy/paste from the old structure into the new would be easiest to review
| @@ -66,8 +66,6 @@ def get_version(): | |||
| # Breathe configuration | |||
| breathe_projects = { | |||
There was a problem hiding this comment.
Not super sure on this change - what should I be looking for with the documentation as part of this?
There was a problem hiding this comment.
The place where this shows up is: https://arrow.apache.org/nanoarrow/latest/reference/device.html
...and I think that a few lines of
arrow-nanoarrow/docs/source/reference/device.rst
Lines 24 to 41 in ec1a692
and
https://github.com/apache/arrow-nanoarrow/blob/main/docs/source/reference/ipc.rst?plain=1#L24-L33
...might need to be updated (the new project would just be nanoarrow_c).
There was a problem hiding this comment.
Just a few comments! I agree that there's some repetition here that could be cleaned up now that all the projects are in the same CMakeLists.txt; however, this is a vast improvement and I think getting the bundling out of CMake will help simplify things considerably.
I'm working on the device extension over the next few weeks and will circle back to this CMake setup/file structure (e.g., I think we might want src/nanoarrow/device|ipc/xxx.c and perhaps to move some of the CMake into the subdirectories).
CMakeLists.txt
Outdated
| @@ -16,7 +16,7 @@ | |||
| # under the License. | |||
|
|
|||
| message(STATUS "Building using CMake version: ${CMAKE_VERSION}") | |||
| cmake_minimum_required(VERSION 3.14) | |||
| cmake_minimum_required(VERSION 3.22) | |||
There was a problem hiding this comment.
I believe 3.14 comes from centos7's cmake3. I think for now you can just ignore the option dependencies...I am doing an overhaul of the device code in the next few weeks and I'm not sure exactly what the final CMake options will be.
CMakeLists.txt
Outdated
| if((NANOARROW_DEVICE OR NANOARROW_IPC) AND (NANOARROW_BUILD_TESTS OR NOT NANOARROW_BUNDLE | ||
| )) |
There was a problem hiding this comment.
Maybe wrap the flatcc bit with if(NANOARROW_IPC) so that it doesn't get run if we're only building with NANOARROW_DEVICE?
There was a problem hiding this comment.
I actually think it was a mistake to include NANOARROW_DEVICE here - the nanoarrow_device target on main includes the flatcc headers as part of the build interface but I don't think actually requires it. Just removed that instead to keep this section untouched
README.md
Outdated
| @@ -132,3 +132,53 @@ meson test nanoarrow: # default test run | |||
| meson test nanoarrow: --wrap valgrind # run tests under valgrind | |||
| meson test nanoarrow: --benchmark --verbose # run benchmarks | |||
| ``` | |||
|
|
|||
| # nanoarrow device option | |||
There was a problem hiding this comment.
This will need updating shortly, so I pasted it into #497 (comment) and I think it might be better to leave it out for now (until the device behaviour has converged).
| @@ -66,8 +66,6 @@ def get_version(): | |||
| # Breathe configuration | |||
| breathe_projects = { | |||
There was a problem hiding this comment.
The place where this shows up is: https://arrow.apache.org/nanoarrow/latest/reference/device.html
...and I think that a few lines of
arrow-nanoarrow/docs/source/reference/device.rst
Lines 24 to 41 in ec1a692
and
https://github.com/apache/arrow-nanoarrow/blob/main/docs/source/reference/ipc.rst?plain=1#L24-L33
...might need to be updated (the new project would just be nanoarrow_c).
No description provided.