szaszm commented on code in PR #1885: URL: https://github.com/apache/nifi-minifi-cpp/pull/1885#discussion_r1815518584
########## cmake/Fetchlibrdkafka.cmake: ########## @@ -0,0 +1,54 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +include(FetchContent) +include(Zstd) +list(PREPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/zstd/dummy") + +include(LZ4) +list(PREPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/lz4/dummy") + +set(WITH_SSL "ON" CACHE STRING "" FORCE) +set(WITH_SASL "OFF" CACHE STRING "" FORCE) +set(WITH_ZSTD "ON" CACHE STRING "" FORCE) +set(WITH_SNAPPY "ON" CACHE STRING "" FORCE) +set(ENABLE_LZ4_EXT "ON" CACHE STRING "" FORCE) +set(RDKAFKA_BUILD_STATIC "ON" CACHE STRING "" FORCE) +set(RDKAFKA_BUILD_EXAMPLES "OFF" CACHE STRING "" FORCE) +set(RDKAFKA_BUILD_TESTS "OFF" CACHE STRING "" FORCE) +set(LIBRDKAFKA_STATICLIB "1" CACHE STRING "" FORCE) + +set(PATCH_FILE "${CMAKE_SOURCE_DIR}/thirdparty/librdkafka/0001-remove-findLZ4-and-findZSTD.patch") +set(PC "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}") + +FetchContent_Declare(libkafka + URL https://github.com/confluentinc/librdkafka/archive/refs/tags/v2.6.0.tar.gz + URL_HASH SHA256=abe0212ecd3e7ed3c4818a4f2baf7bf916e845e902bb15ae48834ca2d36ac745 + PATCH_COMMAND "${PC}" +) + +FetchContent_MakeAvailable(libkafka) + +add_dependencies(rdkafka OpenSSL::SSL OpenSSL::Crypto ZLIB::ZLIB Threads::Threads lz4::lz4 zstd::zstd) Review Comment: are all of these actually necessary? This is not about linking, but blocking the rdkafka build before all of the dependencies have finished building, preventing parallel build. ########## cmake/Fetchlibrdkafka.cmake: ########## @@ -0,0 +1,54 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +include(FetchContent) +include(Zstd) +list(PREPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/zstd/dummy") Review Comment: we could move this line to the Zstd.cmake file, and same with LZ4 ########## cmake/LZ4.cmake: ########## @@ -26,11 +26,29 @@ FetchContent_Declare(lz4 URL https://github.com/lz4/lz4/archive/refs/tags/v1.9.4.tar.gz URL_HASH SHA256=0b0e3aa07c8c063ddf40b082bdf7e37a1562bda40a0ff5272957f3e987e0e54b SOURCE_SUBDIR build/cmake + OVERRIDE_FIND_PACKAGE ) FetchContent_MakeAvailable(lz4) -add_library(lz4::lz4 ALIAS lz4_static) +if(NOT TARGET lz4::lz4) Review Comment: Why do we need to check this, who else adds an `lz4::lz4` target? ########## extensions/kafka/ConsumeKafka.h: ########## @@ -219,9 +217,13 @@ class ConsumeKafka : public KafkaProcessorBase { ADD_COMMON_VIRTUAL_FUNCTIONS_FOR_PROCESSORS - explicit ConsumeKafka(std::string_view name, const utils::Identifier& uuid = utils::Identifier()) : + explicit ConsumeKafka(const std::string_view name, const utils::Identifier& uuid = utils::Identifier()) : Review Comment: I disagree, when the argument in question is not moved-from: Constructing a const object from an rvalue still moves, and for all purposes, top-level-const function (including constructor) arguments behave just like non-const for the callers, the only difference is the mutability of the argument variable inside the function body. In general, I'm pro const wherever it's feasible, so most places except: 1. things that are inherently variable, like a loop variable 2. objects that are moved-from 3. objects that are returned from a function (due to NRVO concerns) 4. non-static data members, because they prevent class movability ########## extensions/kafka/KafkaConnection.h: ########## @@ -80,9 +82,9 @@ class KafkaConnection { KafkaConnectionKey key_; - std::map<std::string, std::shared_ptr<KafkaTopic>> topics_; + std::map<std::string, std::shared_ptr<KafkaTopic>> topics_{}; - gsl::owner<rd_kafka_t*> kafka_connection_; + gsl::owner<rd_kafka_t*> kafka_connection_{}; Review Comment: Any reason for not converting this to unique_ptr just like all of the other places? ########## cmake/LZ4.cmake: ########## @@ -26,11 +26,29 @@ FetchContent_Declare(lz4 URL https://github.com/lz4/lz4/archive/refs/tags/v1.9.4.tar.gz URL_HASH SHA256=0b0e3aa07c8c063ddf40b082bdf7e37a1562bda40a0ff5272957f3e987e0e54b SOURCE_SUBDIR build/cmake + OVERRIDE_FIND_PACKAGE ) FetchContent_MakeAvailable(lz4) -add_library(lz4::lz4 ALIAS lz4_static) +if(NOT TARGET lz4::lz4) + add_library(lz4::lz4 ALIAS lz4_static) + add_library(LZ4::LZ4 ALIAS lz4_static) + + install(TARGETS lz4_static + EXPORT Lz4Targets # Add to Lz4Targets export set + ARCHIVE DESTINATION lib + LIBRARY DESTINATION lib + RUNTIME DESTINATION bin + ) + + # Install the export set itself + install(EXPORT Lz4Targets + FILE Lz4Targets.cmake # Generate Lz4Targets.cmake file + NAMESPACE lz4:: # Set the namespace to lz4:: + DESTINATION lib/cmake/Lz4 # Where to install the CMake config + ) Review Comment: I'm not sure about the `install` calls, they usually install files in the release tarball, and through that the user's machine. Are you sure these are not doing that? -- 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]
