You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by benqiu2016 <gi...@git.apache.org> on 2017/03/09 20:01:22 UTC
[GitHub] nifi-minifi-cpp pull request #66: Ios
GitHub user benqiu2016 opened a pull request:
https://github.com/apache/nifi-minifi-cpp/pull/66
Ios
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 MINIFI-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/benqiu2016/nifi-minifi-cpp ios
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi-cpp/pull/66.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 #66
----
commit 5e11c4e26d3b35b5024266ecb56eafe479a379d6
Author: Bin Qiu <be...@gmail.com>
Date: 2017-03-09T19:46:19Z
MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SUPPORT/LEVELDB_SUPPORT into cmake
commit 34aabbd6f38ecfcbe4bd12ed0d0179d43f662796
Author: Bin Qiu <be...@gmail.com>
Date: 2017-03-09T19:51:09Z
MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SUPPORT/LEVELDB_SUPPORT into cmake
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp issue #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SU...
Posted by benqiu2016 <gi...@git.apache.org>.
Github user benqiu2016 commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/66
i think we may need to revisit the way to do MARCO in the later stage.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp issue #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SU...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/66
Instead of commenting on every macro, perhaps an alternative is to, at build time, include files that are appropriate to your unit. If you want YAML, have a configuration class that's only built as a result of Yaml. This may require having a different directory or including specific files but that's better than the definitions here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp pull request #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/...
Posted by benqiu2016 <gi...@git.apache.org>.
Github user benqiu2016 commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/66#discussion_r105460203
--- Diff: libminifi/src/io/TLSSocket.cpp ---
@@ -20,23 +20,29 @@
#include "Configure.h"
--- End diff --
same as above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp pull request #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/...
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/66#discussion_r105394608
--- Diff: libminifi/src/io/TLSSocket.cpp ---
@@ -20,23 +20,29 @@
#include "Configure.h"
--- End diff --
The entire class should be excluded.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp pull request #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/...
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/66#discussion_r105258693
--- Diff: libminifi/include/io/TLSSocket.h ---
@@ -24,8 +24,10 @@
#include <mutex>
--- End diff --
Can't we just exclude this entire class from compilation without OpenSSL?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp issue #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SU...
Posted by benqiu2016 <gi...@git.apache.org>.
Github user benqiu2016 commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/66
Instead of commenting on every macro, perhaps an alternative is to, at build time, include files that are appropriate to your unit. If you want YAML, have a configuration class that's only built as a result of Yaml. This may require having a different directory or including specific files but that's better than the definitions here.
Above is ideal, still we need to have some #define in the caller to indicate which one to call or we need to have two set of files, one is just stub if the feature was disable, one has the feature dependent stuff.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp issue #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SU...
Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/66
Here's an idea. We could include the files as a result. You could make it so that you have only one or two macros that define, perhaps, template metaprogramming. Look into SFINAE. At the very least you could have only a handful of macro definitions instead of the numerous that exist here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp pull request #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/...
Posted by benqiu2016 <gi...@git.apache.org>.
Github user benqiu2016 closed the pull request at:
https://github.com/apache/nifi-minifi-cpp/pull/66
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp issue #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/YAML_SU...
Posted by bqiust <gi...@git.apache.org>.
Github user bqiust commented on the issue:
https://github.com/apache/nifi-minifi-cpp/pull/66
@apiri @phrocker any feedback? i updated the above comments. Thanks. Bin
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi-cpp pull request #66: MINIFI-215: IOS port, add OPENSSL_SUPPORT/...
Posted by benqiu2016 <gi...@git.apache.org>.
Github user benqiu2016 commented on a diff in the pull request:
https://github.com/apache/nifi-minifi-cpp/pull/66#discussion_r105460135
--- Diff: libminifi/include/io/TLSSocket.h ---
@@ -24,8 +24,10 @@
#include <mutex>
--- End diff --
it is difficult because we need to add more # define in other files. I tried that before
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---