Copilot commented on code in PR #2105:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2105#discussion_r2831891991
##########
cmake/Extensions.cmake:
##########
@@ -22,29 +22,14 @@ define_property(GLOBAL PROPERTY EXTENSION-OPTIONS
set_property(GLOBAL PROPERTY EXTENSION-OPTIONS "")
-set(extension-build-info-file
"${CMAKE_CURRENT_BINARY_DIR}/ExtensionBuildInfo.cpp")
-file(GENERATE OUTPUT ${extension-build-info-file}
- CONTENT "\
- #include \"minifi-cpp/utils/Export.h\"\n\
- #ifdef BUILD_ID_VARIABLE_NAME\n\
- EXTENSIONAPI extern const char* const BUILD_ID_VARIABLE_NAME =
\"__EXTENSION_BUILD_IDENTIFIER_BEGIN__${BUILD_IDENTIFIER}__EXTENSION_BUILD_IDENTIFIER_END__\";\n\
- #else\n\
- static_assert(false, \"BUILD_ID_VARIABLE_NAME is not defined\");\n\
- #endif\n")
-
-function(get_build_id_variable_name extension-name output)
- string(REPLACE "-" "_" result ${extension-name})
- string(APPEND result "_build_identifier")
- set("${output}" "${result}" PARENT_SCOPE)
-endfunction()
-
macro(register_extension extension-name extension-display-name extension-guard
description)
set(${extension-guard} ${extension-name} PARENT_SCOPE)
get_property(extensions GLOBAL PROPERTY EXTENSION-OPTIONS)
set_property(GLOBAL APPEND PROPERTY EXTENSION-OPTIONS ${extension-name})
- get_build_id_variable_name(${extension-name} build-id-variable-name)
- set_source_files_properties(${extension-build-info-file} PROPERTIES
GENERATED TRUE)
- target_sources(${extension-name} PRIVATE ${extension-build-info-file})
+ get_target_property(has_custom_initializer ${extension-name}
HAS_CUSTOM_INITIALIZER)
+ if (NOT has_custom_initializer)
+ target_sources(${extension-name} PRIVATE
${CMAKE_SOURCE_DIR}/extensions/ExtensionInitializer.cpp)
+ endif()
target_compile_definitions(${extension-name}
PRIVATE "MODULE_NAME=${extension-name}"
PRIVATE "BUILD_ID_VARIABLE_NAME=${build-id-variable-name}")
Review Comment:
The reference to `build-id-variable-name` on line 35 is problematic. This
variable is never defined after the removal of the `get_build_id_variable_name`
function. Since the BUILD_ID_VARIABLE_NAME compile definition is no longer used
in the new symbol-based approach for API compatibility checking, this entire
line (line 35) should be removed.
```suggestion
PRIVATE "MODULE_NAME=${extension-name}")
```
##########
Extensions.md:
##########
@@ -49,7 +82,7 @@ extern "C" MinifiExtension* InitExtension(MinifiConfig*
/*config*/) {
.processors_count = 0,
.processors_ptr = nullptr
};
- return
MinifiCreateExtension(minifi::utils::toStringView(MINIFI_API_VERSION),
&ext_create_info);
+ return MinifiCreateCppExtension(&ext_create_info);
Review Comment:
The function call should be
`minifi::utils::MinifiCreateCppExtension(&ext_create_info)` to be consistent
with the actual implementation. The `MinifiCreateCppExtension` function is in
the `minifi::utils` namespace (see
extension-framework/include/utils/ExtensionInitUtils.h:33-35), and all other
C++ extension loaders use the fully qualified name (see SFTPLoader.cpp:47,
PyProcLoader.cpp:52, PythonLibLoader.cpp:117, OpenCVLoader.cpp:45,
ExtensionInitializer.cpp:34).
```suggestion
return minifi::utils::MinifiCreateCppExtension(&ext_create_info);
```
##########
libminifi/src/core/extension/Extension.cpp:
##########
@@ -56,10 +57,43 @@ bool Extension::load(bool global) {
if (!handle_) {
logger_->log_error("Failed to load extension '{}' at '{}': {}", name_,
library_path_, dlerror());
return false;
- } else {
- logger_->log_trace("Loaded extension '{}' at '{}'", name_, library_path_);
+ }
+ logger_->log_trace("Dlopen succeeded for extension '{}' at '{}'", name_,
library_path_);
+ if (findSymbol("MinifiInitCppExtension")) {
+ logger_->log_trace("Loaded cpp extension '{}' at '{}'", name_,
library_path_);
return true;
}
+ if (!findSymbol("MinifiInitExtension")) {
+ logger_->log_error("Failed to load c extension '{}' at '{}': No
initializer found", name_, library_path_);
+ return false;
+ }
+ auto api_version = reinterpret_cast<const char*
const*>(findSymbol("MinifiApiVersion"));
+ if (!api_version) {
+ logger_->log_error("Failed to load c extension '{}' at '{}': No
MinifiApiVersion symbol found", name_, library_path_);
+ return false;
+ }
+ utils::SVMatch match;
+ if (!utils::regexSearch(*api_version, match,
utils::Regex{R"(^([0-9]+)\.([0-9]+)\.([0-9]+)$)"})) {
+ logger_->log_error("Failed to load c extension '{}' at '{}': Api version
is in invalid format: '{}'", name_, library_path_, *api_version);
+ return false;
+ }
+ gsl_Assert(match.size() == 4);
+ version_.major = std::stoi(match[1]);
+ version_.minor = std::stoi(match[2]);
+ version_.patch = std::stoi(match[3]);
+ if (version_.major != MINIFI_API_MAJOR_VERSION) {
+ logger_->log_error("Failed to load c extension '{}' at '{}': Api major
version mismatch, application is '{}' while extension is '{}'",
+ name_, library_path_, MINIFI_API_VERSION, *api_version);
+ return false;
+ }
+ if (version_.minor > MINIFI_API_MINOR_VERSION) {
+ logger_->log_error("Failed to load c extension '{}' at '{}': Extension is
built for a newer version, application is '{}' while extension is '{}'",
+ name_, library_path_, MINIFI_API_VERSION, *api_version);
+ return false;
+ }
+ logger_->log_debug("Loaded c extension '{}' at '{}': Application version is
'{}', extension version is '{}'",
+ name_, library_path_, MINIFI_API_VERSION, *api_version);
+ return true;
Review Comment:
The new symbol-based compatibility checking logic lacks test coverage. The
previous test file (ExtensionVerificationTests.cpp) was removed without
replacement tests. Consider adding tests that verify:
1. C++ extensions with matching BUILD_IDENTIFIER can be loaded
2. C++ extensions with non-matching BUILD_IDENTIFIER fail to load (symbol
resolution failure)
3. C extensions with compatible MinifiApiVersion can be loaded
4. C extensions with incompatible major version are rejected
5. C extensions with newer minor version are rejected
6. C extensions without MinifiApiVersion symbol are rejected
##########
Extensions.md:
##########
@@ -17,8 +17,41 @@
To enable all extensions for your platform, you may use -DENABLE_ALL=TRUE OR
select the option to "Enable all Extensions" in the bootstrap script.
[ReadMe](https://github.com/apache/nifi-minifi-cpp/#bootstrapping)
# Extension internals
-Extensions are dynamic libraries loaded at runtime by the agent. An extension
makes its
-capabilities (classes) available to the system through registrars.
Registration must happen in source files, not headers.
+Extensions are dynamic libraries loaded at runtime by the agent.
+
+## C extensions
+You can build a shared library depending on the C capabilities of the agent as
given in the `minifi-c.h` file.
+For the shared library to be considered a valid extension, it has to have a
global symbol with the name `MinifiCApiVersion`
Review Comment:
The documentation states the symbol should be named `MinifiCApiVersion`, but
the actual code in Extension.cpp line 70 looks for `MinifiApiVersion` (without
the "C"). The documentation should be corrected to match the implementation.
```suggestion
For the shared library to be considered a valid extension, it has to have a
global symbol with the name `MinifiApiVersion`
```
##########
extensions/python/pythonloader/PyProcLoader.cpp:
##########
@@ -49,5 +49,5 @@ extern "C" MinifiExtension* InitExtension(MinifiConfig*
config) {
.processors_count = 0,
.processors_ptr = nullptr
};
- return
MinifiCreateExtension(minifi::utils::toStringView(MINIFI_API_VERSION),
&ext_create_info);
+ return minifi::utils::MinifiCreateCppExtension( &ext_create_info);
Review Comment:
Extra space before the opening parenthesis in the function call. This should
be `MinifiCreateCppExtension(&ext_create_info)` without the space, consistent
with all other calls to this function in the codebase (see SFTPLoader.cpp:47,
PythonLibLoader.cpp:117, OpenCVLoader.cpp:45, ExtensionInitializer.cpp:34).
```suggestion
return minifi::utils::MinifiCreateCppExtension(&ext_create_info);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
