szaszm commented on code in PR #1793: URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1627578730
########## cmake/MiNiFiOptions.cmake: ########## @@ -42,6 +42,7 @@ 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(MINIFI_USE_CONAN_PACKAGER "Instructs cmake to use conan packager to install external libs & build MiNiFi C++" OFF) Review Comment: Instead of a single option, I think setting the package source per dependency would be better. I'm thinking about new CMake options like `MINIFI_LIBCURL_SOURCE` with possible values like `system` (find_package), `conan` or `build` (ExternalProject, FetchContent, etc). If you prefer not adding `system` now, that's fine. You can see an example option with a predetermined set of possible values on line 76-78. ########## conanfile.py: ########## Review Comment: I think we should leave all feature selection options on the defaults, and use the normal FetchContent/ExternalProject build workflows for the rest of the dependencies. This would keep conan builds useful during the transition period, by offering a larger feature set, and it would allow us to track the reduction in compile-times as more dependencies are converted. The above suggested package source options could be an exception, and they can be set here to Conan. I think it's a safe bet that if someone is calling CMake with Conan, they probably want the dependencies acquired through Conan. What do you think? -- 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]
