You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/07/27 16:04:46 UTC
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #1138: MINIFICPP-1471 - Build extensions as dynamic libraries and dynamically load them
fgerlits commented on a change in pull request #1138:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1138#discussion_r677581930
##########
File path: extensions/coap/CMakeLists.txt
##########
@@ -26,7 +26,7 @@ file(GLOB CSOURCES "nanofi/*.c")
file(GLOB SOURCES "*.cpp" "protocols/*.cpp" "processors/*.cpp" "controllerservice/*.cpp" "server/*.cpp" )
add_library(nanofi-coap-c STATIC ${CSOURCES})
-add_library(minifi-coap STATIC ${SOURCES})
+add_library(minifi-coap SHARED ${SOURCES})
set_property(TARGET minifi-coap PROPERTY POSITION_INDEPENDENT_CODE ON)
Review comment:
I guess it doesn't matter, as the shared lib will be position-independent anyway, but you deleted this `set_property(... POSITION_INDEPENDENT_CODE ON)` line from the other extensions, so I would delete it from here, too
##########
File path: cmake/Extensions.cmake
##########
@@ -26,7 +26,27 @@ macro(register_extension extension-name)
get_property(extensions GLOBAL PROPERTY EXTENSION-OPTIONS)
set_property(GLOBAL APPEND PROPERTY EXTENSION-OPTIONS ${extension-name})
target_compile_definitions(${extension-name} PRIVATE "MODULE_NAME=${extension-name}")
- set_target_properties(${extension-name} PROPERTIES ENABLE_EXPORTS True)
+ set_target_properties(${extension-name} PROPERTIES
+ ENABLE_EXPORTS True
+ POSITION_INDEPENDENT_CODE ON)
+ if (WIN32)
+ set_target_properties(${extension-name} PROPERTIES
+ RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
+ ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
+ WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
+ else()
+ set_target_properties(${extension-name} PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin")
+ endif()
+
+ if (WIN32)
+ install(TARGETS ${extension-name} RUNTIME DESTINATION extensions COMPONENT bin)
+ else()
+ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+ target_link_options(${extension-name} PRIVATE "-Wl,--disable-new-dtags")
+ endif()
+ set_target_properties(${extension-name} PROPERTIES INSTALL_RPATH "$ORIGIN")
Review comment:
does this work? no escaping of the `$` is needed?
##########
File path: cmake/Extensions.cmake
##########
@@ -26,7 +26,27 @@ macro(register_extension extension-name)
get_property(extensions GLOBAL PROPERTY EXTENSION-OPTIONS)
set_property(GLOBAL APPEND PROPERTY EXTENSION-OPTIONS ${extension-name})
target_compile_definitions(${extension-name} PRIVATE "MODULE_NAME=${extension-name}")
- set_target_properties(${extension-name} PROPERTIES ENABLE_EXPORTS True)
+ set_target_properties(${extension-name} PROPERTIES
+ ENABLE_EXPORTS True
+ POSITION_INDEPENDENT_CODE ON)
+ if (WIN32)
+ set_target_properties(${extension-name} PROPERTIES
+ RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
+ ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin"
+ WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
+ else()
+ set_target_properties(${extension-name} PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin")
+ endif()
+
+ if (WIN32)
+ install(TARGETS ${extension-name} RUNTIME DESTINATION extensions COMPONENT bin)
+ else()
+ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+ target_link_options(${extension-name} PRIVATE "-Wl,--disable-new-dtags")
Review comment:
why do we need `--disable-new-dtags`?
##########
File path: extensions/script/CMakeLists.txt
##########
@@ -68,13 +58,10 @@ if (ENABLE_LUA_SCRIPTING)
add_definitions(-DLUA_SUPPORT)
file(GLOB LUA_SOURCES "lua/*.cpp")
- add_library(minifi-lua-extensions STATIC ${LUA_SOURCES})
-
- target_link_libraries(minifi-lua-extensions ${LIBMINIFI})
- target_link_libraries(minifi-lua-extensions ${LUA_LIBRARIES} sol)
- target_link_libraries(minifi-script-extensions minifi-lua-extensions)
+ target_sources(minifi-script-extensions PRIVATE ${LUA_SOURCES})
- target_compile_features(minifi-lua-extensions PUBLIC cxx_std_14)
+ target_link_libraries(minifi-script-extensions ${LUA_LIBRARIES} sol)
+ target_compile_features(minifi-script-extensions PUBLIC cxx_std_14)
Review comment:
These used to be in a separate `minifi-lua-extensions` library, now they are part of `minifi-script-extensions`. Why?
##########
File path: libminifi/include/core/extension/Extension.h
##########
@@ -0,0 +1,90 @@
+/**
+ * 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.
+ */
+
+#pragma once
+
+#include <memory>
+#include <vector>
+#include <string>
+
+#include "properties/Configure.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace extension {
+
+class Extension;
+
+using ExtensionConfig = std::shared_ptr<org::apache::nifi::minifi::Configure>;
+using ExtensionInit = bool(*)(Extension*, const ExtensionConfig&);
+
+class ExtensionInitializer;
+
+class Extension {
+ friend class ExtensionInitializer;
+ public:
+ explicit Extension(std::string name, ExtensionInit init);
+ virtual ~Extension();
+
+ bool initialize(const ExtensionConfig& config) {
+ return init_(this, config);
+ }
+
+ const std::string& getName() const {
+ return name_;
+ }
+
+ protected:
Review comment:
It's a bit weird to make these two protected, and then undo this with the `friend` declaration. Why not just make them public?
##########
File path: extensions/opencv/MotionDetector.cpp
##########
@@ -201,6 +201,8 @@ void MotionDetector::onTrigger(const std::shared_ptr<core::ProcessContext> &cont
void MotionDetector::notifyStop() {
}
+REGISTER_RESOURCE(MotionDetector, "Detect motion from captured images."); // NOLINT
Review comment:
why is the `// NOLINT` needed?
##########
File path: libminifi/include/core/extension/Extension.h
##########
@@ -0,0 +1,90 @@
+/**
+ * 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.
+ */
+
+#pragma once
+
+#include <memory>
+#include <vector>
+#include <string>
+
+#include "properties/Configure.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace extension {
+
+class Extension;
+
+using ExtensionConfig = std::shared_ptr<org::apache::nifi::minifi::Configure>;
+using ExtensionInit = bool(*)(Extension*, const ExtensionConfig&);
+
+class ExtensionInitializer;
+
+class Extension {
+ friend class ExtensionInitializer;
+ public:
+ explicit Extension(std::string name, ExtensionInit init);
+ virtual ~Extension();
+
+ bool initialize(const ExtensionConfig& config) {
+ return init_(this, config);
+ }
+
+ const std::string& getName() const {
+ return name_;
+ }
+
+ protected:
+ virtual bool doInitialize(const ExtensionConfig& /*config*/) {
Review comment:
I find `initialize` / `doInitialize` confusing, as it's not clear from their names what the difference is. I don't have a great suggestion for better names; maybe keep `initialize` and rename `doInitialize` and `doDeinitialize` to `initializeClass` and `deinitializeClass`?
##########
File path: extensions/libarchive/ManipulateArchive.h
##########
@@ -27,6 +27,9 @@
#include "core/Processor.h"
#include "core/ProcessSession.h"
+#include "FocusArchiveEntry.h"
+#include "UnfocusArchiveEntry.h"
Review comment:
could these two includes be in the `cpp` file?
--
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