added Avro support for KafkaAdapter #645
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| find_path(Avro_INCLUDE_DIR NAMES avro/Encoder.hh) | ||
| find_library(Avro_LIBRARY NAMES avrocpp libavrocpp) | ||
|
|
||
| # ============================================================================= | ||
| # Workaround for conda-forge avro-cpp fmt::formatter incompatibility on Windows | ||
| # ============================================================================= | ||
| # conda-forge's avro-cpp has fmt::formatter specializations with non-const | ||
| # format() methods, but fmt v12+ requires const. This causes MSVC error C2766. | ||
| # | ||
| # Solution: On Windows, we check if the avro headers have this bug, and if so, | ||
|
Collaborator
There was a problem hiding this comment. Can you not just check for the bug by looking at what version of avro-cpp you are building against? I am also not a fan of this patch, if the Windows version is too old to be compatible with the current env then I'd rather just skip it.
Collaborator
There was a problem hiding this comment. I still see us checking the code directly and not the
Collaborator
Author
There was a problem hiding this comment. Done. Earlier I wasn't 100% sure if we could reliably extract the version, but turns out we can. Also updated the vcpkg submodule to 1.12.1 which apparently has the fmt fix upstream, so we don't need the patch anymore.
Collaborator
There was a problem hiding this comment. Cool, so we can also get rid of the version logic in the FindAvro.cmake too then right. |
||
| # we create patched versions in the build directory and prepend them to the | ||
| # include path so they shadow the broken originals. | ||
| # | ||
| # This workaround can be removed once conda-forge updates avro-cpp. | ||
| # ============================================================================= | ||
|
|
||
| set(CSP_AVRO_PATCHED_INCLUDE_DIR "") | ||
|
|
||
| if(WIN32 AND Avro_INCLUDE_DIR AND NOT CSP_USE_VCPKG) | ||
| # Check if avro/Node.hh has the non-const format() bug | ||
| # The buggy pattern is: "auto format(...) {" without "const" before the brace | ||
| set(_avro_node_hh "${Avro_INCLUDE_DIR}/avro/Node.hh") | ||
| set(_avro_types_hh "${Avro_INCLUDE_DIR}/avro/Types.hh") | ||
| set(_needs_patching FALSE) | ||
|
|
||
| if(EXISTS "${_avro_node_hh}") | ||
| file(READ "${_avro_node_hh}" _node_hh_content) | ||
|
|
||
| # Check if the file contains fmt::formatter and non-const format() | ||
| # We look for "auto format" followed by ")" then whitespace then "{" | ||
| # without "const" in between | ||
| string(FIND "${_node_hh_content}" "fmt::formatter<avro::Name>" _has_formatter) | ||
| if(NOT _has_formatter EQUAL -1) | ||
| # Check specifically for the non-const pattern | ||
| # Buggy: auto format(const avro::Name &n, FormatContext &ctx) { | ||
| # Fixed: auto format(const avro::Name &n, FormatContext &ctx) const { | ||
| string(REGEX MATCH "auto format\\(const avro::Name[^)]+\\)[^c]*\\{" _buggy_pattern "${_node_hh_content}") | ||
| if(_buggy_pattern) | ||
| set(_needs_patching TRUE) | ||
| endif() | ||
| endif() | ||
| endif() | ||
|
|
||
| if(_needs_patching) | ||
| message(STATUS "Detected avro-cpp with non-const fmt::formatter bug - applying build-time patch") | ||
|
|
||
| # Create patched headers directory structure | ||
| set(CSP_AVRO_PATCHED_INCLUDE_DIR "${CMAKE_BINARY_DIR}/_patched_avro_headers") | ||
| set(_patched_avro_dir "${CSP_AVRO_PATCHED_INCLUDE_DIR}/avro") | ||
| file(MAKE_DIRECTORY "${_patched_avro_dir}") | ||
|
|
||
| # Patch Node.hh | ||
| # Replace: auto format(const avro::Name &n, FormatContext &ctx) { | ||
| # With: auto format(const avro::Name &n, FormatContext &ctx) const { | ||
| string(REGEX REPLACE | ||
| "(auto format\\(const avro::Name[^)]+\\))[ \t\r\n]*(\\{)" | ||
| "\\1 const \\2" | ||
| _patched_node_content | ||
| "${_node_hh_content}" | ||
| ) | ||
| file(WRITE "${_patched_avro_dir}/Node.hh" "${_patched_node_content}") | ||
| message(STATUS " Patched: avro/Node.hh -> ${_patched_avro_dir}/Node.hh") | ||
|
|
||
| # Patch Types.hh if it exists and has the same bug | ||
| if(EXISTS "${_avro_types_hh}") | ||
| file(READ "${_avro_types_hh}" _types_hh_content) | ||
| string(FIND "${_types_hh_content}" "fmt::formatter<avro::Type>" _has_type_formatter) | ||
| if(NOT _has_type_formatter EQUAL -1) | ||
| string(REGEX MATCH "auto format\\(avro::Type[^)]+\\)[^c]*\\{" _types_buggy "${_types_hh_content}") | ||
| if(_types_buggy) | ||
| string(REGEX REPLACE | ||
| "(auto format\\(avro::Type[^)]+\\))[ \t\r\n]*(\\{)" | ||
| "\\1 const \\2" | ||
| _patched_types_content | ||
| "${_types_hh_content}" | ||
| ) | ||
| file(WRITE "${_patched_avro_dir}/Types.hh" "${_patched_types_content}") | ||
| message(STATUS " Patched: avro/Types.hh -> ${_patched_avro_dir}/Types.hh") | ||
| endif() | ||
| endif() | ||
| endif() | ||
| endif() | ||
| endif() | ||
|
|
||
| if (NOT TARGET Avro::avrocpp) | ||
| add_library(Avro::avrocpp SHARED IMPORTED) | ||
|
|
||
| # On Windows, IMPORTED_IMPLIB is the .lib file, IMPORTED_LOCATION is the .dll | ||
| # On other platforms, IMPORTED_LOCATION is the shared library | ||
| if(WIN32) | ||
| set_property(TARGET Avro::avrocpp PROPERTY IMPORTED_IMPLIB "${Avro_LIBRARY}") | ||
| else() | ||
| set_property(TARGET Avro::avrocpp PROPERTY IMPORTED_LOCATION "${Avro_LIBRARY}") | ||
| endif() | ||
|
|
||
| # If we have patched headers, prepend them to include path so they shadow originals | ||
| if(CSP_AVRO_PATCHED_INCLUDE_DIR) | ||
| target_include_directories(Avro::avrocpp INTERFACE | ||
| ${CSP_AVRO_PATCHED_INCLUDE_DIR} # Patched headers first (shadows originals) | ||
| ${Avro_INCLUDE_DIR} # Original headers for everything else | ||
| ) | ||
| else() | ||
| target_include_directories(Avro::avrocpp INTERFACE ${Avro_INCLUDE_DIR}) | ||
| endif() | ||
| endif() | ||
|
|
||
| include(FindPackageHandleStandardArgs) | ||
| find_package_handle_standard_args(Avro DEFAULT_MSG Avro_LIBRARY Avro_INCLUDE_DIR) | ||
| mark_as_advanced(Avro_INCLUDE_DIR Avro_LIBRARY Avro::avrocpp) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,13 @@ find_package(DepsKafkaAdapter REQUIRED) | |
|
|
||
| target_link_libraries(csp_kafka_adapter PUBLIC csp_adapter_utils RdKafka::rdkafka RdKafka::rdkafka++) | ||
|
|
||
| # Link Avro library | ||
| if(CSP_USE_VCPKG) | ||
| target_link_libraries(csp_kafka_adapter PUBLIC unofficial::avro-cpp::avrocpp) | ||
|
Collaborator
There was a problem hiding this comment. The overall state of avro packaging (both in conda and vcpkg) really gives me hesitation on adding it as a build-time dependency of csp. I'd much prefer if we could split this out into it's own project, but not sure how feasible that is given our adapter ABI is still a WIP.
Collaborator
Author
There was a problem hiding this comment. The Kafka adapter with Avro support is already fully optional (CSP_BUILD_KAFKA_ADAPTER=ON/OFF). With this PR, on Windows conda-forge builds where avro-cpp is incompatible, it automatically disables itself with a clear warning - no build failures, no patches, just graceful degradation. I agree the avro packaging situation is frustrating. Once the adapter ABI stabilizes, splitting this into a separate project would make sense. For now, keeping it optional within CSP seems like the pragmatic path - users who need Kafka/Avro can enable it, others aren't affected.
Collaborator
There was a problem hiding this comment. Yes, but I assume we are going to build CSP (for distribution) with Avro enabled, just like we build with Kafka enabled currently. So it's not optional from the perspective of the userm who installs pre-built csp from conda/pypi
Collaborator
Author
There was a problem hiding this comment. should we wait then for ABI to stabilize before merging it?
Collaborator
There was a problem hiding this comment. We can merge it as an experimental feature and if avro becomes a nuisance in our build pipeline we can always re-evaluate/remove it. @timkpaine what do you think?
Member
There was a problem hiding this comment. we will eventually support separate adapters but for now i think its better to support everything we support on every platform, and not do platform-specific stuff. There should be no incompatible platform as this has caused problems in the past and makes life annoying for users. |
||
| else() | ||
| target_link_libraries(csp_kafka_adapter PUBLIC Avro::avrocpp) | ||
| endif() | ||
|
|
||
| install(TARGETS csp_kafka_adapter | ||
| PUBLIC_HEADER DESTINATION include/csp/adapters/kafka | ||
| RUNTIME DESTINATION ${CSP_RUNTIME_INSTALL_SUBDIR} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #ifndef _IN_CSP_ADAPTERS_UTILS_AVROINCLUDES_H | ||
| #define _IN_CSP_ADAPTERS_UTILS_AVROINCLUDES_H | ||
|
|
||
| // Centralized avro includes. | ||
| // On Windows conda-forge builds, headers are patched at CMake configure time | ||
| // (see FindAvro.cmake) to fix fmt v12 compatibility. | ||
|
|
||
| #include <avro/Compiler.hh> | ||
| #include <avro/Decoder.hh> | ||
| #include <avro/Encoder.hh> | ||
| #include <avro/Generic.hh> | ||
| #include <avro/GenericDatum.hh> | ||
| #include <avro/Node.hh> | ||
| #include <avro/Schema.hh> | ||
| #include <avro/Stream.hh> | ||
| #include <avro/ValidSchema.hh> | ||
|
|
||
| #endif |
Uh oh!
There was an error while loading. Please reload this page.