martinzink commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1608241627
##########
cmake/MiNiFiOptions.cmake:
##########
@@ -42,6 +42,8 @@ add_minifi_option(DOCKER_SKIP_TESTS "Skip building tests in
docker image targets
add_minifi_option(DOCKER_PUSH "Push created images to the specified tags" OFF)
add_minifi_option(PORTABLE "Instructs the compiler to remove architecture
specific optimizations" ON)
+add_minifi_option(USE_CONAN_PACKAGER "Instructs cmake to use conan packager to
install external libs & build MiNiFi C++" OFF)
Review Comment:
Since there is only two exclusive options as of now, it doesnt make sense to
create two separate options here.
I suggest (for now) to only have a single MINIFI_USE_CONAN_PACKAGER option,
and replace the if elseif with a simple if else in the cmake files
And please prefix the new option with MINIFI_ (it makes easier to
differentiate our options with thirdparty ones)
##########
cmake/BundledZLIB.cmake:
##########
@@ -18,56 +18,65 @@
function(use_bundled_zlib SOURCE_DIR BINARY_DIR)
Review Comment:
When using with conan the naming doesnt really makes sense anymore
I suggest that you create new cmake file e.g.
GetZLIB.cmake or something similar
``` CMake
function(get_zlib SOURCE_DIR BINARY_DIR)
if(MINIFI_USE_CONAN_PACKAGER)
# your new conan code (which could also go into a new cmake file or be
inlined if trivial)
else()
use_bundled_zlib(${SOURCE_DIR} ${BINARY_DIR})
endif()
endfunction(get_zlib)
```
And replace the current use_bundled_zlib calls with this new get_zlib
--
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]
