mitjap commented on code in PR #3299:
URL: https://github.com/apache/avro/pull/3299#discussion_r1925610597
##########
lang/c++/CMakeLists.txt:
##########
@@ -69,17 +70,16 @@ endif()
if (CMAKE_COMPILER_IS_GNUCXX)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wduplicated-cond
-Wduplicated-branches -Wlogical-op -Wuseless-cast -Wconversion -pedantic
-Werror")
-if (AVRO_ADD_PROTECTOR_FLAGS)
- set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fstack-protector-all
-D_GLIBCXX_DEBUG")
- # Unset _GLIBCXX_DEBUG for avrogencpp.cc because using Boost Program
Options
- # leads to linking errors when compiling with _GLIBCXX_DEBUG as described
on
- # https://stackoverflow.com/questions/19729036/
- set_source_files_properties(impl/avrogencpp.cc PROPERTIES COMPILE_FLAGS
"-U_GLIBCXX_DEBUG")
-endif ()
endif ()
if (AVRO_BUILD_TESTS OR AVRO_USE_BOOST)
- find_package (Boost 1.38 REQUIRED COMPONENTS system)
+ # Boost 1.70 and above provide a BoostConfig.cmake package configuration
file.
+ find_package (Boost 1.70 REQUIRED CONFIG COMPONENTS system)
+ if (TARGET Boost::system)
+ message("Found Boost version: ${Boost_VERSION}")
+ else ()
+ message(FATAL_ERROR "Boost::system not found")
+ endif ()
Review Comment:
You mark this dependency as required. I think this check if target exists is
not needed. CMake will fail if it does not find Boost and all requested
components.
##########
lang/c++/CMakeLists.txt:
##########
@@ -202,15 +203,16 @@ if (AVRO_BUILD_EXECUTABLES)
gen (union_redundant_types redundant_types)
add_executable (avrogencpp impl/avrogencpp.cc)
- target_link_libraries (avrogencpp avrocpp_s ${Boost_LIBRARIES})
+ target_link_libraries (avrogencpp avrocpp_s)
+ target_compile_definitions(avrogencpp PRIVATE
AVRO_VERSION="${AVRO_VERSION}")
endif ()
if (AVRO_BUILD_TESTS)
enable_testing()
macro (unittest name)
add_executable (${name} test/${name}.cc)
- target_link_libraries (${name} avrocpp_s ${Boost_LIBRARIES}
${SNAPPY_LIBRARIES} ${ZLIB_LIBRARIES})
+ target_link_libraries (${name} avrocpp_s Boost::system ZLIB::ZLIB
$<IF:$<TARGET_EXISTS:Snappy::snappy>,Snappy::snappy,>)
Review Comment:
I think this should work. Might be more readable with simple `if` expression.
##########
lang/c++/CMakeLists.txt:
##########
@@ -135,26 +127,35 @@ set (AVRO_SOURCE_FILES
impl/CustomAttributes.cc
)
-add_library (avrocpp SHARED ${AVRO_SOURCE_FILES})
-
-set_property (TARGET avrocpp
- APPEND PROPERTY COMPILE_DEFINITIONS AVRO_DYN_LINK)
-
+add_library (avrocpp SHARED ${AVRO_SOURCE_FILES})
Review Comment:
```suggestion
add_library (avrocpp SHARED ${AVRO_SOURCE_FILES})
```
--
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]
