feat: add cpr dependency for rest catalog client #236
Conversation
|
BTW, we need to update NOTICE and LICENSE files for these new dependencies. |
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Could you try apply this change locally and build the example?
diff --git a/example/CMakeLists.txt b/example/CMakeLists.txt
index 3e178a8..4fc05c7 100644
--- a/example/CMakeLists.txt
+++ b/example/CMakeLists.txt
@@ -26,4 +26,4 @@ find_package(Iceberg CONFIG REQUIRED)
add_executable(demo_example demo_example.cc)
-target_link_libraries(demo_example PRIVATE Iceberg::iceberg_bundle_static)
+target_link_libraries(demo_example PRIVATE Iceberg::iceberg_bundle_static Iceberg::iceberg_rest)
LICENSE
Outdated
|
|
||
| -------------------------------------------------------------------------------- | ||
|
|
||
| 3rdparty dependency cpr is statically linked in certain binary |
There was a problem hiding this comment.
@jbonofre Could you help review LICENSE and NOTICE files for adding cpr and curl? BTW, do we need to add OpenSSL which is an indirect dependency (required by curl)?
There was a problem hiding this comment.
@jbonofre Could you help review LICENSE and NOTICE files for adding
cprandcurl? BTW, do we need to addOpenSSLwhich is an indirect dependency (required by curl)?
I have split the task of modifying the license into a separate PR #243 . @jbonofre Could you help review it? In this case, the current PR will not involve license modifications and should be able to be merged quickly.
|
I will do the review later today. |
|
The task of modifying the license has been moved into a separate PR #243 . In this case, the current PR will not involve license modifications and should be able to be merged quickly. |
|
@Fokko I think this is ready to merge. It is the first step to add rest catalog client support. We will add LICENSE/NOTICE in a dedicated PR. |
|
The PR title is out of date. Perhaps |
HeartLinked
changed the title
|
Thanks @HeartLinked for working on this and thanks for the review 🙌 |
|
@wgtmac LICENSE and NOTICE can be done in a separate PR, but must not be forgotten. Do you know if there are any CPP specific tools for checking this? |
Reorganizes the catalog module structure to better support the upcoming REST catalog implementation. Changes: - Create a new `memory` subdirectory within `iceberg-cpp/src/iceberg/catalog/`. Move the existing `in_memory_catalog.cc` and `in_memory_catalog.h` files into this new memory subdirectory. - Add `json` dependencies for the rest package to support serialization and deserialization of data. - Add the `cpp-httplib` dependency to the rest test code. This library will be used to simply simulate an Iceberg REST server, enabling tests of whether the RestCatalog client can communicate with it correctly and handle success and failure scenarios. Hint: - This PR depends on the dependencies introduced in and should be merged after #236. closes #229 .
6 participants
Add cpr and curl dependency for REST catalog. This is related to #232 .
The current code in the REST package is just a demo example to verify the feasibility of cpr, which will be replaced in future commits. The actual content will be added in subsequent PRs so that we can keep the size of each PR reasonable.