apache
MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi
Verified I can Install & Build MiNiFi w OpenSSL, CURL, ZLIB using conan, then I set the MINIFI_HOME environment variable & ran minifi binary executable.
Will add the steps to reproduce (later we can integrate conan into the python bootstrap, the goal was to introduce you guys to the conan build approach with minimal changes): @lordgamez @szaszm
# make sure to install conan2 for your environment
sudo pip install --force-reinstall -v "conan==2.3.1"
# create a "default" conan profile, so conan has it on record, before using your own custom profile. Gets created in ~/.conan2/
conan profile detect
# conanfile.py is in root dir of MiNiFi C++ project
cd $HOME/nifi-minifi-cpp
# create MINIFI_HOME env variable for binary executable minifi
export MINIFI_HOME=$(pwd)
# In case you need to delete all conan packages
# conan remove "*" -c
# install conan packages for MiNiFi C++ using conanfile.py invoking Conan
# since we created default profile earlier, we can override it with our own minifi profile
# make sure path is correct
conan install . --build=missing --output-folder=build_conan -pr=$HOME/nifi-minifi-cpp/etc/conan/profiles/release-linux
# build MiNiFi C++ using conanfile.py invoking Conan & CMake
conan build . --output-folder=build_conan -pr=$HOME/nifi-minifi-cpp/etc/conan/profiles/release-linux
# verify we can run minifi binary executable
./build_conan/bin/minifi
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you to ensure the following steps have been taken:
For all changes:
-
[ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
-
[ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
-
[ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
-
[ ] Is your initial contribution a single, squashed commit?
For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
The macOS CI failure will be fixed in #1790, and the Docker integration test failure in #1795. The clang job fails because of a linter issue in the conanfile.
@szaszm If I get access to a macOS, I can looking into fixing the linter issue in the conanfile. Currently, I am testing MiNiFi from Ubuntu 22.04. I can later test it in Windows too if needed. For now, I addressed @martinzink suggested updates for integrating conan into MiNiFi build infrastructure.
@james94 You don't need a macOS to fix the python linter issue, it's not OS-specific. Just run this from a build directory:
cmake --build . --target flake8
And make sure flake8 is installed.
This is the output:
[1/2] cd /home/szaszm/nifi-minifi-cpp-3/build-clang-ninja && /home/szaszm/nifi-minifi-cpp-3/run_flake8.sh /home/szaszm/nifi-minifi-cpp-3
FAILED: CMakeFiles/flake8 /home/szaszm/nifi-minifi-cpp-3/build-clang-ninja/CMakeFiles/flake8
cd /home/szaszm/nifi-minifi-cpp-3/build-clang-ninja && /home/szaszm/nifi-minifi-cpp-3/run_flake8.sh /home/szaszm/nifi-minifi-cpp-3
/home/szaszm/nifi-minifi-cpp-3/conanfile.py:2:1: F401 'conan.tools.env.VirtualRunEnv' imported but unused
/home/szaszm/nifi-minifi-cpp-3/conanfile.py:9:1: E302 expected 2 blank lines, found 1
/home/szaszm/nifi-minifi-cpp-3/conanfile.py:18:53: E231 missing whitespace after ','
ninja: build stopped: subcommand failed.
I'm having trouble with conan, due to a missing profile. It seems like installation alone is not enough.
@szaszm yeah I was hoping that the conan installation would have automatically created a default conan profile. First lets create a default profile for conan first (then later we'll override it with our minifi conan profile):
# create a "default" conan profile, so conan has it on record, before using your own custom profile. Gets created in ~/.conan2/
conan profile detect
Once you have the default conan profile, then you can try overriding it again with our minifi conan profile:
# install conan packages for MiNiFi C++ using conanfile.py invoking Conan
# since we created default profile earlier, we can override it with our own minifi profile
# make sure path is correct
conan install . --build=missing --output-folder=build_conan -pr=$HOME/nifi-minifi-cpp/etc/build/conan/profiles/release-linux
NOTE: The nice thing about docker environment is we would have already created the default conan profile at the beginning and then later used our specific minifi conan profile.
Does the extra conan profile detect command help?
@james94 what docker environment do you mean? I was trying on arch linux, no containers.
@szaszm I meant we could leverage a docker environment like the one that is mentioned in the README and extend it with conan2 support: https://github.com/apache/nifi-minifi-cpp?tab=readme-ov-file#building-a-docker-image
I usually just launch my own docker container with needed build dependencies for MiNiFi C++, so I can build it within my docker container.
You could also try it directly on linux too.
@szaszm @martinzink @lordgamez just checking in, anything I can do to help speed up testing toward this PR?
Yes I agree with you @martinzink I do plan to add being able to configure minifi options mapping to conan options through the bootstrap py script, but that will be in a later PR, so we keep this initial PR light as @szaszm suggested.
I ran the following commands:
conan profile detect
export MINIFI_HOME=$(pwd) # I was in the source tree root
conan install . --build=missing --output-folder=build_conan
conan build . --output-folder=build_conan
conan install ran suspiciously quickly, I don't think it had to build anything from source, but it did install zlib/1.3.1, openssl/3.2.1 and libcurl/8.6.0.
Unfortunately OpenSSL and Zlib were still built from source using ExternalProject during the CMake build. I think when calling CMake through Conan, they should be using the Conan packages.
@szaszm @martinzink with respect to commit "49de56c": I verified I can build MiNiFi C++ using conan with the specific conan profile etc/conan/profiles/release-linux and with the sources for MINIFI_LIBCURL_SOURCE, MINIFI_OPENSSL_SOURCE
and MINIFI_ZLIB_SOURCE all set to CONAN successfully. There were 2 ENABLED options that I needed to set to OFF, which were ENABLE_LIBARCHIVE and ENABLE_AWS since the build would fail, but later I can get those working again since on my bigger PR-1775 "https://github.com/apache/nifi-minifi-cpp/pull/1775", I was able to keep those options ENABLED when building MiNiFi C++ with conan. I used the following command to build MiNiFi C++ with conan:
conan build . --build=missing --output-folder=build_conan -pr=etc/conan/profiles/release-linux
I did double check I can still build MiNiFi C++ using standalone CMake approach with all the default ENABLED options set to their original values as specified in MiNiFiOptions.cmake too. I used these commands to build MiNiFi C++ with cmake:
mkdir build_cmake
cd build_cmake
cmake ..
make -j $(nproc)
Please try the conan build MiNiFi C++ approach again when you get a chance
@james94 I checked and it seems to build successfully for me. Could you please rebase the PR to the latest main brach to be able to run the CI on it?
@lordgamez thanks for verifying you can build MiNiFi C++ with conan on your side. I appreciate it. I will rebase on latest MiNiFi main branch.
@lordgamez @szaszm I merged latest upstream main branch into my forked main branch and then merged my forked main branch into my MINIFICPP-2346-P1 branch.
- Also I chose not to rebase and instead went for the merge approach since rebasing in the passed caused me to lose my updates on previous branches.
After merging upstream main, I verified I can build MiNiFi using conan and then standalone CMake.
Hi @james94, thanks for the update! Unfortunately the lua.org page where we retrieved the lua sources went down, that's the reason the CI jobs are failing. We merged a commit to move from lua.org to the github page to retrieve the source files with the following commit: https://github.com/apache/nifi-minifi-cpp/commit/b0b2cf63d0cebf32b71608d9337520b8c1f939cf Could you rebase it again to be able to run the CI correctly?
@james94 I could rebase your branch easily without data loss. The existing merge complicates things a bit, but not too much. Here's what I did:
git checkout main
git pull upstream main # update local main branch from apache/nifi-minifi-cpp
git checkout MINIFICPP-2346-P1
git reset --hard HEAD~1 # remove merge commit from the top
git rebase -i main # interactive rebase
# then edited the file, removing Gabor's commits, which were the first 6 IIRC, like here on the "commits" tab. "Making OpenSSL and curl mandatory", + 5x "review update" commits. At the end, exited the editor and git did the rebase.
# at the end, there is a successful rebase to the latest main, with no conflicts, and only your commits.
Thanks for sharing the conan profile at etc/conan/profiles/release-linux. I have concerns about the hardcoded compiler version and "system package manager" settings, but I don't know if this causes any real issues.
Tried compiling on a half-upgraded Ubuntu 22.04 to 24.04, the system packages should already be the 24.04 versions. I'll try again later with another system. I encountered these issues:
CMake Error at <line in CMakeLists.txt> (target_link_libraries):
Target "<target>" links to:
zstd::zstd
but the target was not found. Possible reasons include:
* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.
Affected targets are rocksdb, db_bench, db_sanity_test, write_stress, db_repl_stress, rocksdb_dump, rocksdb_undump, db_stress. It looks like all of them are RocksDB-related.
Also, I would appreciate if you could add the necessary changes to keep the AWS and libarchive extensions enabled and working, so that we can eventually merge this PR standalone without breaking important functionality.
Thanks @szaszm for the steps to run a git rebase without causing data loss. I will give it a try. I think the reason we are seeing the zstd::zstd target not found issue is because we move from Ubuntu 22.04 to Ubuntu 24.04 and maybe conancenter doesn't provide the prebuilt conan package out of box. I think we just need to run these two conan commands:
conan install . --build=missing --output-folder=build_conan
conan build . --output-folder=build_conan
I will look into it today after work. I will follow your rebase steps for Ubuntu 22.04. Thanks again for sharing your steps. Later I will try Ubuntu 24.04. I believe there shouldn't be much of a difference.
@szaszm @lordgamez for this latest git push, I followed marton's steps to do an interactive rebase and also removed the top merge commit that I had to clean up the history and leave only my commits for the git history. Now PR-1793's branch MINIFICPP-2346-P1 should be on top of the latest MiNiFi main branch. I also verified I could build MiNiFi using conan with conan's cmake wrapper and then I followed up by verifying I could still build MiNiFi using standalone CMake. Also it looked like the TESTS were on there way to passing too. When you guys get a chance, please review.
Please make sure the CI workflows are green. I approved their run now, feel free to ping me on slack if you need more approvals. Or let me know if you don't see their output, and I'll post it here then.
@szaszm I tried out running the flake8 linter from build_cmake/ directory:
cd $HOME/nifi-minifi-cpp/build_cmake/
pip install flake8
export PATH="$PATH:~/.local/bin"
source ~/.bashrc
cmake --build . --target flake8
I get the following output (just showing the part related to conanfile.py:
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-develop/conanfile.py:2:1: F401 'conan.tools.env.VirtualRunEnv' imported but unused
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-develop/conanfile.py:14:99: W291 trailing whitespace
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-develop/conanfile.py:16:105: W291 trailing whitespace
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-develop/conanfile.py:22:1: E302 expected 2 blank lines, found 1
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-develop/conanfile.py:31:53: E231 missing whitespace after ','
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-develop/test_package/conanfile.py:7:1: E302 expected 2 blank lines, found 1
gmake[3]: *** [CMakeFiles/flake8.dir/build.make:70: CMakeFiles/flake8] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:2645: CMakeFiles/flake8.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:2652: CMakeFiles/flake8.dir/rule] Error 2
gmake: *** [Makefile:667: flake8] Error 2
@szaszm I resolved the linter issues coming from conanfile.py and test_package/conanfile.py at commit 1dd9160:
gmake[3]: *** [CMakeFiles/flake8.dir/build.make:70: CMakeFiles/flake8] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:2645: CMakeFiles/flake8.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:2652: CMakeFiles/flake8.dir/rule] Error 2
gmake: *** [Makefile:667: flake8] Error 2
flake is a linter, showing you the parts of the code that don't follow the predefined rules. Please fix them in the files you add or change. Let me know if you have any questions.
When you guys get a chance, can one of you approve the CI/CD workflow, so I can check out the result? @lordgamez @martinzink @szaszm
Hi @james94, I checked the build with Conan locally and it works fine for me. The only tests that are failing are the ControllerTests and the LoggerTests. The ControllerTests are failing because of the disabled LibArchive extension so that is understandable. The LoggerTests are failing with an fmt formatter test where the expected log formatting is 1m equals to 60s, but we get 1min equals to 60s. Could it be due to different versions used in our built fmt lib version and the fmt version pulled by Conan? Could you please check that out?
I really appreciate the step by step instructions for building the project using Conan, it would be great to have it as part of the documentation as well. Could you please add it to the documentation in README.md or maybe in a separate markdown file and a link to that in the README.md?
Thanks for all your effort!
Hi @lordgamez. I will look into LoggerTests and see if it is a difference in their expected log formatting and get back to you.
Also thats a great idea to add a section on building MiNiFi C++ using conan. I will consider creating a new markdown file and linking it to the README.md to save space.
Also I notice that MiNiFi is under NiFi projects. When we click on Quick Start Guide or Administrator's Guide, we are sent to MiNiFi github. Will we eventually just have this part of MiNiFi directly linked to the website's documentation? Is there interest in this effort?
- Here's the link to MiNiFi doc from NiFi: https://nifi.apache.org/projects/minifi/
Hi @lordgamez @szaszm , I added a section to the README.md on building MiNiFi and creating a MiNiFi conan package using conan. In the README.md in that conan section, it points to a CONAN.md that goes step by step on how to use conan with MiNiFi too.
- https://github.com/james94/nifi-minifi-cpp/tree/MINIFICPP-2346-P1?tab=readme-ov-file#build-minifi--create-minifi-package-using-conan-v2
- https://github.com/james94/nifi-minifi-cpp/blob/MINIFICPP-2346-P1/CONAN.md
I will check the LoggerTests part this week to see if the format is showing differently in conan fmt compared to CMake installed fmt because of the fmt version difference and get back to you.
Hi @lordgamez @szaszm
I fixed the LoggerTests fmt formatting issue "7def7a0":
For conan, we install fmt 10.2.1 prebuilt conan package to not conflict with spdlog 1.14.0 prebuilt conan package. However, when we switched to a newer version of fmt, that caused the LoggerTest to fail, which didn't happen when we were using fmt 10.1.0 that was version installed when we used the standalone CMake approach. So, we updated our TEST_CASE for 'fmt formatting works with the logger' to explicity create the chrono minutes duration, followed by fmt format and then we pass our formatted minutes duration to our logger->log_critical, so we get the expected fmt formatting to be '1m'. We didn't have to do this explicitly when we used fmt 10.1.0 since even if we passed our logger->log_critical '1min', it would auto format to '1m'. For fmt 10.2.1 to have backward compatibility with fmt 10.1.0 and the result we expected from this TEST_CASE in LoggerTests, we made this update.
I checked that when I build MiNiFi using conan and then build MiNiFi using standalone CMake, their LoggerTest PASSES.
Please let me know if these updates to LoggerTests.cpp are sufficient. Then when you get another chance, can you review my PR and approve if you think its ready?
Hi @lordgamez @szaszm
I fixed the LoggerTests fmt formatting issue "7def7a0":
For conan, we install fmt 10.2.1 prebuilt conan package to not conflict with spdlog 1.14.0 prebuilt conan package. However, when we switched to a newer version of fmt, that caused the LoggerTest to fail, which didn't happen when we were using fmt 10.1.0 that was version installed when we used the standalone CMake approach. So, we updated our TEST_CASE for 'fmt formatting works with the logger' to explicity create the chrono minutes duration, followed by fmt format and then we pass our formatted minutes duration to our logger->log_critical, so we get the expected fmt formatting to be '1m'. We didn't have to do this explicitly when we used fmt 10.1.0 since even if we passed our logger->log_critical '1min', it would auto format to '1m'. For fmt 10.2.1 to have backward compatibility with fmt 10.1.0 and the result we expected from this TEST_CASE in LoggerTests, we made this update. I checked that when I build MiNiFi using conan and then build MiNiFi using standalone CMake, their LoggerTest PASSES.
Please let me know if these updates to LoggerTests.cpp are sufficient. Then when you get another chance, can you review my PR and approve if you think its ready?
Thanks for the fix @james94 ! It works now and all the tests pass (besides ControllerTests which is expected at this point). The only problem I found was that some of the HTTP tests seem to work differently and are taking way too much time to finish, especially ListenHTTPTests. Could this be due to different curl versions maybe?
Test run when building from source: 201/203 Test #68: VerifyInvokeHTTPPostTest ..................... Passed 18.38 sec 199/203 Test #64: InvokeHTTPTests .............................. Passed 15.90 sec 198/203 Test #1: ListenHTTPTests .............................. Passed 13.93 sec
Test run using conan: 217/219 Test #85: VerifyInvokeHTTPPostTest ..................... Passed 33.06 sec 218/219 Test #81: InvokeHTTPTests .............................. Passed 38.32 sec 219/219 Test #9: ListenHTTPTests .............................. Passed 102.88 sec
Regarding the tests Gábor mentioned:
- ControllerTests: apparently that fails because it depends on libarchive
- slower HTTP tests: We should fix them, but I'm OK with merging this PR and fixing it in followups, since it doesn't regress the existing CMake build.
@lordgamez thanks for looking into the HTTP related CURL tests. Yeah for the conanfile.py file, we use libcurl version 8.6.0 and for standalone CMake, we use 8.4.0. It could be a libcurl version difference, but we can also consider in standalone CMake, we configure libcurl in cmake/BundledLibcURL.cmake before being built:
"-DCMAKE_INSTALL_PREFIX=${BINARY_DIR}/thirdparty/curl-install"
-DBUILD_CURL_EXE=OFF
-DBUILD_TESTING=OFF
-DBUILD_SHARED_LIBS=OFF
-DHTTP_ONLY=ON
-DCURL_CA_PATH=none
-DCURL_USE_LIBSSH2=OFF
-DUSE_LIBIDN2=OFF
-DCURL_USE_LIBPSL=OFF
-DCURL_USE_OPENSSL=ON
Whereas for conan, we install that libcurl prebuilt library using the default configs. If we do want to get closer to configuring conan's libcurl like we do for the standalone CMake, we can either make some updates to our conanfile.py or make updates to libcurl's conan recipe. Heres libcurl's conan recipe from conan-center-index repo and line 566 that is an example of how they set "BUILD_CURL_EXE" to False aka OFF:
- https://github.com/conan-io/conan-center-index/blob/master/recipes/libcurl/all/conanfile.py#L566
Do you think we see a difference in HTTP performance because of these configurations to libcurl build?
@lordgamez thanks for looking into the HTTP related CURL tests. Yeah for the conanfile.py file, we use libcurl version 8.6.0 and for standalone CMake, we use 8.4.0. It could be a libcurl version difference, but we can also consider in standalone CMake, we configure libcurl in cmake/BundledLibcURL.cmake before being built:
"-DCMAKE_INSTALL_PREFIX=${BINARY_DIR}/thirdparty/curl-install" -DBUILD_CURL_EXE=OFF -DBUILD_TESTING=OFF -DBUILD_SHARED_LIBS=OFF -DHTTP_ONLY=ON -DCURL_CA_PATH=none -DCURL_USE_LIBSSH2=OFF -DUSE_LIBIDN2=OFF -DCURL_USE_LIBPSL=OFF -DCURL_USE_OPENSSL=ONWhereas for conan, we install that libcurl prebuilt library using the default configs. If we do want to get closer to configuring conan's libcurl like we do for the standalone CMake, we can either make some updates to our conanfile.py or make updates to libcurl's conan recipe. Heres libcurl's conan recipe from conan-center-index repo and line 566 that is an example of how they set "BUILD_CURL_EXE" to False aka OFF:
- https://github.com/conan-io/conan-center-index/blob/master/recipes/libcurl/all/conanfile.py#L566
Do you think we see a difference in HTTP performance because of these configurations to libcurl build?
Hi @james94, I think both options are plausible to cause the difference in the test run, we should try to build from source with the newer version and with the default build options too and see if one of those builds behaves the same way as the conan build, and maybe check out the logs what causes this behavior. Maybe some timeouts are different, or some additional checks are added in the newer version, not sure. Unfortunately I didn't have time to try it out this week, maybe I'll have some free time to check these builds next week, or if you're up for it and have some time for it feel free to check it out.