Standalone 1/N: Move driver source and test files to separate directories #293
Conversation
aliddell
changed the title
src/CMakeLists.txt
Outdated
| if (NOT TARGET acquire-core-logger) | ||
| add_subdirectory(../acquire-common/acquire-core-libs ${CMAKE_CURRENT_BINARY_DIR}/acquire-core-libs) | ||
| endif () | ||
| set(CMAKE_POSITION_INDEPENDENT_CODE ON) |
There was a problem hiding this comment.
Question: Why do we need to specify PIC? I understand it's typically required for shared libraries, but is it necessary for the driver code or the API? If it's needed for the driver, should we relocate the PIC settings to the driver's CMakeLists file?
There was a problem hiding this comment.
I can save this for the streaming library.
src/driver/CMakeLists.txt
Outdated
| @@ -0,0 +1,61 @@ | |||
| option(BUILD_ACQUIRE_DRIVER_ZARR "Build the Acquire Zarr driver" ON) | |||
There was a problem hiding this comment.
It makes more sense to set this option in the root CMakeLists file and conditionally add the driver's subdirectory, rather than wrapping this entire configuration file in an if statement.
There was a problem hiding this comment.
Do you mean the top level or the one in src/?
tests/CMakeLists.txt
Outdated
| add_dependencies(${tgt} ${project}-copy-${driver}-for-tests) | ||
| endforeach () | ||
| endforeach () | ||
| add_subdirectory(driver) |
There was a problem hiding this comment.
Rather than using a separate top-level CMake file for tests, you could simply include this subdirectory in the driver's CMake file.
tests/driver/CMakeLists.txt
Outdated
| @@ -0,0 +1,87 @@ | |||
| if (BUILD_ACQUIRE_DRIVER_ZARR) | |||
There was a problem hiding this comment.
This relates to the previous comment. If you add this subdirectory from the driver's CMake file (which is already added based on the same condition), you could remove this if-statement.
tests/driver/CMakeLists.txt
Outdated
| # | ||
| # Tests | ||
| # | ||
| set(tests |
There was a problem hiding this comment.
Note: While I believe it's important to manually list all source and header files in the project's main build configuration, I prefer using file(GLOB TEST_SOURCES ${CMAKE_CURRENT_LIST_DIR}/**/*.cpp) for test-related builds. This dynamic file loading reduces overhead for test configurations.
This is just a suggestion—feel free to disregard. However, I'd be interested in your thoughts if you disagree.
There was a problem hiding this comment.
That's an interesting suggestion, but I think I'll save it for later experimentation.
Defines but does not implement the API. Depends on #293.
Splitting up #291. Moves files and updates CMakeLists.txt files accordingly.