You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by phrocker <gi...@git.apache.org> on 2018/10/29 18:55:32 UTC
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
GitHub user phrocker opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/430
MINIFICPP-659: Break out CAPI into nanofi
Break out structure -- facilitating Python and new nanofi structure
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 master)?
- [ ] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/phrocker/nifi-minifi-cpp MINIFICPP-659
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/430.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #430
----
commit 362fc9142a5d09ab6d8c4d84472dd43b9151a00e
Author: Marc Parisi <ph...@...>
Date: 2018-10-29T18:53:37Z
MINIFICPP-659: Break out CAPI into nanofi
Break out structure -- facilitating Python and new nanofi structure
----
---
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/430#discussion_r229112853
--- Diff: thirdparty/rocksdb/CMakeLists.txt ---
@@ -592,16 +592,16 @@ if(WIN32)
set(LIBS ${ROCKSDB_STATIC_LIB} ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
else()
set(SYSTEM_LIBS ${CMAKE_THREAD_LIBS_INIT})
- set(LIBS ${ROCKSDB_SHARED_LIB} ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
- add_library(${ROCKSDB_SHARED_LIB} SHARED ${SOURCES})
+ set(LIBS ${ROCKSDB_STATIC_LIB} ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
+ #add_library(${ROCKSDB_SHARED_LIB} SHARED ${SOURCES})
# target_link_libraries(${ROCKSDB_SHARED_LIB}
# ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
- set_target_properties(${ROCKSDB_SHARED_LIB} PROPERTIES
- LINKER_LANGUAGE CXX
- VERSION ${ROCKSDB_VERSION}
- SOVERSION ${ROCKSDB_VERSION_MAJOR}
- CXX_STANDARD 11
- OUTPUT_NAME "rocksdb")
+# set_target_properties(${ROCKSDB_SHARED_LIB} PROPERTIES
--- End diff --
Why commented?
If we comment it, even temporary, let's add some comment that justifies.
---
[GitHub] nifi-minifi-cpp issue #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/430
@apiri rebased but don't see the updates in the PR. Do you see them?
---
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/430#discussion_r229138685
--- Diff: thirdparty/rocksdb/CMakeLists.txt ---
@@ -592,16 +592,16 @@ if(WIN32)
set(LIBS ${ROCKSDB_STATIC_LIB} ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
else()
set(SYSTEM_LIBS ${CMAKE_THREAD_LIBS_INIT})
- set(LIBS ${ROCKSDB_SHARED_LIB} ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
- add_library(${ROCKSDB_SHARED_LIB} SHARED ${SOURCES})
+ set(LIBS ${ROCKSDB_STATIC_LIB} ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
+ #add_library(${ROCKSDB_SHARED_LIB} SHARED ${SOURCES})
# target_link_libraries(${ROCKSDB_SHARED_LIB}
# ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})
- set_target_properties(${ROCKSDB_SHARED_LIB} PROPERTIES
- LINKER_LANGUAGE CXX
- VERSION ${ROCKSDB_VERSION}
- SOVERSION ${ROCKSDB_VERSION_MAJOR}
- CXX_STANDARD 11
- OUTPUT_NAME "rocksdb")
+# set_target_properties(${ROCKSDB_SHARED_LIB} PROPERTIES
--- End diff --
My OSX machine is having some issues so I left this here and remove once OSX passes. Was hoping I could do a quick change but it seems that OSX is failing. It will be removed with a comment telling maintainers why the removal happened.
---
[GitHub] nifi-minifi-cpp issue #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/430
reviewing
---
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/430
---
[GitHub] nifi-minifi-cpp issue #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/430
@phrocker would you mind rebasing this? didn't want to make any bad assumptions between this and current master, otherwise looks good and build was fine
---
[GitHub] nifi-minifi-cpp issue #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/430
@apiri rebasing now, thanks.
---
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/430#discussion_r229111455
--- Diff: nanofi/examples/CMakeLists.txt ---
@@ -61,18 +61,18 @@ add_executable(generate_flow generate_flow.c)
add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(generate_flow capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(generate_flow nanofi ${CMAKE_THREAD_LIBS_INIT})
-target_link_libraries(terminate_handler capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(terminate_handler nanofi ${CMAKE_THREAD_LIBS_INIT})
if (NOT WIN32)
add_executable(transmit_flow transmit_flow.c)
-target_link_libraries(transmit_flow capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(transmit_flow nanofi ${CMAKE_THREAD_LIBS_INIT})
add_executable(monitor_directory monitor_directory.c)
-target_link_libraries(monitor_directory capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(monitor_directory nanofi ${CMAKE_THREAD_LIBS_INIT})
--- End diff --
Are you sure this is fine?
I see that nanofi is linked with whole-archive flag to make sure it contains all symbols from minifi, however when linking happens here and monitor_directory binary is created, the linker is still free to drop unused symbols.
Fix me, I would also be happy if we could skip doing that ugly flag-magic here. :)
---
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by arpadboda <gi...@git.apache.org>.
Github user arpadboda commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/430#discussion_r229235146
--- Diff: nanofi/examples/CMakeLists.txt ---
@@ -61,18 +61,18 @@ add_executable(generate_flow generate_flow.c)
add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(generate_flow capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(generate_flow nanofi ${CMAKE_THREAD_LIBS_INIT})
-target_link_libraries(terminate_handler capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(terminate_handler nanofi ${CMAKE_THREAD_LIBS_INIT})
if (NOT WIN32)
add_executable(transmit_flow transmit_flow.c)
-target_link_libraries(transmit_flow capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(transmit_flow nanofi ${CMAKE_THREAD_LIBS_INIT})
add_executable(monitor_directory monitor_directory.c)
-target_link_libraries(monitor_directory capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(monitor_directory nanofi ${CMAKE_THREAD_LIBS_INIT})
--- End diff --
Thanks, this looks good.
---
[GitHub] nifi-minifi-cpp pull request #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/430#discussion_r229138444
--- Diff: nanofi/examples/CMakeLists.txt ---
@@ -61,18 +61,18 @@ add_executable(generate_flow generate_flow.c)
add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(generate_flow capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(generate_flow nanofi ${CMAKE_THREAD_LIBS_INIT})
-target_link_libraries(terminate_handler capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(terminate_handler nanofi ${CMAKE_THREAD_LIBS_INIT})
if (NOT WIN32)
add_executable(transmit_flow transmit_flow.c)
-target_link_libraries(transmit_flow capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(transmit_flow nanofi ${CMAKE_THREAD_LIBS_INIT})
add_executable(monitor_directory monitor_directory.c)
-target_link_libraries(monitor_directory capi ${CMAKE_THREAD_LIBS_INIT} ${LINK_FLAGS} minifi ${LINK_END_FLAGS})
+target_link_libraries(monitor_directory nanofi ${CMAKE_THREAD_LIBS_INIT})
--- End diff --
Cmake implants those target libs in place. Therefore, I get
/usr/bin/c++ -std=c++11 -DOPENSSL_SUPPORT -std=c++11 -std=c++11 -rdynamic CMakeFiles/monitor_directory.dir/monitor_directory.c.o -o monitor_directory -Wl,-rpath,/home/marc/dev/nifi-minifi-cpp/build/thirdparty/uuid:/home/marc/dev/nifi-minifi-cpp/build/thirdparty/yaml-cpp-yaml-cpp-20171024 ../libnanofi.a -lpthread -Wl,--whole-archive ../../libminifi/libminifi.a ../../libminifi/libcore-minifi.a -ldl ../../thirdparty/uuid/libuuid.so ../../thirdparty/yaml-cpp-yaml-cpp-20171024/libyaml-cpp.so.0.5.3 ../../extensions/expression-language/noop/libminifi-expression-language-extensions.a -lz -lssl -lcrypto -Wl,--no-whole-archive
Even though I had the above build statement. This is because of the way CMAKE injects the targets, so there isn't the need for the extraneous target protections. On my system monitror_directory works appropriately after testing, but this does mean that MINIFICPP-660 should probably be changed to enable better testing these examples in docker images. I will take a look at that since I've already started 660
---
[GitHub] nifi-minifi-cpp issue #430: MINIFICPP-659: Break out CAPI into nanofi
Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/430
@phrocker it doesn't seem like I was unable to pull them... unfortunately, may just have to open up a new PR with that commit
---