-
Notifications
You must be signed in to change notification settings - Fork 5
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
Standalone 1/N: Move driver source and test files to separate directories #293
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,57 +1,3 @@ | ||
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) | ||
|
||
set(tgt acquire-driver-zarr) | ||
add_library(${tgt} MODULE | ||
common/dimension.hh | ||
common/dimension.cpp | ||
common/thread.pool.hh | ||
common/thread.pool.cpp | ||
common/s3.connection.hh | ||
common/s3.connection.cpp | ||
common/utilities.hh | ||
common/utilities.cpp | ||
writers/sink.hh | ||
writers/sink.creator.hh | ||
writers/sink.creator.cpp | ||
writers/file.sink.hh | ||
writers/file.sink.cpp | ||
writers/s3.sink.hh | ||
writers/s3.sink.cpp | ||
writers/array.writer.hh | ||
writers/array.writer.cpp | ||
writers/zarrv2.array.writer.hh | ||
writers/zarrv2.array.writer.cpp | ||
writers/zarrv3.array.writer.hh | ||
writers/zarrv3.array.writer.cpp | ||
writers/blosc.compressor.hh | ||
writers/blosc.compressor.cpp | ||
zarr.hh | ||
zarr.cpp | ||
zarr.v2.hh | ||
zarr.v2.cpp | ||
zarr.v3.hh | ||
zarr.v3.cpp | ||
zarr.driver.c | ||
) | ||
|
||
target_include_directories(${tgt} PRIVATE | ||
$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/src> | ||
) | ||
|
||
target_enable_simd(${tgt}) | ||
target_link_libraries(${tgt} PRIVATE | ||
acquire-core-logger | ||
acquire-core-platform | ||
acquire-device-kit | ||
acquire-device-properties | ||
blosc_static | ||
nlohmann_json::nlohmann_json | ||
miniocpp::miniocpp | ||
) | ||
set_target_properties(${tgt} PROPERTIES | ||
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>" | ||
) | ||
|
||
install(TARGETS ${tgt} LIBRARY DESTINATION lib) | ||
add_subdirectory(driver) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
option(BUILD_ACQUIRE_DRIVER_ZARR "Build the Acquire Zarr driver" ON) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean the top level or the one in src/? |
||
|
||
if (BUILD_ACQUIRE_DRIVER_ZARR) | ||
if (NOT TARGET acquire-core-logger) | ||
add_subdirectory(${CMAKE_SOURCE_DIR}/acquire-common/acquire-core-libs ${CMAKE_CURRENT_BINARY_DIR}/acquire-core-libs) | ||
endif () | ||
|
||
set(tgt acquire-driver-zarr) | ||
add_library(${tgt} MODULE | ||
common/dimension.hh | ||
common/dimension.cpp | ||
common/thread.pool.hh | ||
common/thread.pool.cpp | ||
common/s3.connection.hh | ||
common/s3.connection.cpp | ||
common/utilities.hh | ||
common/utilities.cpp | ||
writers/sink.hh | ||
writers/sink.creator.hh | ||
writers/sink.creator.cpp | ||
writers/file.sink.hh | ||
writers/file.sink.cpp | ||
writers/s3.sink.hh | ||
writers/s3.sink.cpp | ||
writers/array.writer.hh | ||
writers/array.writer.cpp | ||
writers/zarrv2.array.writer.hh | ||
writers/zarrv2.array.writer.cpp | ||
writers/zarrv3.array.writer.hh | ||
writers/zarrv3.array.writer.cpp | ||
writers/blosc.compressor.hh | ||
writers/blosc.compressor.cpp | ||
zarr.hh | ||
zarr.cpp | ||
zarr.v2.hh | ||
zarr.v2.cpp | ||
zarr.v3.hh | ||
zarr.v3.cpp | ||
zarr.driver.c | ||
) | ||
|
||
target_include_directories(${tgt} PRIVATE | ||
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> | ||
) | ||
|
||
target_enable_simd(${tgt}) | ||
target_link_libraries(${tgt} PRIVATE | ||
acquire-core-logger | ||
acquire-core-platform | ||
acquire-device-kit | ||
acquire-device-properties | ||
blosc_static | ||
nlohmann_json::nlohmann_json | ||
miniocpp::miniocpp | ||
) | ||
set_target_properties(${tgt} PROPERTIES | ||
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>" | ||
) | ||
|
||
install(TARGETS ${tgt} LIBRARY DESTINATION lib) | ||
endif () |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,90 +1,5 @@ | ||
if (${NOTEST}) | ||
message(STATUS "Skipping test targets") | ||
else () | ||
set(NOTEST "TRUE") | ||
add_subdirectory(../acquire-common/acquire-driver-common ${CMAKE_CURRENT_BINARY_DIR}/acquire-driver-common) | ||
add_subdirectory(../acquire-common/acquire-video-runtime ${CMAKE_CURRENT_BINARY_DIR}/acquire-video-runtime) | ||
set(NOTEST "FALSE") | ||
|
||
# | ||
# PARAMETERS | ||
# | ||
set(project acquire-driver-zarr) # CMAKE_PROJECT_NAME gets overridden if this is a subtree of another project | ||
|
||
# | ||
# Tests | ||
# | ||
set(tests | ||
list-devices | ||
unit-tests | ||
get | ||
get-meta | ||
get-set-get | ||
external-metadata-with-whitespace-ok | ||
restart-stopped-zarr-resets-threadpool | ||
repeat-start | ||
metadata-dimension-sizes | ||
write-zarr-v2-raw | ||
write-zarr-v2-raw-chunk-size-larger-than-frame-size | ||
write-zarr-v2-raw-with-even-chunking | ||
write-zarr-v2-raw-with-even-chunking-and-rollover | ||
write-zarr-v2-raw-with-ragged-chunking | ||
write-zarr-v2-with-lz4-compression | ||
write-zarr-v2-with-zstd-compression | ||
write-zarr-v2-compressed-with-chunking | ||
write-zarr-v2-compressed-with-chunking-and-rollover | ||
write-zarr-v2-raw-multiscale | ||
write-zarr-v2-raw-multiscale-with-trivial-tile-size | ||
write-zarr-v2-compressed-multiscale | ||
write-zarr-v2-to-s3 | ||
multiscales-metadata | ||
write-zarr-v3-raw | ||
write-zarr-v3-raw-with-ragged-sharding | ||
write-zarr-v3-raw-chunk-exceeds-array | ||
write-zarr-v3-compressed | ||
write-zarr-v3-raw-multiscale | ||
write-zarr-v3-to-s3 | ||
) | ||
|
||
foreach (name ${tests}) | ||
set(tgt "${project}-${name}") | ||
add_executable(${tgt} ${name}.cpp) | ||
target_compile_definitions(${tgt} PUBLIC "TEST=\"${tgt}\"") | ||
set_target_properties(${tgt} PROPERTIES | ||
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>" | ||
) | ||
target_include_directories(${tgt} PRIVATE "${CMAKE_CURRENT_LIST_DIR}/../") | ||
target_link_libraries(${tgt} | ||
acquire-core-logger | ||
acquire-core-platform | ||
acquire-video-runtime | ||
nlohmann_json::nlohmann_json | ||
miniocpp::miniocpp | ||
) | ||
|
||
add_test(NAME test-${tgt} COMMAND ${tgt}) | ||
set_tests_properties(test-${tgt} PROPERTIES LABELS "anyplatform;acquire-driver-zarr") | ||
endforeach () | ||
|
||
# | ||
# Copy driver to tests | ||
# | ||
list(POP_FRONT tests onename) | ||
|
||
foreach (driver | ||
acquire-driver-common | ||
acquire-driver-zarr | ||
) | ||
add_custom_target(${project}-copy-${driver}-for-tests | ||
COMMAND ${CMAKE_COMMAND} -E copy | ||
$<TARGET_FILE:${driver}> | ||
$<TARGET_FILE_DIR:${project}-${onename}> | ||
DEPENDS ${driver} | ||
COMMENT "Copying ${driver} to $<TARGET_FILE_DIR:${project}-${onename}>" | ||
) | ||
|
||
foreach (name ${tests}) | ||
add_dependencies(${tgt} ${project}-copy-${driver}-for-tests) | ||
endforeach () | ||
endforeach () | ||
add_subdirectory(driver) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than using a separate top-level CMake file for tests, you could simply include this subdirectory in the driver's CMake file. |
||
endif () |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
if (BUILD_ACQUIRE_DRIVER_ZARR) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
set(NOTEST "TRUE") | ||
add_subdirectory(${CMAKE_SOURCE_DIR}/acquire-common ${CMAKE_CURRENT_BINARY_DIR}/acquire-common) | ||
set(NOTEST "FALSE") | ||
|
||
# | ||
# PARAMETERS | ||
# | ||
set(project acquire-driver-zarr) # CMAKE_PROJECT_NAME gets overridden if this is a subtree of another project | ||
|
||
# | ||
# Tests | ||
# | ||
set(tests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an interesting suggestion, but I think I'll save it for later experimentation. |
||
list-devices | ||
unit-tests | ||
get | ||
get-meta | ||
get-set-get | ||
external-metadata-with-whitespace-ok | ||
restart-stopped-zarr-resets-threadpool | ||
repeat-start | ||
metadata-dimension-sizes | ||
write-zarr-v2-raw | ||
write-zarr-v2-raw-chunk-size-larger-than-frame-size | ||
write-zarr-v2-raw-with-even-chunking | ||
write-zarr-v2-raw-with-even-chunking-and-rollover | ||
write-zarr-v2-raw-with-ragged-chunking | ||
write-zarr-v2-with-lz4-compression | ||
write-zarr-v2-with-zstd-compression | ||
write-zarr-v2-compressed-with-chunking | ||
write-zarr-v2-compressed-with-chunking-and-rollover | ||
write-zarr-v2-raw-multiscale | ||
write-zarr-v2-raw-multiscale-with-trivial-tile-size | ||
write-zarr-v2-compressed-multiscale | ||
write-zarr-v2-to-s3 | ||
multiscales-metadata | ||
write-zarr-v3-raw | ||
write-zarr-v3-raw-with-ragged-sharding | ||
write-zarr-v3-raw-chunk-exceeds-array | ||
write-zarr-v3-compressed | ||
write-zarr-v3-raw-multiscale | ||
write-zarr-v3-to-s3 | ||
) | ||
|
||
foreach (name ${tests}) | ||
set(tgt "${project}-${name}") | ||
add_executable(${tgt} ${name}.cpp) | ||
target_compile_definitions(${tgt} PUBLIC "TEST=\"${tgt}\"") | ||
set_target_properties(${tgt} PROPERTIES | ||
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>" | ||
) | ||
target_include_directories(${tgt} PRIVATE "${CMAKE_CURRENT_LIST_DIR}/../") | ||
target_link_libraries(${tgt} | ||
acquire-core-logger | ||
acquire-core-platform | ||
acquire-video-runtime | ||
nlohmann_json::nlohmann_json | ||
miniocpp::miniocpp | ||
) | ||
|
||
add_test(NAME test-${tgt} COMMAND ${tgt}) | ||
set_tests_properties(test-${tgt} PROPERTIES LABELS "anyplatform;acquire-driver-zarr") | ||
endforeach () | ||
|
||
# | ||
# Copy driver to tests | ||
# | ||
list(POP_FRONT tests onename) | ||
|
||
foreach (driver | ||
acquire-driver-common | ||
acquire-driver-zarr | ||
) | ||
add_custom_target(${project}-copy-${driver}-for-tests | ||
COMMAND ${CMAKE_COMMAND} -E copy | ||
$<TARGET_FILE:${driver}> | ||
$<TARGET_FILE_DIR:${project}-${onename}> | ||
DEPENDS ${driver} | ||
COMMENT "Copying ${driver} to $<TARGET_FILE_DIR:${project}-${onename}>" | ||
) | ||
|
||
foreach (name ${tests}) | ||
add_dependencies(${tgt} ${project}-copy-${driver}-for-tests) | ||
endforeach () | ||
endforeach () | ||
endif () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can save this for the streaming library.