szaszm commented on PR #1775: URL: https://github.com/apache/nifi-minifi-cpp/pull/1775#issuecomment-2112726707
@james94 I'm busy with other commitments to keep up with this, but I'd like to prevent wasting too much of your time on something we can't use. We can't review a 17k diff, so it will never be merged as is stands now. If you want to pursue this, please create a separate pull request, where you only convert a single dependency to build with conan, so we can provide feedback there, and only change what's absolutely necessary. A good first target could be OpenSSL, because it takes forever to build on Windows. Please make an effort to keep the diff as small as possible, in the 100s if possible, but keep it under 1000. We can review many small pull requests, but not giants like this. Example: #603 didn't get any reviews, despite being a very valuable feature As the conversion of the first dependency is reviewed, we both learn from it, and subsequent dependency source conversions will be easier. Please only pursue 1 or max 2 features at a time, so we can keep up and provide the necessary feedback. This will inevitably take a few months. I'll also give some early feedback on things I noticed so far: - Please use a proper multi-choice option for selecting the source of a dependency instead of USE_CONAN_PACKAGER and USE_CMAKE_FETCH_CONTENT binary options. For example: MINIFI_OPENSSL_SOURCE: [conan|find_package|bundled]. The second option could keep just find_package, and rely on sources from the system. If it doesn't work, just having [conan|bundled] is fine. - Please don't include patches that are not strictly necessary to make conan builds work - Please prefix minifi-related options with MINIFI_ in cmake. Not all options follow this now, but future new CMake options should. - Please don't add new dependencies in the context of converting to Conan. For example, minifi doesn't depend on ffmpeg or libtiff at the moment. If they're needed for a new feature, the new feature needs to be a separate pull request. -- 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]
