From c630c27644d6a37b7aadd2487e64fe080c291280 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 15 Aug 2024 12:35:32 -0400 Subject: [PATCH 1/5] Make the combination of get and set idempotent. --- src/zarr.cpp | 20 ++-- tests/CMakeLists.txt | 1 + tests/get-set-get.cpp | 211 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 221 insertions(+), 11 deletions(-) create mode 100644 tests/get-set-get.cpp diff --git a/src/zarr.cpp b/src/zarr.cpp index a227f2be..2383d6b1 100644 --- a/src/zarr.cpp +++ b/src/zarr.cpp @@ -20,11 +20,13 @@ as_path(const StorageProperties& props) return {}; } - const size_t offset = - strlen(props.uri.str) > 7 && strcmp(props.uri.str, "file://") == 0 ? 7 - : 0; - return { props.uri.str + offset, - props.uri.str + offset + props.uri.nbytes - (offset + 1) }; + std::string uri{ props.uri.str, props.uri.nbytes - 1 }; + + if (uri.find("file://") == std::string::npos) { + return uri; + } + + return uri.substr(7); } /// \brief Check that the JSON string is valid. (Valid can mean empty.) @@ -68,19 +70,15 @@ validate_props(const StorageProperties* props) CHECK(tokens.size() > 2); // http://endpoint/bucket } else { const fs::path path = as_path(*props); - fs::path parent_path = path.parent_path().string(); + fs::path parent_path = path.parent_path(); if (parent_path.empty()) parent_path = "."; EXPECT(fs::is_directory(parent_path), "Expected \"%s\" to be a directory.", - parent_path.c_str()); + parent_path.string().c_str()); // check directory is writable - EXPECT(fs::is_directory(parent_path), - "Expected \"%s\" to be a directory.", - parent_path.c_str()); - const auto perms = fs::status(fs::path(parent_path)).permissions(); EXPECT((perms & (fs::perms::owner_write | fs::perms::group_write | diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b259dbce..cc4c2f4b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -19,6 +19,7 @@ else () unit-tests get get-meta + get-set-get external-metadata-with-whitespace-ok restart-stopped-zarr-resets-threadpool repeat-start diff --git a/tests/get-set-get.cpp b/tests/get-set-get.cpp new file mode 100644 index 00000000..0021af83 --- /dev/null +++ b/tests/get-set-get.cpp @@ -0,0 +1,211 @@ +/// @file get-set-get.cpp +/// @brief Test that successive calls to Zarr::get() return the same value. + +#include "platform.h" +#include "logger.h" +#include "device/kit/driver.h" +#include "device/hal/driver.h" +#include "device/hal/storage.h" +#include "device/props/storage.h" + +#include +#include +#include +#include + +#define containerof(P, T, F) ((T*)(((char*)(P)) - offsetof(T, F))) + +/// Helper for passing size static strings as function args. +/// For a function: `f(char*,size_t)` use `f(SIZED("hello"))`. +/// Expands to `f("hello",5)`. +#define SIZED(str) str, sizeof(str) + +#define L aq_logger +#define LOG(...) L(0, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define LOGE(...) L(1, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define EXPECT(e, ...) \ + do { \ + if (!(e)) { \ + LOGE(__VA_ARGS__); \ + goto Error; \ + } \ + } while (0) +#define CHECK(e) EXPECT(e, "Expression evaluated as false:\n\t%s", #e) + +/// Check that a==b +/// example: `ASSERT_EQ(int,"%d",42,meaning_of_life())` +#define ASSERT_EQ(T, fmt, a, b) \ + do { \ + T a_ = (T)(a); \ + T b_ = (T)(b); \ + EXPECT( \ + a_ == b_, "Expected %s==%s but " fmt "!=" fmt "\n", #a, #b, a_, b_); \ + } while (0) + +/// Check that strings a == b +/// example: `ASSERT_STREQ("foo",container_of_foo)` +#define ASSERT_STREQ(a, b) \ + do { \ + std::string a_ = (a); \ + std::string b_ = (b); \ + EXPECT(a_ == b_, \ + "Expected '%s'=='%s' but '%s'!= '%s'", \ + #a, \ + #b, \ + a_.c_str(), \ + b_.c_str()); \ + } while (0) + +namespace fs = std::filesystem; + +void +reporter(int is_error, + const char* file, + int line, + const char* function, + const char* msg) +{ + fprintf(is_error ? stderr : stdout, + "%s%s(%d) - %s: %s\n", + is_error ? "ERROR " : "", + file, + line, + function, + msg); +} + +typedef struct Driver* (*init_func_t)(void (*reporter)(int is_error, + const char* file, + int line, + const char* function, + const char* msg)); + +bool +validate(const struct StorageProperties& props) +{ + std::string abspath = fs::absolute(fs::path(TEST ".zarr")).string(); + std::string uri{ props.uri.str }; + ASSERT_STREQ(uri, "file://" + abspath); + CHECK(strcmp(props.external_metadata_json.str, R"({"foo":"bar"})") == 0); + + CHECK(props.acquisition_dimensions.size == 3); + CHECK(props.acquisition_dimensions.data != nullptr); + + CHECK(0 == strcmp(props.acquisition_dimensions.data[0].name.str, "x")); + CHECK(DimensionType_Space == props.acquisition_dimensions.data[0].kind); + CHECK(props.acquisition_dimensions.data[0].array_size_px == 64); + CHECK(props.acquisition_dimensions.data[0].chunk_size_px == 16); + CHECK(props.acquisition_dimensions.data[0].shard_size_chunks == 2); + + CHECK(0 == strcmp(props.acquisition_dimensions.data[1].name.str, "y")); + CHECK(DimensionType_Space == props.acquisition_dimensions.data[1].kind); + CHECK(props.acquisition_dimensions.data[1].array_size_px == 48); + CHECK(props.acquisition_dimensions.data[1].chunk_size_px == 16); + CHECK(props.acquisition_dimensions.data[1].shard_size_chunks == 3); + + CHECK(0 == strcmp(props.acquisition_dimensions.data[2].name.str, "z")); + CHECK(DimensionType_Space == props.acquisition_dimensions.data[2].kind); + CHECK(props.acquisition_dimensions.data[2].array_size_px == 0); + CHECK(props.acquisition_dimensions.data[2].chunk_size_px == 6); + CHECK(props.acquisition_dimensions.data[2].shard_size_chunks == 1); + + CHECK(props.first_frame_id == 0); // this is ignored + + CHECK(props.enable_multiscale); + + return true; + +Error: + return false; +} + +int +main() +{ + logger_set_reporter(reporter); + lib lib{}; + CHECK(lib_open_by_name(&lib, "acquire-driver-zarr")); + { + auto init = (init_func_t)lib_load(&lib, "acquire_driver_init_v0"); + auto driver = init(reporter); + CHECK(driver); + const auto n = driver->device_count(driver); + for (uint32_t i = 0; i < n; ++i) { + DeviceIdentifier id{}; + CHECK(driver->describe(driver, &id, i) == Device_Ok); + + std::string name{ id.name }; + + if (id.kind == DeviceKind_Storage && name.starts_with("Zarr")) { + struct Device* device = nullptr; + struct Storage* storage = nullptr; + + CHECK(Device_Ok == driver_open_device(driver, i, &device)); + storage = containerof(device, struct Storage, device); + + struct StorageProperties props = { 0 }; + + // unconfigured behavior + CHECK(storage_get(storage, &props) == Device_Ok); + + // set + CHECK(props.uri.str); + CHECK(strcmp(props.uri.str, "") == 0); + CHECK(props.uri.nbytes == 1); + + CHECK(props.external_metadata_json.str); + CHECK(strcmp(props.external_metadata_json.str, "") == 0); + CHECK(props.external_metadata_json.nbytes == 1); + + CHECK(props.first_frame_id == 0); + + CHECK(props.pixel_scale_um.x == 1); + CHECK(props.pixel_scale_um.y == 1); + + CHECK(props.acquisition_dimensions.size == 0); + CHECK(props.acquisition_dimensions.data == nullptr); + + CHECK(props.enable_multiscale == 0); + + CHECK(storage_properties_init( + &props, + 13, + SIZED(TEST ".zarr"), + SIZED(R"({"foo":"bar"})"), + { 1, 1 }, + 3 // we need at least 3 dimensions to validate settings + )); + + CHECK(storage_properties_set_dimension( + &props, 0, SIZED("x") + 1, DimensionType_Space, 64, 16, 2)); + CHECK(storage_properties_set_dimension( + &props, 1, SIZED("y") + 1, DimensionType_Space, 48, 16, 3)); + CHECK(storage_properties_set_dimension( + &props, 2, SIZED("z") + 1, DimensionType_Space, 0, 6, 1)); + + props.enable_multiscale = true; + + // configure the storage device + CHECK(Device_Ok == storage_set(storage, &props)); + CHECK(Device_Ok == storage_get(storage, &props)); + + CHECK(validate(props)); + + // set and validate props again + CHECK(Device_Ok == storage_set(storage, &props)); + CHECK(Device_Ok == storage_get(storage, &props)); + + CHECK(validate(props)); + + storage_properties_destroy(&props); + + CHECK(Device_Ok == driver_close_device(device)); + } + } + } + lib_close(&lib); + return 0; +Error: + lib_close(&lib); + return 1; +} \ No newline at end of file From 44f2f080e332b8607090c24f864f11ad337bb0d0 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 15 Aug 2024 13:13:56 -0400 Subject: [PATCH 2/5] Update file doc for get-set-get.cpp. --- tests/get-set-get.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/get-set-get.cpp b/tests/get-set-get.cpp index 0021af83..48ad400e 100644 --- a/tests/get-set-get.cpp +++ b/tests/get-set-get.cpp @@ -1,5 +1,6 @@ /// @file get-set-get.cpp -/// @brief Test that successive calls to Zarr::get() return the same value. +/// @brief Test that getting and resetting storage properties will not render +/// any properties invalid. #include "platform.h" #include "logger.h" From 475fbd20b44decd3a87f6525278e01b664c649e2 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 15 Aug 2024 13:22:41 -0400 Subject: [PATCH 3/5] Don't prepend web URIs with "file://"! --- src/zarr.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/zarr.cpp b/src/zarr.cpp index 2383d6b1..6d17b455 100644 --- a/src/zarr.cpp +++ b/src/zarr.cpp @@ -392,10 +392,13 @@ zarr::Zarr::get(StorageProperties* props) const storage_properties_destroy(props); std::string uri; - if (!dataset_root_.empty()) { + if (common::is_web_uri(dataset_root_)) { + uri = dataset_root_; + } else if (!dataset_root_.empty()) { fs::path dataset_root_abs = fs::absolute(dataset_root_); uri = "file://" + dataset_root_abs.string(); } + const size_t bytes_of_filename = uri.empty() ? 0 : uri.size() + 1; const char* metadata = external_metadata_json_.empty() From 68240ab9214ea2dac9a2b9e9f622ce4d228ef58f Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 20 Aug 2024 17:05:32 -0700 Subject: [PATCH 4/5] wip: log the path --- src/zarr.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zarr.cpp b/src/zarr.cpp index 6d17b455..b69e7911 100644 --- a/src/zarr.cpp +++ b/src/zarr.cpp @@ -70,6 +70,7 @@ validate_props(const StorageProperties* props) CHECK(tokens.size() > 2); // http://endpoint/bucket } else { const fs::path path = as_path(*props); + LOG("path: '%s'", path.string().c_str()); fs::path parent_path = path.parent_path(); if (parent_path.empty()) parent_path = "."; From df03cb21069495d3ef849a7fe27b79715e0427f4 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 21 Aug 2024 10:16:04 -0700 Subject: [PATCH 5/5] Remove log statement. Add comment about uri.substr --- src/zarr.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/zarr.cpp b/src/zarr.cpp index b69e7911..be85d613 100644 --- a/src/zarr.cpp +++ b/src/zarr.cpp @@ -26,7 +26,7 @@ as_path(const StorageProperties& props) return uri; } - return uri.substr(7); + return uri.substr(7); // strlen("file://") == 7 } /// \brief Check that the JSON string is valid. (Valid can mean empty.) @@ -70,7 +70,6 @@ validate_props(const StorageProperties* props) CHECK(tokens.size() > 2); // http://endpoint/bucket } else { const fs::path path = as_path(*props); - LOG("path: '%s'", path.string().c_str()); fs::path parent_path = path.parent_path(); if (parent_path.empty()) parent_path = ".";