MINIFICPP-2768 use gcc-toolset-14 on rocky#2155
MINIFICPP-2768 use gcc-toolset-14 on rocky#2155martinzink wants to merge 4 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates MiNiFi C++ compiler/toolchain support by switching to C++23 std::expected and newer compiler baselines (gcc 13+, clang 17+), including Rocky Linux gcc-toolset-14, and removing legacy gcc12/expected-lite workarounds.
Changes:
- Replace
nonstd::expected(expected-lite) usages with C++23std::expectedandstd::unexpectedacross APIs, core, extensions, and tests. - Remove expected-lite + old GCC/coroutines workarounds from CMake, NOTICE/LICENSE, and CI/build images.
- Bump documented/CI compiler baselines (gcc 13, clang 17) and update Rocky Linux build container to gcc-toolset-14.
Reviewed changes
Copilot reviewed 121 out of 121 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| minifi-api/include/minifi-cpp/core/controller/ControllerServiceContext.h | Switch ControllerServiceContext API to std::expected. |
| minifi-api/include/minifi-cpp/core/Property.h | Switch Property expected-returning APIs to std::expected. |
| minifi-api/include/minifi-cpp/core/ProcessSession.h | Switch rollbackNoThrow API to std::expected. |
| minifi-api/include/minifi-cpp/core/ProcessContext.h | Switch ProcessContext/ProcessorInfo property APIs to std::expected. |
| minifi-api/include/minifi-cpp/core/ConfigurableComponent.h | Switch ConfigurableComponent property APIs to std::expected. |
| minifi-api/include/minifi-cpp/controllers/RecordSetReader.h | Switch RecordSetReader read() to std::expected. |
| libminifi/test/unit/ParsingUtilsTests.cpp | Update tests from nonstd::make_unexpected to std::unexpected. |
| libminifi/test/unit/OptionalTest.cpp | Update optional-to-expected tests to std::expected. |
| libminifi/test/unit/ExpectedTest.cpp | Update monadic-ops tests to std::expected/std::unexpect. |
| libminifi/src/utils/file/AssetManager.cpp | Switch sync() return type + fetch callback to std::expected. |
| libminifi/src/core/state/nodes/ResponseNodeLoader.cpp | Switch loader factory API to std::expected. |
| libminifi/src/core/ProcessSession.cpp | Switch rollbackNoThrow implementation to std::expected. |
| libminifi/src/core/ProcessContextImpl.cpp | Switch ProcessContextImpl property methods to std::expected. |
| libminifi/src/c2/FlowStatusBuilder.cpp | Switch builder methods + error handling to std::expected. |
| libminifi/src/c2/C2Utils.cpp | Switch debug-bundle creation to std::expected. |
| libminifi/src/c2/C2Agent.cpp | Switch asset/debug bundle helpers to std::expected. |
| libminifi/src/SchedulingAgent.cpp | Switch scheduling trigger APIs to std::expected. |
| libminifi/src/FlowController.cpp | Switch applyConfiguration/applyUpdate to std::expected. |
| libminifi/include/utils/file/AssetManager.h | Update AssetManager sync() signature to std::expected. |
| libminifi/include/core/state/nodes/ResponseNodeLoader.h | Update ResponseNodeLoaderImpl API to std::expected. |
| libminifi/include/core/state/UpdateController.h | Update StateMonitor applyUpdate() API to std::expected. |
| libminifi/include/core/flow/Node.h | Replace nonstd expected include with <expected>. |
| libminifi/include/core/controller/ControllerService.h | Switch ControllerServiceContextImpl to std::expected. |
| libminifi/include/core/ProcessSession.h | Update ProcessSessionImpl rollbackNoThrow() to std::expected. |
| libminifi/include/core/ProcessContextImpl.h | Update ProcessContextImpl property APIs to std::expected. |
| libminifi/include/c2/FlowStatusBuilder.h | Update FlowStatusBuilder APIs to std::expected. |
| libminifi/include/c2/C2Utils.h | Update createDebugBundleArchive() to std::expected. |
| libminifi/include/SchedulingAgent.h | Update SchedulingAgent APIs to std::expected. |
| libminifi/include/FlowController.h | Update FlowController update/apply APIs to std::expected. |
| libminifi/CMakeLists.txt | Remove expected-lite + old stdc++fs conditional linkage. |
| extensions/windows-event-log/wel/WindowsEventLog.h | Switch provider API to std::expected. |
| extensions/windows-event-log/wel/WindowsEventLog.cpp | Switch error returns to std::unexpected. |
| extensions/windows-event-log/wel/Bookmark.h | Switch bookmark XML generation API to std::expected. |
| extensions/windows-event-log/wel/Bookmark.cpp | Switch error returns to std::unexpected. |
| extensions/windows-event-log/ConsumeWindowsEventLog.h | Switch processing/render APIs to std::expected. |
| extensions/windows-event-log/ConsumeWindowsEventLog.cpp | Switch error returns to std::unexpected. |
| extensions/standard-processors/utils/JoltUtils.h | Switch parsing/processing APIs to std::expected. |
| extensions/standard-processors/utils/JoltUtils.cpp | Switch parsing/processing error returns to std::unexpected. |
| extensions/standard-processors/tests/integration/InvokeHTTPTests.cpp | Update tests to construct std::unexpected(...). |
| extensions/standard-processors/tests/CMakeLists.txt | Remove legacy coroutines enablement. |
| extensions/standard-processors/processors/SplitText.cpp | Switch internal fragment parsing to std::expected. |
| extensions/standard-processors/processors/SplitRecord.h | Switch readRecordsPerSplit() to std::expected. |
| extensions/standard-processors/processors/SplitRecord.cpp | Add <expected>, switch code to std::expected. |
| extensions/standard-processors/processors/PutUDP.cpp | Switch hostname resolve/send helpers to std::expected. |
| extensions/standard-processors/processors/InvokeHTTP.h | Switch parseDataTransferSpeed() to std::expected. |
| extensions/standard-processors/processors/InvokeHTTP.cpp | Switch helper + parsing pipeline to std::expected. |
| extensions/standard-processors/processors/ConvertRecord.cpp | Add <expected>, switch to std::expected. |
| extensions/standard-processors/modbus/ReadModbusFunctions.h | Switch response parsing APIs to std::expected. |
| extensions/standard-processors/modbus/ReadModbusFunctions.cpp | Switch error returns to std::unexpected. |
| extensions/standard-processors/modbus/FetchModbusTcp.h | Switch coroutine results to std::expected. |
| extensions/standard-processors/modbus/FetchModbusTcp.cpp | Switch coroutine errors to std::unexpected. |
| extensions/standard-processors/controllers/XMLReader.h | Switch reader API to std::expected. |
| extensions/standard-processors/controllers/XMLReader.cpp | Switch error returns to std::unexpected. |
| extensions/standard-processors/controllers/JsonTreeReader.h | Switch reader API to std::expected. |
| extensions/standard-processors/controllers/JsonTreeReader.cpp | Switch parsing + remove GCC12 workaround pragmas. |
| extensions/standard-processors/CMakeLists.txt | Remove legacy coroutines enablement. |
| extensions/sql/CMakeLists.txt | Use system ODBC on Apple as well as Windows. |
| extensions/smb/tests/utils/TempSmbShare.h | Switch factory API to std::expected. |
| extensions/smb/tests/utils/MockSmbConnectionControllerService.h | Switch test helper to std::expected. |
| extensions/smb/SmbConnectionControllerService.h | Switch connect/disconnect to std::expected. |
| extensions/smb/SmbConnectionControllerService.cpp | Switch error returns to std::unexpected. |
| extensions/rocksdb-repos/FlowFileLoader.cpp | Update comment references from nonstd to std expected. |
| extensions/python/types/PyRecordSetReader.cpp | Switch bridge code to std::expected. |
| extensions/opc/src/putopc.cpp | Switch configureTargetNode() to std::expected. |
| extensions/opc/include/putopc.h | Switch configureTargetNode() declaration to std::expected. |
| extensions/mqtt/processors/PublishMQTT.cpp | Switch record_set variable to std::expected. |
| extensions/llamacpp/tests/RunLlamaCppInferenceTests.cpp | Switch mock generate() to std::expected. |
| extensions/llamacpp/processors/LlamaContext.h | Switch interface to std::expected. |
| extensions/llamacpp/processors/DefaultLlamaContext.h | Switch interface to std::expected. |
| extensions/llamacpp/processors/DefaultLlamaContext.cpp | Switch errors to std::unexpected. |
| extensions/kubernetes/MetricsFilter.h | Switch filter() to std::expected. |
| extensions/kubernetes/MetricsFilter.cpp | Switch parse/validation errors to std::unexpected. |
| extensions/kubernetes/MetricsApi.h | Switch podMetricsList() to std::expected. |
| extensions/kubernetes/MetricsApi.cpp | Switch error returns to std::unexpected. |
| extensions/grafana-loki/PushGrafanaLokiREST.h | Switch submitRequest() to std::expected. |
| extensions/grafana-loki/PushGrafanaLokiREST.cpp | Switch HTTP failure returns to std::unexpected. |
| extensions/grafana-loki/PushGrafanaLokiGrpc.h | Switch submitRequest() to std::expected. |
| extensions/grafana-loki/PushGrafanaLokiGrpc.cpp | Switch gRPC failure returns to std::unexpected. |
| extensions/grafana-loki/PushGrafanaLoki.h | Switch virtual submitRequest() to std::expected. |
| extensions/elasticsearch/PostElasticsearch.cpp | Switch request/parse helpers to std::expected. |
| extensions/couchbase/tests/MockCouchbaseClusterService.h | Switch mock service APIs to std::expected. |
| extensions/couchbase/controllerservices/CouchbaseClusterService.h | Switch Couchbase client/service APIs to std::expected. |
| extensions/couchbase/controllerservices/CouchbaseClusterService.cpp | Switch errors to std::unexpected. |
| extensions/civetweb/processors/ListenHTTP.h | Switch promise payload type to std::expected. |
| extensions/aws/processors/PutKinesisStream.h | Switch batch result types to std::expected. |
| extensions/aws/processors/PutKinesisStream.cpp | Switch failures to std::unexpected. |
| extension-framework/cpp-extension-lib/src/core/ProcessContext.cpp | Switch C extension wrapper to std::expected. |
| extension-framework/cpp-extension-lib/include/api/core/ProcessContext.h | Include <expected> + switch API to std::expected. |
| extension-framework/CMakeLists.txt | Remove expected-lite + old stdc++fs conditional linkage. |
| docker/rockylinux/Dockerfile | Use gcc-toolset-14 and drop gcc12 header patching. |
| core-framework/src/utils/net/DNS.cpp | Switch DNS helpers to std::expected. |
| core-framework/src/utils/file/FileUtils.cpp | Switch Windows file time helper to std::expected. |
| core-framework/src/utils/Cron.cpp | Adjust case-insensitive parsing workaround logic/comments. |
| core-framework/src/core/Property.cpp | Switch Property impl methods to std::expected. |
| core-framework/src/core/ConfigurableComponentImpl.cpp | Switch property accessors to std::expected. |
| core-framework/include/utils/net/DNS.h | Add <expected> + switch APIs to std::expected. |
| core-framework/include/utils/file/PathUtils.h | Switch validation helper to std::expected. |
| core-framework/include/utils/file/FileUtils.h | Switch helpers to std::expected. |
| core-framework/include/core/controller/ControllerServiceBase.h | Switch property helper wrappers to std::expected. |
| core-framework/include/core/ConfigurableComponentImpl.h | Switch interface to std::expected. |
| core-framework/common/src/utils/StringUtils.cpp | Switch parseCharacter() to std::expected. |
| core-framework/common/src/utils/ParsingUtils.cpp | Switch parsing APIs to std::expected. |
| core-framework/common/include/utils/expected.h | Replace nonstd expected utilities with std::expected and update fmt formatter. |
| core-framework/common/include/utils/detail/MonadicOperationWrappers.h | Update docs mentioning std::expected. |
| core-framework/common/include/utils/StringUtils.h | Switch parseCharacter/parseNumber to std::expected. |
| core-framework/common/include/utils/ParsingUtils.h | Switch parsing API declarations/templates to std::expected. |
| core-framework/common/include/utils/OptionalUtils.h | Include <expected> and switch optional->expected pipe operator. |
| core-framework/common/CMakeLists.txt | Remove expected-lite linkage/conditionals. |
| core-framework/CMakeLists.txt | Remove expected-lite linkage/conditionals. |
| controller/tests/ControllerTests.cpp | Update test sink to std::expected. |
| controller/Controller.h | Switch getDebugBundle() to std::expected. |
| controller/Controller.cpp | Switch getDebugBundle() errors to std::unexpected. |
| cmake/ExpectedLite.cmake | Remove expected-lite FetchContent module. |
| cmake/CppVersion.cmake | Require -std=c++23 support and fail fast when missing. |
| cmake/Coroutines.cmake | Remove legacy coroutine-flag helper. |
| README.md | Update documented minimum compiler versions. |
| NOTICE | Remove expected-lite notice entry. |
| LICENSE | Remove expected-lite bundled license stanza. |
| CMakeLists.txt | Remove ExpectedLite + coroutines includes and old GCC restrict workaround. |
| .github/workflows/compiler-support.yml | Update gcc/clang matrix to gcc-13 and clang-17 minimums. |
| .github/workflows/ci.yml | Move macOS CI to macos-15 and add unixODBC dependency install. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| template <typename T, typename E> | ||
| concept ExpectedTypesDoNotConflict = | ||
| (!HasStdExpected<T, E> || | ||
| !std::same_as<nonstd::expected<T, E>, std::expected<T, E>>); | ||
| !std::same_as<std::expected<T, E>, std::expected<T, E>>); |
There was a problem hiding this comment.
ExpectedTypesDoNotConflict is currently always false whenever HasStdExpected<T, E> is true, because std::same_as<std::expected<T, E>, std::expected<T, E>> is always true. That disables the fmt::formatter<std::expected<...>> specialization, so formatting std::expected will either fail to compile or silently lose the intended formatting behavior. Fix by removing/rewriting this conflict-avoidance concept (e.g., guard the specialization based on whether {fmt} already provides a std::expected formatter, or key off {fmt} version feature macros) so the formatter is actually enabled when needed.
| (std::is_void_v<T> || fmt::is_formattable<T, Char>::value) && | ||
| fmt::is_formattable<E, Char>::value | ||
| struct fmt::formatter<nonstd::expected<T, E>, Char> { | ||
| struct fmt::formatter<std::expected<T, E>, Char> { |
There was a problem hiding this comment.
ExpectedTypesDoNotConflict is currently always false whenever HasStdExpected<T, E> is true, because std::same_as<std::expected<T, E>, std::expected<T, E>> is always true. That disables the fmt::formatter<std::expected<...>> specialization, so formatting std::expected will either fail to compile or silently lose the intended formatting behavior. Fix by removing/rewriting this conflict-avoidance concept (e.g., guard the specialization based on whether {fmt} already provides a std::expected formatter, or key off {fmt} version feature macros) so the formatter is actually enabled when needed.
| // Due to libstdc++ bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78714 | ||
| // the month parsing with '%b' and the weekday parsing with '%a' can be case-sensitive. | ||
| // Normalizing to Title Case prevents parsing failures across all GCC versions. | ||
| std::stringstream getCaseInsensitiveCStream(const std::string& str) { | ||
| #if defined(__GNUC__) && (__GNUC__ < 13) | ||
| #if defined(__GNUC__) && !defined(__clang__) | ||
| auto patched_str = string::toLower(str); | ||
| if (!patched_str.empty()) | ||
| if (!patched_str.empty()) { | ||
| patched_str[0] = static_cast<char>(std::toupper(static_cast<unsigned char>(patched_str[0]))); | ||
| } |
There was a problem hiding this comment.
The comment describes a libstdc++ behavior, but the preprocessor condition excludes Clang (!defined(__clang__)). Clang builds that use libstdc++ can still hit the same parsing behavior, so this change can reintroduce parsing failures in clang+libstdc++ environments. Consider keying the workaround on libstdc++ (e.g., __GLIBCXX__ / _GLIBCXX_RELEASE checks) rather than the compiler, or otherwise ensure the normalization applies whenever the underlying stdlib exhibits the behavior.
compiler support changes: gcc 13, clang 17 removed: expected-lite and other gcc12 workarounds
| // This has been fixed in gcc13 | ||
| // Due to libstdc++ bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78714 | ||
| // the month parsing with '%b' and the weekday parsing with '%a' can be case-sensitive. | ||
| // Normalizing to Title Case prevents parsing failures across all GCC versions. |
There was a problem hiding this comment.
If you want to drop GCC < 13, then we can drop the workaround too.
| chown -R ${USER}:${USER} ${MINIFI_BASE_DIR} | ||
|
|
||
| # Patching standard header to avoid https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105651 | ||
| RUN patch -p1 /opt/rh/gcc-toolset-12/root/usr/include/c++/12/bits/basic_string.tcc ${MINIFI_BASE_DIR}/thirdparty/libstdc++/avoid_bogus_Wrestrict_PR105651.patch |
| } // namespace | ||
|
|
||
| nonstd::expected<uint64_t, std::error_code> invoke_http::parseDataTransferSpeed(const std::string_view input) { | ||
| std::expected<uint64_t, std::error_code> invoke_http::parseDataTransferSpeed(const std::string_view input) { |
There was a problem hiding this comment.
monadic operations are standard now, so we should replace utils::andThen with .and_then too
There was a problem hiding this comment.
good point however those rewrites can be not straightforward, so not gonna pollute this PR with it
| const auto property = getProcessorInfo().getSupportedProperty(name); | ||
| if (!property) { | ||
| return nonstd::make_unexpected(PropertyErrorCode::NotSupportedProperty); | ||
| return std::unexpected(PropertyErrorCode::NotSupportedProperty); |
There was a problem hiding this comment.
I would use braced init with std::unexpected to make it clear that this is a constructor call, and for the other benefits of direct-list-initialization, e.g. no implicit narrowing. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax
There was a problem hiding this comment.
the monadic operations are standard, we don't need these utilities or their tests anymore
There was a problem hiding this comment.
yeah i rather do that in a followup PR, it might have been a bad idea to purge expected lite in the firstplace with these compiler chagnes
4e03de6 to
621dbe6
Compare
extensions/sql/CMakeLists.txt
Outdated
| add_minifi_library(minifi-sql SHARED ${SOURCES}) | ||
|
|
||
| if(WIN32) | ||
| # Modern CMake: Target-specific includes instead of global include_directories() |
There was a problem hiding this comment.
I don't think this needs a comment, it's expected knowledge of people working with cmake nowadays that legacy global and directory-based commands are to be avoided.
extensions/sql/CMakeLists.txt
Outdated
| SYSTEM PRIVATE | ||
| "../../thirdparty/rapidjson-1.1.0/include/" | ||
| "../../thirdparty/rapidjson-1.1.0/include/rapidjson" |
There was a problem hiding this comment.
instead of adding the dependency include dirs explicitly, the proper approach would be using target_link_libraries against the rapidjson target
There was a problem hiding this comment.
the monadic operations have been standardized, so our implementations can be removed
621dbe6 to
eb2d8ee
Compare
59ac29c to
a421cf4
Compare
compiler support changes: gcc 13, clang 17
removed: expected-lite and other gcc12 workarounds
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.