You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2023/07/06 09:59:12 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
lordgamez opened a new pull request, #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603
https://issues.apache.org/jira/browse/MINIFICPP-2098
--------------
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:
- [ ] 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 GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1260891818
##########
extensions/kubernetes/CMakeLists.txt:
##########
@@ -14,8 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
-if (NOT (CMAKE_SYSTEM_NAME STREQUAL "Linux" AND ENABLE_KUBERNETES))
+if (NOT (CMAKE_SYSTEM_NAME STREQUAL "Linux" AND (ENABLE_ALL OR ENABLE_KUBERNETES)))
Review Comment:
Thanks, updated in latest push.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255377737
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
That could be confusing, removed in 24d0b4b9e848d2eb77ee7b4f3aa4e0b62130e45f
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255335310
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
do we want to add the test processors as well? if enabled, this extension is also added to the build artifact (the `register_extension` does not seem to discriminate), so either we should improve `register_extension` to not add this or hold off on enabling this with `ENABLE_ALL`
--
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
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1254230356
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
Review Comment:
Test processors can be used in both unit and docker tests. As SKIP_TESTS is always disabled in docker builds, it should not be relevant in docker tests. In unit tests there should not be an "AND" but an "OR" logical connection between skipping the tests and enabling the test processors, but that would break the test processor build in the docker tests. Because of this we should just skip checking the SKIP_TESTS flag altogether.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255350376
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
My thought was that these processors are only built if the `SKIP_TESTS` is `OFF`, which means we are building the tests that need this extension anyway. So if in this case we do not specifically set `ENABLE_TEST_PROCESSORS` because we have already set `ENABLE_ALL` to `ON` then we probably should build the test processors as they will be needed for building the tests.
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255369605
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
building a package with `ENABLE_ALL` will now include the test processors which could be confusing
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255361449
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
The problem with that is that these processors are also used in the docker tests where the SKIP_TESTS is always ON.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1260874176
##########
extensions/kubernetes/CMakeLists.txt:
##########
@@ -14,8 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
-if (NOT (CMAKE_SYSTEM_NAME STREQUAL "Linux" AND ENABLE_KUBERNETES))
+if (NOT (CMAKE_SYSTEM_NAME STREQUAL "Linux" AND (ENABLE_ALL OR ENABLE_KUBERNETES)))
Review Comment:
You added a `NOT OPENSSL_OFF` condition to this in #1583; please rebase and resolve the conflict.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1254230356
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
Review Comment:
Test processors can be used in both unit and docker tests. As SKIP_TESTS is always disabled in docker builds, it should not be relevant in docker tests. In unit tests there should not be an "AND" but an "OR" logical connection between skipping the tests and enabling the test processors, but that would break the test processor build in the docker tests. Because of this we should just skip checking the SKIP_TESTS flag altogether.
--
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255358477
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
we could tie it to just `SKIP_TESTS` as you mention this is needed anyway for the tests
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1603: MINIFICPP-2098 Add ENABLE_ALL for all extensions
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1603:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1603#discussion_r1255350376
##########
extensions/test-processors/CMakeLists.txt:
##########
@@ -17,7 +17,7 @@
# under the License.
#
-if (SKIP_TESTS AND NOT ENABLE_TEST_PROCESSORS)
+if (SKIP_TESTS AND NOT (ENABLE_ALL OR ENABLE_TEST_PROCESSORS))
Review Comment:
My thought was that as these processors are only built if the `SKIP_TESTS` is `OFF`, which means we are building the tests that need this extension anyway. If in this case we do not specifically set `ENABLE_TEST_PROCESSORS` because we have already set `ENABLE_ALL` to `ON` then we probably should build the test processors as they will be needed for building the tests.
--
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