You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "james94 (via GitHub)" <gi...@apache.org> on 2024/05/21 11:24:30 UTC
[PR] Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
james94 opened a new pull request, #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793
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
~~~bash
# make sure to install conan2 for your environment
sudo pip install --force-reinstall -v "conan==2.3.1"
# 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
conan install . --build=missing --output-folder=build_conan -pr=$HOME/nifi-minifi-cpp/etc/build/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/build/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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1714362473
##########
test_package/src/AbstractProcessorTest.cpp:
##########
@@ -0,0 +1,114 @@
+/**
Review Comment:
I think we should run the same unit tests as we would run normally, even in a Conan build. I'm not a fan of duplicating a single test and introducing a new test package for conan builds, it adds too much maintenance burden.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2123815671
@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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2124853146
@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 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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2189541466
@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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1793: MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2422897684
Thanks for the support everyone on this Conan integration effort with MiNiFi.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1718946350
##########
test_package/src/AbstractProcessorTest.cpp:
##########
@@ -0,0 +1,114 @@
+/**
Review Comment:
I decided to leave out the **cmake.ctest()** from being run automatically after conan finishes building MiNiFi C++, so the user will still need to go into **build_conan** folder and then run **ctest**. For the **conan build** and **conan create approaches** for MiNiFi C++, **217/219** TESTs PASS. For **standalone CMake approach,** **275/275** TESTs PASS.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2317658774
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1714371690
##########
test_package/src/AbstractProcessorTest.cpp:
##########
@@ -0,0 +1,114 @@
+/**
Review Comment:
We can update the conanfile.py to run existing MiNiFi C++ GTESTs, so we dont duplicate the already created tests. I was mainly copying over a MiNiFi test here for a quick check. I will try **cmake.ctest()**, so I can get rid of this copied test program and test_package/ folder.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1632094367
##########
CMakeLists.txt:
##########
Review Comment:
@szaszm I just realized I had a forgot to update the **USE_CONAN_PACKAGER** in the conanfile.py to be **MINIFI_USE_CONAN_PACKAGER** variable. Thats why we were seeing the OpenSSL, ZLib, and LibCURL being built from source instead of using the conan prebuilt versions. I just updated the conanfile.py.
I have shared the output from when I ran `conan install . --build=missing --output-folder=build_conan` similar to how you did using the **default conan profile**. I also attached the output from running the conan build. You'll see this time, MiNiFi conan cmake uses conan's prebuilt packages for OpenSSL, ZLib and LibCURL instead of building them from source.
[conan_install_default_profile_jguzman_060824.log](https://github.com/user-attachments/files/15749397/conan_install_default_profile_jguzman_060824.log)
[conan_build_default_profile_jguzman_060824.log](https://github.com/user-attachments/files/15749398/conan_build_default_profile_jguzman_060824.log)
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2282185105
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712219867
##########
conanfile.py:
##########
Review Comment:
I added ability to create **conan package** for **MiNiFi C++**, which is similar to CMake's **make package**. I updated the steps at the top of my PR above to include conan create command. Please give it a try when you get a chance @szaszm
Here is the command you can try and at the end after conan succesfully creates the MiNiFi C++ conan package, conan will run the GTEST(s) in the **test_package/** folder.
Here is the conan create command for reference:
~~~bash
cd $HOME/nifi-minifi-cpp
conan create . --user=minifi --channel=develop -pr=$HOME/nifi-minifi-cpp/etc/conan/profiles/release-linux
~~~
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1714371690
##########
test_package/src/AbstractProcessorTest.cpp:
##########
@@ -0,0 +1,114 @@
+/**
Review Comment:
We can update the conanfile.py for the test_package to try to run existing MiNiFi C++ GTESTs, so we dont duplicate the already created tests. I was mainly copying over a MiNiFi test here for a quick check. I will try **cmake.ctest()**, so I can get rid of this copied test program.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1718946350
##########
test_package/src/AbstractProcessorTest.cpp:
##########
@@ -0,0 +1,114 @@
+/**
Review Comment:
I decided to leave out the **cmake.ctest()** from being run automatically after conan finishes building MiNiFi C++, so the user will still need to go into **build_conan** folder and then run **ctest**. For the conan build and conan create approaches for MiNiFi C++, **217/219** TESTs PASS. For standalone CMake approach, **275/275** TESTs PASS.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1736130571
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
It works when I install the headers, but we shouldn't depend on system headers for a conan package.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1737196067
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
I just double checked the **conanfile.py**, it appears zlibs is not in conan package **shared_requires** tuple. Maybe it was removed by accident. I will add it back. Here will be the updated **shared_requires** tuple
~~~bash
shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6", "fmt/10.2.1", "spdlog/1.14.0", "catch2/3.5.4", "zlib/1.2.11")
~~~
We will try **zlib/1.2.11** for conan package to match the **zlib** system library version. @szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1737196067
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
**UPDATED (Sept 2, 2024)**: @szaszm
I just double checked the **conanfile.py**, it appears zlibs is not in conan package **shared_requires** tuple. I realized some other conan packages depended on **zlib**, so conan automatically downloaded **zlib 1.3.1** prebuilt conan package. I will add it explicitly, but with **zlib 1.2.11** for that conan package version to match the version that standalone CMake would install for MiNiFi. Here will be the updated **shared_requires** tuple
~~~bash
shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6", "fmt/10.2.1", "spdlog/1.14.0", "catch2/3.5.4", "zlib/1.2.11", "zstd/1.5.2")
~~~
I didn't see that missing header from zlib during compilation librdkafka anymore when building MiNiFi with conan build. I also added in **zstd/1.5.2** conan package to match standalone CMake's zstd version. I added this zstd conan package because BundledRocksDB was complaining about not being able to find **zstd::zstd**. So, for the conan build of MiNiFi, I added both zlib and zstd to build MiNiFi successfully.
I will double check by manually building MiNiFi with conan and get back to you, but for now, I updated the **conanfile.py** with **zlib** added to the **shared_requires** tuple and its on the repo now @szaszm
When building MiNiFi with conan build, **217/219 ctests PASS**.
34 - ProvenanceTests (Subprocess aborted)
219 - ControllerTests (Failed)
I think **ProvenanceTests** from RocksDB fails because of
double free or corruption (!prev)
Test #34: ProvenanceTests ........ Subprocess aborted***Exception: 0.19 sec
In regards to this **ProvenanceTests**, the main difference between MiNiFi conan build and standalone CMake build is that in conan build we use **zstd** and **zlib** conan packages to try to link into CMake's system library installed **RocksDB** while with standalone CMake approach, we use CMake to **build and install zlib & zstd external system libs** while installing system library **RocksDB**
@szaszm any thoughts on what could cause **ProvenanceTest** to have this **double free or corruption (!prev)** error when building MiNiFi using conan? This doesn't happen when I build MiNiFi with standalone CMake. Also in my much larger PR where I added support to install **patched RocksDB conan package** based on **standalone CMake's patched RocksDB**, I dont think I saw this ctest error: https://github.com/apache/nifi-minifi-cpp/pull/1775
When building MiNiFi with standalone CMake, **all 275 ctests PASS.**
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1733645535
##########
libminifi/test/unit/LoggerTests.cpp:
##########
@@ -380,7 +380,9 @@ TEST_CASE("Setting max log entry length to unlimited results in unlimited log en
TEST_CASE("fmt formatting works with the logger") {
LogTestController::getInstance().setTrace<logging::Logger>();
std::shared_ptr<logging::Logger> logger = logging::LoggerFactory<logging::Logger>::getLogger();
- logger->log_critical("{} equals to {}", 1min, 60s);
+ std::chrono::minutes duration{1};
Review Comment:
Made this update for compatibility between fmt 10.1.0 that is installed by standalone CMake approach and fmt 10.2.1 that is installed by conan approach. I verified after building MiNiFi using both approaches that the LoggerTest passes.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1737196067
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
**UPDATED (Sept 2, 2024)**: @szaszm
I just double checked the **conanfile.py**, it appears zlibs is not in conan package **shared_requires** tuple. I realized some other conan packages depended on **zlib**, so conan automatically downloaded **zlib 1.3.1** prebuilt conan package. I will add it explicitly, but with **zlib 1.2.11** for that conan package version to match the version that standalone CMake would install for MiNiFi. Here will be the updated **shared_requires** tuple
~~~bash
shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6", "fmt/10.2.1", "spdlog/1.14.0", "catch2/3.5.4", "zlib/1.2.11", "zstd/1.5.2")
~~~
I didn't see that missing header from zlib during compilation **librdkafka** anymore when building MiNiFi with conan build. I also added in **zstd/1.5.2** conan package to match standalone CMake's zstd version. I added this zstd conan package because BundledRocksDB was complaining about not being able to find **zstd::zstd**. So, for the conan build of MiNiFi, I added both zlib and zstd to build MiNiFi successfully.
I will double check by manually building MiNiFi with conan and get back to you, but for now, I updated the **conanfile.py** with **zlib** added to the **shared_requires** tuple and its on the repo now @szaszm
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
@szaszm When building MiNiFi with conan build, **217/219 ctests PASS**.
34 - ProvenanceTests (Subprocess aborted)
219 - ControllerTests (Failed)
I think **ProvenanceTests** from RocksDB fails because of
double free or corruption (!prev)
Test #34: ProvenanceTests ........ Subprocess aborted***Exception: 0.19 sec
In regards to this **ProvenanceTests**, the main difference between MiNiFi conan build and standalone CMake build is that in conan build we use **zstd** and **zlib** conan packages to try to link into CMake's system library installed **RocksDB** while with standalone CMake approach, we use CMake to **build and install zlib & zstd external system libs** while installing system library **RocksDB**
@szaszm any thoughts on what could cause **ProvenanceTest** to have this **double free or corruption (!prev)** error when building MiNiFi using conan? This doesn't happen when I build MiNiFi with standalone CMake. Also in my much larger PR where I added support to install **patched RocksDB conan package** based on **standalone CMake's patched RocksDB**, I dont think I saw this ctest error: https://github.com/apache/nifi-minifi-cpp/pull/1775
When building MiNiFi with standalone CMake, **all 275 ctests PASS.**
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2331896597
> @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:
>
> ```shell
> "-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?
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1661078485
##########
conanfile.py:
##########
Review Comment:
I would change the default CMake target to `package` when building with Conan. When using CMake directly, the user can just call `make` or `make package` as they wish. But since it's not possible, or not immediately obvious how to do this with `conan build`, I'd do a build that's as complete as possible.
##########
conanfile.py:
##########
Review Comment:
just a random thought: I don't want any of the changes to prevent future adoption of vcpkg, as it seems to be the more popular c++ package manager. Do you think this would be possible?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1628099101
##########
CMakeLists.txt:
##########
Review Comment:
Ah I see. If you ran into an issue with finding the conan_toolchain.cmake, then that could make sense why OpenSSL and Zlib were built from source instead of using the conan packages. Heres a snippet from the conanfile.py relevant to conan_toolchain:
~~~python
tc = CMakeToolchain(self)
tc.variables["USE_CONAN_PACKAGER"] = "ON"
tc.variables["USE_STANDALONE_CMAKE"] = "OFF"
~~~
If the CMakeToolchain() failed to be created due to not being able to find the conan_toolchain.cmake, then the USE_CONAN_PACKAGER = "ON" wouldn't have happened and thus we would see that the OpenSSL and Zlib sources are used instead of conan package. I will try out your steps to see if I run into the issues you ran into.
I do want to note that I did use a conan profile along minifi path `nifi-minifi-cpp/etc/build/conan/profiles/release-linux`, so its common amongst build environments.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1752526587
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
@lordgamez @szaszm for the conan build MiNiFi side, do we need to get ProvenanceTests working for this PR or could we get that Test passing for a follow up PR?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2278781324
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1737196067
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
**UPDATED (Sept 1, 2024)**: @szaszm
I just double checked the **conanfile.py**, it appears zlibs is not in conan package **shared_requires** tuple. I realized some other conan packages depended on **zlib**, so conan automatically downloaded **zlib 1.3.1** prebuilt conan package. I will add it explicitly. Here will be the updated **shared_requires** tuple
~~~bash
shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6", "fmt/10.2.1", "spdlog/1.14.0", "catch2/3.5.4", "zlib/1.3.1")
~~~
I will try **zlib/1.3.1** for conan package, which is different **zlib/1.2.11** system library version. We shouldn't see that missing headers from zlib during compilation anymore, but if we do, I will use zlib's CMake variables to provide the path to the needed headers.
I will double check by manually building MiNiFi with conan and get back to you, but for now, I updated the **conanfile.py** with **zlib** added to the **shared_requires** tuple and its on the repo now @szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2340004862
Hi @james94, I tried building the curl library from source using version 8.6.0 and the default build options and it seems to be working the same way as before, having the runtime around 11-12 seconds, so I'm not sure at the moment what's causing the difference with the conan build.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1737196067
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
I just double checked the **conanfile.py**, it appears zlibs is not in conan package **shared_requires** tuple. Maybe it was removed by accident. I will add it back. Here will be the updated **shared_requires** tuple
~~~bash
shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6", "fmt/10.2.1", "spdlog/1.14.0", "catch2/3.5.4", "zlib/1.2.11")
~~~
We added **zlib/1.2.11** for conan package to match the **zlib** system library version. We shouldn't see that missing headers from zlib during compilation anymore. I will double check by manually building MiNiFi with conan and get back to you, but for now, I updated the **conanfile.py** with **zlib** added to the **shared_requires** tuple and its on the repo now @szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2301411483
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!
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2124941465
I'm having trouble with conan, due to a missing profile. It seems like installation alone is not enough.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2149629299
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1627791848
##########
CMakeLists.txt:
##########
Review Comment:
Another issue now, during `conan build`:
```
CMake Error at /home/szaszm/.local/lib/python3.10/site-packages/cmake/data/share/cmake-3.26/Modules/CMakeDetermineSystem.cmake:154 (message):
Could not find toolchain file: conan_toolchain.cmake
Call Stack (most recent call first):
CMakeLists.txt:5 (project)
```
I'm commenting this on a file so you can reply in a thread.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2309603288
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.
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1736081140
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
There is some issue with the zlib conan build, I'm getting missing headers. Did you test compilation with the zlib headers not installed on the system?
```
In file included from /home/szaszm/nifi-minifi-cpp/build_conan/extensions/librdkafka/kafka-external-prefix/src/kafka-external/src/rdkafka_buf.h:32,
from /home/szaszm/nifi-minifi-cpp/build_conan/extensions/librdkafka/kafka-external-prefix/src/kafka-external/src/rdkafka_int.h:88,
from /home/szaszm/nifi-minifi-cpp/build_conan/extensions/librdkafka/kafka-external-prefix/src/kafka-external/src/rdavl.c:29:
/home/szaszm/nifi-minifi-cpp/build_conan/extensions/librdkafka/kafka-external-prefix/src/kafka-external/src/rdcrc32.h:57:10: fatal error: zlib.h: No such file or directory
57 | #include <zlib.h>
| ^~~~~~~~
compilation terminated.
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712666278
##########
test_package/src/AbstractProcessorTest.cpp:
##########
@@ -0,0 +1,114 @@
+/**
Review Comment:
I realized I forgot to add this conan test_package/ folder that comes with an example **AbstractProcessorTest.cpp**, which is the same one from the MiNiFi GTESTs. This helps us test if features are available from the MiNiFi conan package. We can simulate if we were to add MiNiFi C++ library to a C++ project, would the features we need be available to it. For now, I just copied this AbstractProcessorTest.cpp GTEST example. This test_package/ comes with CMakeLists.txt and its own conanfile.py too.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2292632530
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
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1704728639
##########
conanfile.py:
##########
Review Comment:
@szaszm I will come back to address the feedback this week. I will get back to you soon with the conan create package update, which is similar to cmake make package.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2175568753
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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1628099101
##########
CMakeLists.txt:
##########
Review Comment:
Ah I see. If you ran into an issue with finding the conan_toolchain.cmake, then that could make sense why OpenSSL and Zlib were built from source instead of using the conan packages. Heres a snippet from the conanfile.py relevant to conan_toolchain:
~~~python
tc = CMakeToolchain(self)
tc.variables["MINIFI_USE_CONAN_PACKAGER"] = "ON"
~~~
If the CMakeToolchain() failed to be created due to not being able to find the conan_toolchain.cmake, then the **MINIFI_USE_CONAN_PACKAGER = "ON"** wouldn't have happened and thus we would see that the OpenSSL and Zlib sources are used instead of conan package. I will try out your steps to see if I run into the issues you ran into.
I do want to note that I did use a conan profile along minifi path `nifi-minifi-cpp/etc/build/conan/profiles/release-linux`, so its common amongst build environments.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1632099498
##########
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:
I will make the update to this PR for **OpenSSL, LibCURL, ZLib** @szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1632101312
##########
conanfile.py:
##########
Review Comment:
I agree with your suggestion for now that for all the feature selection options that are not **OpenSSL, CURL, ZLib**, we go with the default selected options currently in MiNiFiOptions.cmake and for now we use the normal **FetchContent/ExternalProject** for the rest of MiNiFi C++ external lib dependencies.
Yes sounds good. For now, I will leave the default enabled MiNiFi Options set as they are in MiNiFiOptions.cmake and remove the **tc.variables[...]** ones from conanfile.py. We will only bring in **tc.variables[...]** variables when we are overriding one of the existing MiNiFi options from the CMake side with conan. This will only be relevant for the new MiNiFi options we discussed above for example **MINIFI_LIBCURL_SOURCE** and the other 2 libs sources.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1741173901
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
@szaszm @lordgamez When building MiNiFi with conan build, **217/219 ctests PASS**.
34 - ProvenanceTests (Subprocess aborted)
219 - ControllerTests (Failed)
I think **ProvenanceTests** from RocksDB fails because of
double free or corruption (!prev)
Test #34: ProvenanceTests ........ Subprocess aborted***Exception: 0.19 sec
In regards to this **ProvenanceTests**, the main difference between MiNiFi conan build and standalone CMake build is that in conan build we use **zstd** and **zlib** conan packages to try to link into CMake's system library installed **RocksDB** while with standalone CMake approach, we use CMake to **build and install zlib & zstd external system libs** while installing system library **RocksDB**
@szaszm any thoughts on what could cause **ProvenanceTest** to have this **double free or corruption (!prev)** error when building MiNiFi using conan? This doesn't happen when I build MiNiFi with standalone CMake. Also in my much larger PR where I added support to install **patched RocksDB conan package** based on **standalone CMake's patched RocksDB**, I dont think I saw this ctest error: https://github.com/apache/nifi-minifi-cpp/pull/1775
When building MiNiFi with standalone CMake, **all 275 ctests PASS.**
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1609216217
##########
cmake/BundledZLIB.cmake:
##########
@@ -18,56 +18,65 @@
function(use_bundled_zlib SOURCE_DIR BINARY_DIR)
Review Comment:
I incorporated your feedback into commit [cf2face](https://github.com/apache/nifi-minifi-cpp/pull/1793/commits/cf2facee3a1ecc2909d439a4fe9cd1ca60830617)
I do like your suggestion, it makes the MiNiFi CMake code look cleaner with the conan v2.3.1 integration.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712219867
##########
conanfile.py:
##########
Review Comment:
I added ability to create **conan package** for **MiNiFi C++**, which is similar to CMake's **make package**. I updated the steps at the top of my PR above to include conan create command. Please give it a try when you get a chance @szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712441351
##########
CMakeLists.txt:
##########
@@ -593,9 +596,11 @@ set(CPACK_PACKAGE_VENDOR "Apache NiFi")
set(CPACK_PACKAGE_VERSION_MAJOR ${PROJECT_VERSION_MAJOR})
set(CPACK_PACKAGE_VERSION_MINOR ${PROJECT_VERSION_MINOR})
set(CPACK_PACKAGE_VERSION_PATCH ${PROJECT_VERSION_PATCH})
-set(CPACK_PACKAGE_DESCRIPTION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
-configure_file("${CMAKE_CURRENT_SOURCE_DIR}/LICENSE" "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt" COPYONLY)
-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt")
+if(NOT MINIFI_BUILD_CONAN_PACKAGE)
+ set(CPACK_PACKAGE_DESCRIPTION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
+ configure_file("${CMAKE_CURRENT_SOURCE_DIR}/LICENSE" "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt" COPYONLY)
+ set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt")
+endif()
Review Comment:
When we create the **MiNiFi C++ conan package**, **LICENSE** and **NOTICE** are in the conan package. In the **conanfile.py**, we tell conan to include all the files assigned to **shared_sources** variable to be included in the conan package. In our conanfile.py's **package()** method, we also run **cmake.install()**, so in MiNiFi C++'s CMakeLists.txt files all CMake commands using **install(...)** will also copy over those sources (.h*, .i*, etc) and compiled binary assets (static libs, shared libs, minifi binary, etc) into our MiNiFi C++ conan package.
In the past when I tried maintaining that CPACK part way of doing it, when I ran **conan create**, it complained about those 3 lines. However, today with **conan 2.0.17** I removed that MINIFI_BUILD_CONAN_PACKAGE check and was able to successfully run **conan create** with those 3 lines enabled. I will push the update commit up to the repo now.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712442759
##########
CMakeLists.txt:
##########
@@ -593,9 +596,11 @@ set(CPACK_PACKAGE_VENDOR "Apache NiFi")
set(CPACK_PACKAGE_VERSION_MAJOR ${PROJECT_VERSION_MAJOR})
set(CPACK_PACKAGE_VERSION_MINOR ${PROJECT_VERSION_MINOR})
set(CPACK_PACKAGE_VERSION_PATCH ${PROJECT_VERSION_PATCH})
-set(CPACK_PACKAGE_DESCRIPTION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
-configure_file("${CMAKE_CURRENT_SOURCE_DIR}/LICENSE" "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt" COPYONLY)
-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt")
+if(NOT MINIFI_BUILD_CONAN_PACKAGE)
+ set(CPACK_PACKAGE_DESCRIPTION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
+ configure_file("${CMAKE_CURRENT_SOURCE_DIR}/LICENSE" "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt" COPYONLY)
+ set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt")
+endif()
Review Comment:
Heres a view of my minifi cpp conan package p/ folder where you can see **LICENSE, NOTICE** and other assets copied over.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2282181692
@szaszm I tried out running the flake8 linter from build_cmake/ directory:
~~~bash
cd $HOME/nifi-minifi-cpp
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:
~~~bash
/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
~~~
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712690658
##########
conanfile.py:
##########
@@ -0,0 +1,115 @@
+from conan import ConanFile
+from conan.tools.env import VirtualRunEnv
+from conan.tools.cmake import CMake, CMakeToolchain
+from conan.tools.files import collect_libs, copy
+import os
+
+required_conan_version = ">=2.0"
+
+shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6",
+ "catch2/3.5.4", "fmt/10.2.1", "spdlog/1.14.0")
+
+shared_sources = ("CMakeLists.txt", "libminifi/*", "extensions/*", "minifi_main/*", "nanofi/*",
+ "bin/*", "bootstrap/*", "cmake/*", "conf/*", "controller/*", "encrypt-config/*",
+ "etc/*", "examples/*", "msi/*", "thirdparty/*", "docker/*", "LICENSE", "NOTICE",
+ "README.md", "C2.md", "CONFIGURE.md", "CONTRIBUTING.md", "CONTROLLERS.md", "EXPRESSIONS.md",
+ "Extensions.md", "JNI.md", "METRICS.md", "OPS.md", "PROCESSORS.md", "ThirdParties.md",
+ "Windows.md", "aptitude.sh", "arch.sh", "bootstrap.sh", "bstrp_functions.sh", "centos.sh",
+ "CPPLINT.cfg", "darwin.sh", "debian.sh", "deploy.sh", "fedora.sh", "generateVersion.sh",
+ "linux.sh", "rheldistro.sh", "run_clang_tidy.sh", "run_clang_tidy.sh", "run_flake8.sh",
+ "run_shellcheck.sh", "suse.sh", "versioninfo.rc.in")
+
+class MiNiFiCppMain(ConanFile):
+ name = "minifi-cpp"
+ version = "0.99.1"
+ license = "Apache-2.0"
+ requires = shared_requires
+ settings = "os", "compiler", "build_type", "arch"
+ generators = "CMakeDeps"
+ options = {"shared": [True, False], "fPIC": [True, False]}
+
+ default_options = {"shared": False, "fPIC": True,}
+
+ exports_sources = shared_sources
+
+ def generate(self):
+ tc = CMakeToolchain(self)
+
+ tc.variables["MINIFI_LIBCURL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_OPENSSL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_ZLIB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CIVETWEB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_LIBXML2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CATCH2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_FMT_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_SPDLOG_SOURCE"] = "CONAN"
+
+ tc.variables["SKIP_TESTS"] = "OFF"
+ tc.variables["ENABLE_OPENWSMAN"] = "ON"
+ tc.variables["ENABLE_CIVET"] = "ON"
+ tc.variables["ENABLE_EXPRESSION_LANGUAGE"] = "ON"
+ tc.variables["ENABLE_BZIP2"] = "ON"
+ tc.variables["ENABLE_ROCKSDB"] = "ON"
+ tc.variables["BUILD_ROCKSDB"] = "ON"
+
+ tc.variables["ENABLE_LIBARCHIVE"] = "OFF"
Review Comment:
Yes it only affects the Conan build part of MiNiFi. As a reminder, in my bigger MiNiFi Conan PR for MiNiFi, I already verified I could ENABLE most of these MiNiFi features using Conan. I just didn't want this PR to be super large. Yes we can ENABLE more of MiNiFi features using Conan as we do more follow up PRs.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1632099378
##########
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:
@szaszm I think I see where you are talking about with a custom option that has multiple possible values. This is the example I found in MiNiFiOptions.cmake
~~~cmake
# Option: STRICT_GSL_CHECKS
# AUDIT: Enable all checks, including gsl_ExpectsAudit() and gsl_EnsuresAudit()
# ON: Enable all checks, excluding gsl_ExpectsAudit() and gsl_EnsuresAudit() (GSL default)
# DEBUG_ONLY: Like ON in the Debug configuration, OFF in others (MiNiFi C++ default)
# OFF: Throw on contract checking and assertion failures instead of calling std::terminate()
set(STRICT_GSL_CHECKS "DEBUG_ONLY" CACHE STRING "Contract checking and assertion failures call terminate")
list(APPEND STRICT_GSL_CHECKS_Values AUDIT ON DEBUG_ONLY OFF)
set_property(CACHE STRICT_GSL_CHECKS PROPERTY STRINGS ${STRICT_GSL_CHECKS_Values})
~~~
I agree I think it would be great to create new the CMake options for the source of how we retrieve external libs like the following:
~~~cmake
# Option: MINIFI_LIBCURL_SOURCE
# BUILD: MiNiFi LibCURL source retrieved from CMake's ExternalProject, FetchContent, etc
# CONAN: MiNiFi LibCURL source retrieved from conan packager
# SYSTEM: MiNiFi LibCURL source retrieved from native OS system
set(MINIFI_LIBCURL_SOURCE "BUILD" CACHE STRING "Builds LibCURL from source using standalone CMake approach"
list(APPEND MINIFI_LIBCURL_SOURCE_Values SYSTEM BUILD CONAN)
set_property(MINIFI_LIBCURL_SOURCE PROPERTY STRINGS ${MINIFI_LIBCURL_SOURCE_Values})
~~~
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2157569050
@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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2313758391
Hi @lordgamez @szaszm
I fixed the LoggerTests fmt formatting issue:
>
> 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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2125181150
@james94 what docker environment do you mean? I was trying on arch linux, no containers.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2125050170
@szaszm just in case you run into an issue with conan finding the minifi conan profile, you may need to create a default profile for conan first:
~~~bash
# 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:
~~~bash
# 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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2141170468
@szaszm @martinzink @lordgamez just checking in, anything I can do to help speed up testing toward this PR?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1632101312
##########
conanfile.py:
##########
Review Comment:
I agree with your suggestion for now that for all the feature selection options that are not **OpenSSL, CURL, ZLib**, we go with the default selected options currently in MiNiFiOptions.cmake and for now we use the normal **FetchContent/ExternalProject** for the rest of MiNiFi C++ external lib dependencies.
Yes sounds good. For now, I will leave the default enabled MiNiFi Options set as they are in MiNiFiOptions.cmake and remove the **tc.variables[...]** ones from conanfile.py. We will only bring in **tc.variables[...]** variables when we are overriding one of the existing MiNiFi options from the CMake side with conan. This will only be relevant for the new MiNiFi options we discussed above for example **MINIFI_LIBCURL_SOURCE** and the other 2 libs sources.
@szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712232992
##########
CMakeLists.txt:
##########
@@ -593,9 +596,11 @@ set(CPACK_PACKAGE_VENDOR "Apache NiFi")
set(CPACK_PACKAGE_VERSION_MAJOR ${PROJECT_VERSION_MAJOR})
set(CPACK_PACKAGE_VERSION_MINOR ${PROJECT_VERSION_MINOR})
set(CPACK_PACKAGE_VERSION_PATCH ${PROJECT_VERSION_PATCH})
-set(CPACK_PACKAGE_DESCRIPTION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
-configure_file("${CMAKE_CURRENT_SOURCE_DIR}/LICENSE" "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt" COPYONLY)
-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt")
+if(NOT MINIFI_BUILD_CONAN_PACKAGE)
+ set(CPACK_PACKAGE_DESCRIPTION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/README.md")
+ configure_file("${CMAKE_CURRENT_SOURCE_DIR}/LICENSE" "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt" COPYONLY)
+ set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_BINARY_DIR}/LICENSE.txt")
+endif()
Review Comment:
Why are these disabled in conan packages? We need to include the LICENSE and NOTICE files in every distribution, including binary ones.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1655482613
##########
conanfile.py:
##########
@@ -0,0 +1,37 @@
+from conan import ConanFile
+from conan.tools.env import VirtualRunEnv
+from conan.tools.cmake import CMake, CMakeToolchain
+
+required_conan_version = ">=2.1.0"
Review Comment:
Sorry to hear that pip messed up your path with broken CMake binaries. Yeah of course we can look into lowering the conan version to 2.0. I will also try out **conan 2.0.17** directly to be on same version that Nix supports installing.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712663897
##########
conanfile.py:
##########
@@ -0,0 +1,115 @@
+from conan import ConanFile
+from conan.tools.env import VirtualRunEnv
+from conan.tools.cmake import CMake, CMakeToolchain
+from conan.tools.files import collect_libs, copy
+import os
+
+required_conan_version = ">=2.0"
+
+shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6",
+ "catch2/3.5.4", "fmt/10.2.1", "spdlog/1.14.0")
+
+shared_sources = ("CMakeLists.txt", "libminifi/*", "extensions/*", "minifi_main/*", "nanofi/*",
+ "bin/*", "bootstrap/*", "cmake/*", "conf/*", "controller/*", "encrypt-config/*",
+ "etc/*", "examples/*", "msi/*", "thirdparty/*", "docker/*", "LICENSE", "NOTICE",
+ "README.md", "C2.md", "CONFIGURE.md", "CONTRIBUTING.md", "CONTROLLERS.md", "EXPRESSIONS.md",
+ "Extensions.md", "JNI.md", "METRICS.md", "OPS.md", "PROCESSORS.md", "ThirdParties.md",
+ "Windows.md", "aptitude.sh", "arch.sh", "bootstrap.sh", "bstrp_functions.sh", "centos.sh",
+ "CPPLINT.cfg", "darwin.sh", "debian.sh", "deploy.sh", "fedora.sh", "generateVersion.sh",
+ "linux.sh", "rheldistro.sh", "run_clang_tidy.sh", "run_clang_tidy.sh", "run_flake8.sh",
+ "run_shellcheck.sh", "suse.sh", "versioninfo.rc.in")
+
+class MiNiFiCppMain(ConanFile):
+ name = "minifi-cpp"
+ version = "0.99.1"
+ license = "Apache-2.0"
+ requires = shared_requires
+ settings = "os", "compiler", "build_type", "arch"
+ generators = "CMakeDeps"
+ options = {"shared": [True, False], "fPIC": [True, False]}
+
+ default_options = {"shared": False, "fPIC": True,}
+
+ exports_sources = shared_sources
+
+ def generate(self):
+ tc = CMakeToolchain(self)
+
+ tc.variables["MINIFI_LIBCURL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_OPENSSL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_ZLIB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CIVETWEB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_LIBXML2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CATCH2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_FMT_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_SPDLOG_SOURCE"] = "CONAN"
+
+ tc.variables["SKIP_TESTS"] = "OFF"
+ tc.variables["ENABLE_OPENWSMAN"] = "ON"
+ tc.variables["ENABLE_CIVET"] = "ON"
+ tc.variables["ENABLE_EXPRESSION_LANGUAGE"] = "ON"
+ tc.variables["ENABLE_BZIP2"] = "ON"
+ tc.variables["ENABLE_ROCKSDB"] = "ON"
+ tc.variables["BUILD_ROCKSDB"] = "ON"
+
+ tc.variables["ENABLE_LIBARCHIVE"] = "OFF"
Review Comment:
As long as this only affects the Conan build, and they will be re-enabled in later iterations, then I'm fine with it. But if we end up long-term with a limited Conan build that lacks essential features, then that's a maintenance burden that adds little value.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1764491347
##########
cmake/BundledRocksDB.cmake:
##########
@@ -63,10 +64,17 @@ function(use_bundled_rocksdb SOURCE_DIR BINARY_DIR)
-DROCKSDB_INSTALL_ON_WINDOWS=ON
-DWITH_XPRESS=ON)
else()
+ if(MINIFI_ZSTD_SOURCE STREQUAL "CONAN" AND MINIFI_ZLIB_SOURCE STREQUAL "CONAN")
+ list(APPEND ROCKSDB_CMAKE_ARGS
+ -DWITH_ZLIB=OFF
+ -DWITH_ZSTD=OFF)
+ else()
Review Comment:
It is possible to fix RocksDB with conan zlib and zstd. I will try to add support for RocksDB conan as alternative to RocksDB cmake, so we can undo the updates that I made to BundledRocksDB.cmake, so that BundledRocksDB.cmake just stays standalone CMake specific and separate from conan. I will follow up on this.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2125301849
@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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2122713677
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2156261471
@szaszm @martinzink with respect to commit **"[49de56c](https://github.com/apache/nifi-minifi-cpp/pull/1793/commits/49de56cd6a559a567e4df7e6037473f8eebd74f6)"**: 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:
~~~bash
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:
~~~bash
mkdir build_cmake
cd build_cmake
cmake ..
make -j $(nproc)
~~~
Please try the conan build MiNiFi C++ approach again when you get a chance
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1712571218
##########
conanfile.py:
##########
@@ -0,0 +1,115 @@
+from conan import ConanFile
+from conan.tools.env import VirtualRunEnv
+from conan.tools.cmake import CMake, CMakeToolchain
+from conan.tools.files import collect_libs, copy
+import os
+
+required_conan_version = ">=2.0"
+
+shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6",
+ "catch2/3.5.4", "fmt/10.2.1", "spdlog/1.14.0")
+
+shared_sources = ("CMakeLists.txt", "libminifi/*", "extensions/*", "minifi_main/*", "nanofi/*",
+ "bin/*", "bootstrap/*", "cmake/*", "conf/*", "controller/*", "encrypt-config/*",
+ "etc/*", "examples/*", "msi/*", "thirdparty/*", "docker/*", "LICENSE", "NOTICE",
+ "README.md", "C2.md", "CONFIGURE.md", "CONTRIBUTING.md", "CONTROLLERS.md", "EXPRESSIONS.md",
+ "Extensions.md", "JNI.md", "METRICS.md", "OPS.md", "PROCESSORS.md", "ThirdParties.md",
+ "Windows.md", "aptitude.sh", "arch.sh", "bootstrap.sh", "bstrp_functions.sh", "centos.sh",
+ "CPPLINT.cfg", "darwin.sh", "debian.sh", "deploy.sh", "fedora.sh", "generateVersion.sh",
+ "linux.sh", "rheldistro.sh", "run_clang_tidy.sh", "run_clang_tidy.sh", "run_flake8.sh",
+ "run_shellcheck.sh", "suse.sh", "versioninfo.rc.in")
+
+class MiNiFiCppMain(ConanFile):
+ name = "minifi-cpp"
+ version = "0.99.1"
+ license = "Apache-2.0"
+ requires = shared_requires
+ settings = "os", "compiler", "build_type", "arch"
+ generators = "CMakeDeps"
+ options = {"shared": [True, False], "fPIC": [True, False]}
+
+ default_options = {"shared": False, "fPIC": True,}
+
+ exports_sources = shared_sources
+
+ def generate(self):
+ tc = CMakeToolchain(self)
+
+ tc.variables["MINIFI_LIBCURL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_OPENSSL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_ZLIB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CIVETWEB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_LIBXML2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CATCH2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_FMT_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_SPDLOG_SOURCE"] = "CONAN"
+
+ tc.variables["SKIP_TESTS"] = "OFF"
+ tc.variables["ENABLE_OPENWSMAN"] = "ON"
+ tc.variables["ENABLE_CIVET"] = "ON"
+ tc.variables["ENABLE_EXPRESSION_LANGUAGE"] = "ON"
+ tc.variables["ENABLE_BZIP2"] = "ON"
+ tc.variables["ENABLE_ROCKSDB"] = "ON"
+ tc.variables["BUILD_ROCKSDB"] = "ON"
+
+ tc.variables["ENABLE_LIBARCHIVE"] = "OFF"
Review Comment:
For ease of creating the MiNiFi C++ conan package, I disabled the following libraries LIBARCHIVE to CONTROLLER. For a follow up PR, I can work on making sure that each of these features can come ENABLED or DISABLED when creating the MiNiFi C++ conan package. For now, I think this is fine. What are your thoughts @szaszm ?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2305151712
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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1718960988
##########
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:
Currently in our conanfile.py, we have the following MINIFI external libraries sources set to CONAN:
~~~python
tc.variables["MINIFI_LIBCURL_SOURCE"] = "CONAN"
tc.variables["MINIFI_OPENSSL_SOURCE"] = "CONAN"
tc.variables["MINIFI_ZLIB_SOURCE"] = "CONAN"
tc.variables["MINIFI_CIVETWEB_SOURCE"] = "CONAN"
tc.variables["MINIFI_LIBXML2_SOURCE"] = "CONAN"
tc.variables["MINIFI_FMT_SOURCE"] = "CONAN"
tc.variables["MINIFI_SPDLOG_SOURCE"] = "CONAN"
tc.variables["MINIFI_CATCH2_SOURCE"] = "CONAN"
~~~
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1754357747
##########
cmake/BundledRocksDB.cmake:
##########
@@ -63,10 +64,17 @@ function(use_bundled_rocksdb SOURCE_DIR BINARY_DIR)
-DROCKSDB_INSTALL_ON_WINDOWS=ON
-DWITH_XPRESS=ON)
else()
+ if(MINIFI_ZSTD_SOURCE STREQUAL "CONAN" AND MINIFI_ZLIB_SOURCE STREQUAL "CONAN")
+ list(APPEND ROCKSDB_CMAKE_ARGS
+ -DWITH_ZLIB=OFF
+ -DWITH_ZSTD=OFF)
+ else()
Review Comment:
I think we should fix RocksDB with Conan zlib and zstd instead, if that's possible. Or is that not possible or feasible?
##########
cmake/GetZstd.cmake:
##########
@@ -0,0 +1,27 @@
+# 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.
+
+function(get_zstd)
+ if(MINIFI_ZSTD_SOURCE STREQUAL "CONAN")
+ message("Using Conan Packager to manage installing prebuilt zstd external lib")
Review Comment:
```suggestion
message("Using Conan Packager to install zstd")
```
##########
cmake/GetZLIB.cmake:
##########
@@ -0,0 +1,27 @@
+# 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.
+
+function(get_zlib SOURCE_DIR BINARY_DIR)
+ if(MINIFI_ZLIB_SOURCE STREQUAL "CONAN")
+ message("Using Conan Packager to manage installing prebuilt zlib external lib")
Review Comment:
```suggestion
message("Using Conan to install zlib")
```
##########
cmake/GetZstd.cmake:
##########
@@ -0,0 +1,27 @@
+# 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.
+
+function(get_zstd)
+ if(MINIFI_ZSTD_SOURCE STREQUAL "CONAN")
+ message("Using Conan Packager to manage installing prebuilt zstd external lib")
+ find_package(zstd REQUIRED)
+ add_library(zstd::zstd ALIAS zstd::libzstd_static)
+ elseif(MINIFI_ZSTD_SOURCE STREQUAL "BUILD")
+ message("Using CMAKE's ExternalProject_Add to manage source building zstd external lib")
Review Comment:
```suggestion
message("Using CMake to build zstd from source")
```
##########
etc/conan/profiles/release-linux:
##########
@@ -0,0 +1,12 @@
+[settings]
+os=Linux
+arch=x86_64
+build_type=Release
+compiler=gcc
+compiler.cppstd=gnu20
Review Comment:
We generally prefer ISO standard over GNU.
```suggestion
compiler.cppstd=20
```
##########
cmake/GetZLIB.cmake:
##########
@@ -0,0 +1,27 @@
+# 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.
+
+function(get_zlib SOURCE_DIR BINARY_DIR)
+ if(MINIFI_ZLIB_SOURCE STREQUAL "CONAN")
+ message("Using Conan Packager to manage installing prebuilt zlib external lib")
+ find_package(ZLIB REQUIRED)
+ elseif(MINIFI_ZLIB_SOURCE STREQUAL "BUILD")
+ message("Using CMAKE's ExternalProject_Add to manage source building zlib external lib")
Review Comment:
```suggestion
message("Using CMake to build zlib from source")
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1628450263
##########
CMakeLists.txt:
##########
Review Comment:
I don't have that conan profile in the minifi source tree, so I just used the default from conan detect. It's the default of the environment, so I guess it shouldn't matter.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1628527729
##########
CMakeLists.txt:
##########
Review Comment:
Thanks for the heads up on the **conan profile** not being in the PR, I just realized **.gitignore** prevented it from showing up cause one of the folders got caught in the ignore. I updated the path to **release-linux** and now its in `nifi-minifi-cpp/etc/conan/profiles/release-linux`. You should be able to use it as you run **conan install** and **conan build** commands @szaszm
Also I think the default conan profile should work out of box, but to be consistent, I would suggest you also give the **release-linux** conan profile a try and see if you get similar results that I got for **conan install and conan build**. I have also attached logs for your reference.
[minifi_conan_install_output_jguzman_060524.txt](https://github.com/user-attachments/files/15597404/minifi_conan_install_output_jguzman_060524.txt)
[minifi_conan_build_output_jguzman_060524.txt](https://github.com/user-attachments/files/15597409/minifi_conan_build_output_jguzman_060524.txt)
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1718944100
##########
conanfile.py:
##########
@@ -0,0 +1,115 @@
+from conan import ConanFile
+from conan.tools.env import VirtualRunEnv
+from conan.tools.cmake import CMake, CMakeToolchain
+from conan.tools.files import collect_libs, copy
+import os
+
+required_conan_version = ">=2.0"
+
+shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6",
+ "catch2/3.5.4", "fmt/10.2.1", "spdlog/1.14.0")
+
+shared_sources = ("CMakeLists.txt", "libminifi/*", "extensions/*", "minifi_main/*", "nanofi/*",
+ "bin/*", "bootstrap/*", "cmake/*", "conf/*", "controller/*", "encrypt-config/*",
+ "etc/*", "examples/*", "msi/*", "thirdparty/*", "docker/*", "LICENSE", "NOTICE",
+ "README.md", "C2.md", "CONFIGURE.md", "CONTRIBUTING.md", "CONTROLLERS.md", "EXPRESSIONS.md",
+ "Extensions.md", "JNI.md", "METRICS.md", "OPS.md", "PROCESSORS.md", "ThirdParties.md",
+ "Windows.md", "aptitude.sh", "arch.sh", "bootstrap.sh", "bstrp_functions.sh", "centos.sh",
+ "CPPLINT.cfg", "darwin.sh", "debian.sh", "deploy.sh", "fedora.sh", "generateVersion.sh",
+ "linux.sh", "rheldistro.sh", "run_clang_tidy.sh", "run_clang_tidy.sh", "run_flake8.sh",
+ "run_shellcheck.sh", "suse.sh", "versioninfo.rc.in")
+
+class MiNiFiCppMain(ConanFile):
+ name = "minifi-cpp"
+ version = "0.99.1"
+ license = "Apache-2.0"
+ requires = shared_requires
+ settings = "os", "compiler", "build_type", "arch"
+ generators = "CMakeDeps"
+ options = {"shared": [True, False], "fPIC": [True, False]}
+
+ default_options = {"shared": False, "fPIC": True,}
+
+ exports_sources = shared_sources
+
+ def generate(self):
+ tc = CMakeToolchain(self)
+
+ tc.variables["MINIFI_LIBCURL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_OPENSSL_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_ZLIB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CIVETWEB_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_LIBXML2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_CATCH2_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_FMT_SOURCE"] = "CONAN"
+ tc.variables["MINIFI_SPDLOG_SOURCE"] = "CONAN"
+
+ tc.variables["SKIP_TESTS"] = "OFF"
+ tc.variables["ENABLE_OPENWSMAN"] = "ON"
+ tc.variables["ENABLE_CIVET"] = "ON"
+ tc.variables["ENABLE_EXPRESSION_LANGUAGE"] = "ON"
+ tc.variables["ENABLE_BZIP2"] = "ON"
+ tc.variables["ENABLE_ROCKSDB"] = "ON"
+ tc.variables["BUILD_ROCKSDB"] = "ON"
+
+ tc.variables["ENABLE_LIBARCHIVE"] = "OFF"
Review Comment:
I updated the conanfile.py, so for more features, we go with MiNiFi C++'s default settings and now only these particular features are DISABLED:
~~~python
tc.variables["ENABLE_LIBARCHIVE"] = "OFF"
tc.variables["ENABLE_AWS"] = "OFF"
tc.variables["ENABLE_SQL"] = "OFF"
tc.variables["ENABLE_GCP"] = "OFF"
tc.variables["ENABLE_LUA_SCRIPTING"] = "OFF"
~~~
In a follow up PR, I will add support for ENABLING them using conan while still providing support for standalone CMake.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1667607551
##########
conanfile.py:
##########
Review Comment:
I believe conan's equivalent to CMake's `make package` is leveraging conan's ConanFile class's **package(...)** method and making sure it comes with all the headers, binaries, etc that would be included when running `make package`. I will check MiNiFi's `make package` to verify that ConanFile class's **package(...)** creates a MiNiFi C++ conan package with the similar assets. For reference, here is my follow up PR that works toward this effort:
- "MINIFICPP-2346-P2" **PR-1813** Follow Up to "MINIFICPP-2346-P1": https://github.com/apache/nifi-minifi-cpp/pull/1813
- You'll notice a lot of changes in the diff, but that is because I haven't rebased on top of latest main branch, which I will do once we get this PR merged.
I can incorporate the ConanFile class's **package()** here to add the ability to run `conan create`, which is similar to `make package`. I will try to keep the changes to a minimum too.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2145872447
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1632118893
##########
conanfile.py:
##########
Review Comment:
@szaszm I updated MiNiFiOptions.cmake with 3 new options for instance MINIFI_LIBCURL_SOURCE and I am using MiNiFi options default ones for Enabled options. In the case of RocksDB, when building using the default conan profile, I run into an issue where it complains that I need to use at least gnu20. This issue happened because my default conan profile was using gnu17. This is why I was suggesting we use a common conan profile that is already defined in MiNiFi C++ that is already set to use gnu20. If I dont use the predefined conan profile for MiNiFi located in `etc/conan/profiles/release-linux`, then I get this following RocksDB error:
~~~bash
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-conan2/build_conan/thirdparty/rocksdb-src/include/rocksdb/options.h:400:8: error: defaulted ‘bool rocksdb::DbPath::operator==(const rocksdb::DbPath&) const’ only available with ‘-std=c++20’ or ‘-std=gnu++20’
400 | bool operator==(const DbPath& other) const = default;
| ^~~~~~~~
/home/ubuntu/src/james/pipeline/nifi-minifi-cpp-conan2/build_conan/thirdparty/rocksdb-src/include/rocksdb/options.h:1012:8: error: defaulted ‘bool rocksdb::DBOptions::operator==(const rocksdb::DBOptions&) const’ only available with ‘-std=c++20’ or ‘-std=gnu++20’
1012 | bool operator==(const DBOptions& other) const = default;
| ^~~~~~~~
~~~
Therefore, I suggest we use a common conan profile `etc/conan/profiles/release-linux` @szaszm
Heres the command, which is also at the beginning of my PR:
~~~bash
conan build . --build=missing --output-folder=build_conan -pr=etc/conan/profiles/release-linux
~~~
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2158893667
@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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2173825059
@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. Initially, I had probably close to **200** changes and after merging in upstream's main branch to my MINIFICPP-2346-P1 branch, the changes went up to **1,846** changes. Is this okay?
- 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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1737196067
##########
cmake/GetZLIB.cmake:
##########
Review Comment:
I just double checked the **conanfile.py**, it appears zlibs is not in conan package **shared_requires** tuple. I realized some other conan packages depended on **zlib**, so conan automatically downloaded **zlib 1.3.1** prebuilt conan package. I will add it explicitly. Here will be the updated **shared_requires** tuple
~~~bash
shared_requires = ("openssl/3.2.1", "libcurl/8.6.0", "civetweb/1.16", "libxml2/2.12.6", "fmt/10.2.1", "spdlog/1.14.0", "catch2/3.5.4", "zlib/1.3.1")
~~~
I will try **zlib/1.3.1** for conan package, which is different **zlib/1.2.11** system library version. We shouldn't see that missing headers from zlib during compilation anymore, but if we do, I will use zlib's CMake variables to provide the path to the needed headers.
I will double check by manually building MiNiFi with conan and get back to you, but for now, I updated the **conanfile.py** with **zlib** added to the **shared_requires** tuple and its on the repo now @szaszm
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2341744066
@lordgamez thanks for looking into the CURL external lib for MiNiFi C++ and comparing the difference between standalone CMake CURL built from source and the conan CURL prebuilt conan package. It is surprising that the conan CURL has a slower runtime than the standalone CMake CURL. I can look into it further this week and get back to you.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1609215848
##########
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:
I incorporated your feedback into commit [cf2face](https://github.com/apache/nifi-minifi-cpp/pull/1793/commits/cf2facee3a1ecc2909d439a4fe9cd1ca60830617)
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2317025229
> Hi @lordgamez @szaszm
>
> I fixed the LoggerTests fmt formatting issue "[7def7a0](https://github.com/apache/nifi-minifi-cpp/pull/1793/commits/7def7a081390fa991302e5d4882a0eff494dbfa3)":
>
> > 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
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on code in PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#discussion_r1733645535
##########
libminifi/test/unit/LoggerTests.cpp:
##########
@@ -380,7 +380,9 @@ TEST_CASE("Setting max log entry length to unlimited results in unlimited log en
TEST_CASE("fmt formatting works with the logger") {
LogTestController::getInstance().setTrace<logging::Logger>();
std::shared_ptr<logging::Logger> logger = logging::LoggerFactory<logging::Logger>::getLogger();
- logger->log_critical("{} equals to {}", 1min, 60s);
+ std::chrono::minutes duration{1};
Review Comment:
Made this update for compatibility between fmt 10.1.0 that is installed by standalone CMake approach and fmt 10.2.1 that is installed by conan approach. I verified after building MiNiFi using both approaches that the LoggerTest passes @szaszm @martinzink @lordgamez
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2346-P1: Integrated Conan2 for OpenSSL, CURL, ZLIB & Build MiNiFi [nifi-minifi-cpp]
Posted by "james94 (via GitHub)" <gi...@apache.org>.
james94 commented on PR #1793:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1793#issuecomment-2318664776
@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:
~~~bash
"-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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org