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.

Sign up for GitHub

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

Jump to bottom

Define the Zarr streaming API #291

Closed
wants to merge 18 commits into from
Closed

Define the Zarr streaming API #291

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
30560f3
remove miniocpp binaries
aliddell Aug 30, 2024
a612210
Define API
aliddell Aug 30, 2024
72cfe47
Merge remote-tracking branch 'upstream/main' into standalone-sequence
aliddell Aug 30, 2024
e5afc9d
More documentation
aliddell Aug 30, 2024
4ef1c86
Move driver tests to tests/driver. Add logger and stream settings code.
aliddell Aug 30, 2024
18ac6d4
Get it building.
aliddell Aug 30, 2024
4397b75
Swap order of ZarrDimensionType_Channel and ZarrDimensionType_Time.
aliddell Sep 3, 2024
ed8be14
Add external metadata handling.
aliddell Sep 3, 2024
6843827
Do JSON validation when setting external metadata.
aliddell Sep 3, 2024
9b397c6
Set -fPIC.
aliddell Sep 4, 2024
6a4aa86
Allow setting empty external metadata.
aliddell Sep 4, 2024
6cae047
Copy settings.
aliddell Sep 4, 2024
3fa201c
Merge remote-tracking branch 'upstream/main' into standalone-sequence
aliddell Sep 5, 2024
e5bce03
Rename internal/ directory to streaming/. Move driver code to driver/…
aliddell Sep 5, 2024
d808a25
Remove float16 datatype.
aliddell Sep 5, 2024
250c661
Remove unsupported compressors.
aliddell Sep 5, 2024
96b580a
Finish responding to PR comments.
aliddell Sep 16, 2024
180b8cb
Don't use strnlen_s.
aliddell Sep 16, 2024
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: 3 additions & 1 deletion .github/workflows/test_pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,6 @@ jobs:

- name: Test
working-directory: ${{github.workspace}}/build
run: ctest -C ${{env.BUILD_TYPE}} -L acquire-driver-zarr --output-on-failure
run: |
ctest -C ${{env.BUILD_TYPE}} -L acquire-driver-zarr --output-on-failure
ctest -C ${{env.BUILD_TYPE}} -L acquire-zarr --output-on-failure
59 changes: 52 additions & 7 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,54 @@
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

####### Acquire Zarr Streaming Library #######

set(tgt acquire-zarr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add some information about why "streaming" is now the primary target? Additionally, would it be more effective to organize the code by placing all "driver" code in one folder and "streaming" code in another? This approach would allow each subfolder to have its own CMake file defining its target, rather than having a single CMake file defining two targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point, the streaming code is going to go into its own repo, but this is probably a good opportunity to separate them.


add_library(${tgt} STATIC
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's best practice to let users decide whether to build a library as static or shared. While CMake defaults to STATIC, it offers the BUILD_SHARED_LIBS option for override. I'd omit specifying the library type unless necessary—in which case, I'll add a comment explaining why.

include/zarr.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's standard practice to place the include folder containing public headers outside the src directory. This separation clearly distinguishes the public interface from implementation details and simplifies integration for users.

internal/logger.hh
internal/logger.cpp
internal/stream.settings.hh
internal/stream.settings.cpp
internal/zarr.stream.hh
internal/zarr.stream.cpp
)

target_include_directories(${tgt}
PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/internal>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you choose to name a directory internal? If it solely contains streaming code, shouldn't its name reflect that content? This question relates to my earlier suggestion about creating separate directories for each target, each with its own CMake file.

)

target_link_libraries(${tgt} PRIVATE
blosc_static
miniocpp::miniocpp
)

set_target_properties(${tgt} PROPERTIES
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>"
)

install(TARGETS ${tgt}
LIBRARY DESTINATION lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

The presence of a LIBRARY DESTINATION configuration alongside an explicit request for a STATIC library suggests that there may not be a strict requirement for compiling a static library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain what you mean by that?

ARCHIVE DESTINATION lib
)

# Install public header files
install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/include/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I typically use include(GNUInstallDirs) to manage installation defaults. This provides variables like CMAKE_INSTALL_LIBDIR, which are better suited for cross-platform compatibility. Despite the name, I think it also provides the correct defaults for Windows and MacOS.

DESTINATION include
FILES_MATCHING PATTERN "*.h"
)

####### Acquire Zarr Driver #######

if (NOT TARGET acquire-core-logger)
add_subdirectory(../acquire-common/acquire-core-libs ${CMAKE_CURRENT_BINARY_DIR}/acquire-core-libs)
endif ()

set(tgt acquire-driver-zarr)
add_library(${tgt} MODULE
set(tgt-driver acquire-driver-zarr)
add_library(${tgt-driver} MODULE
common/dimension.hh
common/dimension.cpp
common/thread.pool.hh
Expand Down Expand Up @@ -36,12 +81,12 @@ add_library(${tgt} MODULE
zarr.driver.c
)

target_include_directories(${tgt} PRIVATE
target_include_directories(${tgt-driver} PRIVATE
$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/src>
)

target_enable_simd(${tgt})
target_link_libraries(${tgt} PRIVATE
target_enable_simd(${tgt-driver})
target_link_libraries(${tgt-driver} PRIVATE
acquire-core-logger
acquire-core-platform
acquire-device-kit
Expand All @@ -50,8 +95,8 @@ target_link_libraries(${tgt} PRIVATE
nlohmann_json::nlohmann_json
miniocpp::miniocpp
)
set_target_properties(${tgt} PROPERTIES
set_target_properties(${tgt-driver} PROPERTIES
MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>"
)

install(TARGETS ${tgt} LIBRARY DESTINATION lib)
install(TARGETS ${tgt-driver} LIBRARY DESTINATION lib)
Loading
Loading