wgtmac commented on code in PR #631:
URL: https://github.com/apache/iceberg-cpp/pull/631#discussion_r3182841911
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -129,9 +134,13 @@ add_iceberg_test(util_test
uuid_test.cc
visit_type_test.cc)
-add_iceberg_test(roaring_test SOURCES roaring_test.cc)
+add_iceberg_test(roaring_test USE_DATA SOURCES roaring_test.cc)
Review Comment:
IMO, we can remove the executable `roaring_test` as well as the file
`roaring_test.cc`.
##########
src/iceberg/puffin/file_metadata.h:
##########
@@ -105,16 +105,16 @@ struct ICEBERG_EXPORT BlobMetadata {
friend bool operator==(const BlobMetadata& lhs, const BlobMetadata& rhs) =
default;
};
-ICEBERG_EXPORT std::string ToString(const BlobMetadata& blob_metadata);
+ICEBERG_DATA_EXPORT std::string ToString(const BlobMetadata& blob_metadata);
/// \brief Metadata about a Puffin file.
-struct ICEBERG_EXPORT FileMetadata {
+struct ICEBERG_DATA_EXPORT FileMetadata {
Review Comment:
I'm not quite sure if puffin file metadata (and other associated metadata
definitions) belong to the `iceberg_data` library. I'm fine to go with the
current PR and change if necessary in the future.
##########
src/iceberg/CMakeLists.txt:
##########
@@ -169,6 +157,56 @@ add_iceberg_lib(iceberg
OUTPUTS
ICEBERG_LIBRARIES)
+set(ICEBERG_DATA_SOURCES
+ data/data_writer.cc
+ data/delete_loader.cc
+ data/equality_delete_writer.cc
+ data/position_delete_writer.cc
+ data/writer.cc
+ deletes/position_delete_index.cc
+ deletes/roaring_position_bitmap.cc
+ puffin/file_metadata.cc
+ puffin/json_serde.cc
+ puffin/puffin_format.cc)
+
+set(ICEBERG_DATA_STATIC_BUILD_INTERFACE_LIBS)
+set(ICEBERG_DATA_SHARED_BUILD_INTERFACE_LIBS)
+set(ICEBERG_DATA_STATIC_INSTALL_INTERFACE_LIBS)
+set(ICEBERG_DATA_SHARED_INSTALL_INTERFACE_LIBS)
+
+list(APPEND ICEBERG_DATA_STATIC_BUILD_INTERFACE_LIBS
+ "$<IF:$<TARGET_EXISTS:iceberg_static>,iceberg_static,iceberg_shared>"
+ roaring::roaring)
+list(APPEND ICEBERG_DATA_SHARED_BUILD_INTERFACE_LIBS
+ "$<IF:$<TARGET_EXISTS:iceberg_shared>,iceberg_shared,iceberg_static>"
+ roaring::roaring)
+list(APPEND
+ ICEBERG_DATA_STATIC_INSTALL_INTERFACE_LIBS
+
"$<IF:$<TARGET_EXISTS:iceberg::iceberg_static>,iceberg::iceberg_static,iceberg::iceberg_shared>"
+ "$<IF:$<BOOL:${CROARING_VENDORED}>,iceberg::roaring,roaring::roaring>")
+list(APPEND
+ ICEBERG_DATA_SHARED_INSTALL_INTERFACE_LIBS
+
"$<IF:$<TARGET_EXISTS:iceberg::iceberg_shared>,iceberg::iceberg_shared,iceberg::iceberg_static>"
+ "$<IF:$<BOOL:${CROARING_VENDORED}>,iceberg::roaring,roaring::roaring>")
+
+add_iceberg_lib(iceberg_data
+ SOURCES
+ ${ICEBERG_DATA_SOURCES}
+ EXTRA_INCLUDES
+ ${ICEBERG_INCLUDES}
+ SHARED_LINK_LIBS
+ ${ICEBERG_DATA_SHARED_BUILD_INTERFACE_LIBS}
+ STATIC_LINK_LIBS
+ ${ICEBERG_DATA_STATIC_BUILD_INTERFACE_LIBS}
+ STATIC_INSTALL_INTERFACE_LIBS
+ ${ICEBERG_DATA_STATIC_INSTALL_INTERFACE_LIBS}
+ SHARED_INSTALL_INTERFACE_LIBS
+ ${ICEBERG_DATA_SHARED_INSTALL_INTERFACE_LIBS})
+
+if(TARGET iceberg_data_static)
+ target_compile_definitions(iceberg_data_static PRIVATE ICEBERG_STATIC)
+endif()
Review Comment:
```suggestion
```
`iceberg_data_static` has linked `iceberg_static` so all symbols from the
latter should be imported. Let's remove these lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
