-
Notifications
You must be signed in to change notification settings - Fork 95
MINIFICPP-2567 C++23 Support #1968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces updates for C++23 compatibility across MiNiFi C++ and its third‐party dependencies by updating patches, header inclusions, and build configurations. Key changes include:
- Updating third‐party patches (OpenCV, GoogleCloudCpp, Couchbase) to account for C++23 requirements.
- Removing the BoxedRecordField abstraction and using RecordField directly.
- Revising CMake and test files to use updated APIs and C++23 standard flags.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
thirdparty/opencv/c++23_fixes.patch | Update header includes for C++23 support. |
thirdparty/google-cloud-cpp/c++23_fixes.patch | Adjust JSON error parsing to extract string values. |
thirdparty/couchbase/c++23_fixes.patch | Revise header includes and remove now-unnecessary forward declarations. |
minifi-api/include/minifi-cpp/core/RecordField.h | Remove BoxedRecordField and change RecordObject to directly store RecordField. |
libminifi/test/unit/NetUtilsTest.cpp | Replace address parsing method with asio::ip::make_address. |
libminifi/test/libtest/unit/TestRecord.h | Update test records to use direct RecordField construction. |
libminifi/src/core/RecordField.cpp | Update JSON serialization/deserialization to remove BoxedRecordField usage. |
extensions/standard-processors/tests/unit/RecordSetTests.cpp | Update tests to use RecordField directly (note inconsistent namespace qualification). |
extensions/standard-processors/tests/unit/PutTCPTests.cpp | Change asynchronous post call to use asio::post with the io_context parameter. |
extensions/standard-processors/modbus/ByteConverters.h | Add missing include of to support C++23 compilation. |
extensions/standard-processors/controllers/JsonTreeReader.cpp | Update JSON parsing to use direct RecordField values. |
extensions/standard-processors/controllers/JsonRecordSetWriter.cpp | Adjust JSON serialization to use the new RecordField interface. |
cmake/RangeV3.cmake | Add compile definitions required for Range-V3 with C++23 support. |
cmake/LlamaCpp.cmake | Update LlamaCpp dependency to a newer version. |
cmake/GoogleCloudCpp.cmake | Add c++23_fixes.patch to the patching sequence. |
cmake/CppVersion.cmake | Upgrade C++ standard to C++23 and adjust compiler flag detection. |
cmake/Couchbase.cmake | Incorporate c++23_fixes.patch in Couchbase build configuration. |
cmake/BundledRocksDB.cmake | Update RocksDB version and patch for C++23 support. |
cmake/BundledOpenCV.cmake | Integrate c++23_fixes.patch into the OpenCV build configuration. |
cmake/Asio.cmake | Update the Asio dependency to a newer version that supports C++23. |
The readme needs to be updated to reflect the supported compiler versions, and changes in the build procedure if any |
I tried compiling the code with gcc 11.4.0 and it seems to have some build issues in our files and in thirdparties as well:
According to this gcc 11.4 is not the lowest required compiler version for our code. We should either define the correct lowest version that still compiles (enforcing it in CMake configuration time), or if it's possible patch the code to make it compile with gcc 11.4. Edit: I compiled the code with gcc 12.3.0 and that succeeded, we could update the requirements to that version. The clang requirement may be enough to be set to 16 instead of 17 as currently the CI clang job is using version 16 and it compiler successfully. Of course we can discuss if we want to set it higher depending on the language features we want to use in the future, but I would leave the requirements at gcc 12 and clang 16 for now. |
I'm running into more issues when building on Arch Linux with GCC 15.1, I think we should fix those too. Interestingly, one of them seems to be a compiler bug, I'm working on reporting it to GCC. Triggered by ListenHTTP::Handler::request_buffer_. https://godbolt.org/z/dvc68js8a A few fixes I made so far: 442705f |
FYI Release build of fmt library fails with the following issue due to a false positive warning Wstringop-overflow: fmtlib/fmt#4125 |
I've included @lordgamez 's clang20 fixes and @fgerlits's windows fixes. Thank you guys. I've also made a bunch of gcc15 modifications so please rereview it |
I've also added a new ci that checks the min and max supported gcc and clang versions. It successfully ran with ENABLE_ALL and MINIFI_FAIL_ON_WARNINGS |
libminifi/test/unit/ExpectedTest.cpp
Outdated
@@ -473,7 +473,7 @@ TEST_CASE("expected valueOrElse", "[expected][valueOrElse]") { | |||
REQUIRE(gsl::narrow<int>("hello"sv.size()) == (ex | utils::valueOrElse([](const std::string& err) { return gsl::narrow<int>(err.size()); }))); | |||
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](std::string){ throw std::exception(); }), std::exception); // NOLINT(performance-unnecessary-value-param) | |||
REQUIRE_THROWS_AS(ex | utils::valueOrElse([](const std::string&) -> int { throw std::exception(); }), std::exception); | |||
REQUIRE_THROWS_WITH(std::move(ex) | utils::valueOrElse([](std::string&& error) -> int { throw std::runtime_error(error); }), "hello"); | |||
REQUIRE_THROWS_WITH([&] { std::move(ex) | utils::valueOrElse([](const std::string& error) -> int { throw std::runtime_error(error); }); }(), "hello"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the string taken by const ref now? The point of the test is to ensure that when an expected is moved, the callback gets an rvalue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now i remember
/home/runner/work/nifi-minifi-cpp/nifi-minifi-cpp/libminifi/test/unit/ExpectedTest.cpp:479:78: error: rvalue reference parameter 'error' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved,-warnings-as-errors] 479 | REQUIRE_THROWS_MATCHES(std::move(ex) | utils::valueOrElse([](std::string&& error) -> int { throw std::runtime_error(error); }),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should turn off the linter for that line then. It needs to be rvalue reference for the test to make any sense.
Co-authored-by: Ferenc Gerlits <[email protected]>
Co-authored-by: Ferenc Gerlits <[email protected]>
Co-authored-by: Ferenc Gerlits <[email protected]>
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:
For documentation related changes:
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.