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 2022/01/18 13:44:26 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
martinzink opened a new pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244
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] szaszm commented on a change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r787063121
##########
File path: docker/Dockerfile
##########
@@ -61,6 +61,7 @@ ARG DISABLE_LZMA=OFF
ARG DISABLE_BZIP2=OFF
ARG ENABLE_SCRIPTING=ON
Review comment:
I prefer disabled by default. We do the same in cmake because libpython is a problematic dependency for shipping binary packages. It changes soname every minor version, which breaks compatibility with any other version.
--
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 change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786951780
##########
File path: docker/Dockerfile
##########
@@ -61,6 +61,7 @@ ARG DISABLE_LZMA=OFF
ARG DISABLE_BZIP2=OFF
ARG ENABLE_SCRIPTING=ON
Review comment:
In that case as that is our released docker image I think we can disable it by default. It shouldn't have any impact and I don't think it should be included if not specified.
--
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] martinzink commented on a change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786920475
##########
File path: docker/Dockerfile
##########
@@ -61,6 +61,7 @@ ARG DISABLE_LZMA=OFF
ARG DISABLE_BZIP2=OFF
ARG ENABLE_SCRIPTING=ON
Review comment:
I am not sure about this, the minimal image explicitly disables this though
https://github.com/apache/nifi-minifi-cpp/blob/main/cmake/DockerConfig.cmake#L77-L78
--
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 change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786869766
##########
File path: docker/Dockerfile
##########
@@ -61,6 +61,7 @@ ARG DISABLE_LZMA=OFF
ARG DISABLE_BZIP2=OFF
ARG ENABLE_SCRIPTING=ON
Review comment:
Should we have scripting enabled by default? I just realized this and because the DISABLE_PYTHON_SCRIPTING flag is off by default, it automatically installs python packages which adds 80MBs to the image. I'm not sure if scripting should be enabled. (If it's needed by the users of the published minimal image then it should be enabled in that config)
--
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] martinzink commented on pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
martinzink commented on pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#issuecomment-1016375680
All changes have been incorporated into https://github.com/apache/nifi-minifi-cpp/pull/1232
--
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 commented on a change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r787063121
##########
File path: docker/Dockerfile
##########
@@ -61,6 +61,7 @@ ARG DISABLE_LZMA=OFF
ARG DISABLE_BZIP2=OFF
ARG ENABLE_SCRIPTING=ON
Review comment:
I prefer disabled by default. We do the same in cmake because libpython is a problematic dependency for shipping binary packages. It changes soname every minor version, which breaks compatibility with any other version.
It can also be considered a security risk in the sense that whoever has access to a C2 server can run arbitrary code on the agents.
--
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 change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786847285
##########
File path: docker/Dockerfile
##########
@@ -159,6 +163,7 @@ RUN addgroup -g ${GID} ${USER} && adduser -u ${UID} -D -G ${USER} -g "" ${USER}
if [ "$ENABLE_JNI" = "ON" ]; then apk add --no-cache openjdk8-jre-base; fi && \
if [ "$ENABLE_PCAP" = "ON" ]; then apk add --no-cache libpcap; fi && \
if [ "$ENABLE_USB_CAMERA" = "ON" ]; then apk add --no-cache libpng libusb; fi && \
+ if [ "$ENABLE_LUA_SCRIPTING" = "ON" ]; then apk add --no-cache lua; fi && \
Review comment:
I think we should also add a check for ENABLE_SCRIPTING flag here. Also just a thought but maybe we could also include the ENABLE_ALL check here as well for all of these optional packages.
##########
File path: docker/Dockerfile
##########
@@ -159,6 +163,7 @@ RUN addgroup -g ${GID} ${USER} && adduser -u ${UID} -D -G ${USER} -g "" ${USER}
if [ "$ENABLE_JNI" = "ON" ]; then apk add --no-cache openjdk8-jre-base; fi && \
if [ "$ENABLE_PCAP" = "ON" ]; then apk add --no-cache libpcap; fi && \
if [ "$ENABLE_USB_CAMERA" = "ON" ]; then apk add --no-cache libpng libusb; fi && \
+ if [ "$ENABLE_LUA_SCRIPTING" = "ON" ]; then apk add --no-cache lua; fi && \
if [ "$ENABLE_OPENCV" = "ON" ] || [ "$ENABLE_BUSTACHE" = "ON" ]; then apk add --no-cache boost; fi && \
if { [ "$ENABLE_PYTHON" = "ON" ] || [ "$ENABLE_SCRIPTING" = "ON" ]; } && [ -z "$DISABLE_PYTHON_SCRIPTING" ]; then apk add --no-cache python3-dev; fi
Review comment:
Not strictly related to this PR, but python3-dev should not be needed here, only the python3 package. Also ENABLE_PYTHON check can be removed here as it turns out that it's unrelated to the python scripting extension. (It's an unused nanofi based extension.)
--
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] martinzink commented on a change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786955587
##########
File path: docker/Dockerfile
##########
@@ -159,6 +163,7 @@ RUN addgroup -g ${GID} ${USER} && adduser -u ${UID} -D -G ${USER} -g "" ${USER}
if [ "$ENABLE_JNI" = "ON" ]; then apk add --no-cache openjdk8-jre-base; fi && \
if [ "$ENABLE_PCAP" = "ON" ]; then apk add --no-cache libpcap; fi && \
if [ "$ENABLE_USB_CAMERA" = "ON" ]; then apk add --no-cache libpng libusb; fi && \
+ if [ "$ENABLE_LUA_SCRIPTING" = "ON" ]; then apk add --no-cache lua; fi && \
if [ "$ENABLE_OPENCV" = "ON" ] || [ "$ENABLE_BUSTACHE" = "ON" ]; then apk add --no-cache boost; fi && \
if { [ "$ENABLE_PYTHON" = "ON" ] || [ "$ENABLE_SCRIPTING" = "ON" ]; } && [ -z "$DISABLE_PYTHON_SCRIPTING" ]; then apk add --no-cache python3-dev; fi
Review comment:
good idea changed it in https://github.com/apache/nifi-minifi-cpp/pull/1244/commits/c222bf84612ca4ff4bd85c6cf5dc79b03cbc1fed
--
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] martinzink commented on a change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786954815
##########
File path: docker/Dockerfile
##########
@@ -159,6 +163,7 @@ RUN addgroup -g ${GID} ${USER} && adduser -u ${UID} -D -G ${USER} -g "" ${USER}
if [ "$ENABLE_JNI" = "ON" ]; then apk add --no-cache openjdk8-jre-base; fi && \
if [ "$ENABLE_PCAP" = "ON" ]; then apk add --no-cache libpcap; fi && \
if [ "$ENABLE_USB_CAMERA" = "ON" ]; then apk add --no-cache libpng libusb; fi && \
+ if [ "$ENABLE_LUA_SCRIPTING" = "ON" ]; then apk add --no-cache lua; fi && \
Review comment:
I added the ENABLE_SCRIPTING check in https://github.com/apache/nifi-minifi-cpp/pull/1244/commits/c222bf84612ca4ff4bd85c6cf5dc79b03cbc1fed
i would skip the ENABLE_ALL for now because it is not that consistent (e.g. its not even considered when building the minifi-scripting library https://github.com/apache/nifi-minifi-cpp/blob/main/extensions/script/CMakeLists.txt#L47 )
I think we should refactor this whole ENABLE_ALL flag in a different ticket
--
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] martinzink commented on a change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r787494594
##########
File path: docker/Dockerfile
##########
@@ -61,6 +61,7 @@ ARG DISABLE_LZMA=OFF
ARG DISABLE_BZIP2=OFF
ARG ENABLE_SCRIPTING=ON
Review comment:
changed it in https://github.com/apache/nifi-minifi-cpp/pull/1244/commits/cf4fbe4c44e764f775278784a06ee2aef6893b84
--
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 change in pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244#discussion_r786959906
##########
File path: docker/Dockerfile
##########
@@ -159,6 +163,7 @@ RUN addgroup -g ${GID} ${USER} && adduser -u ${UID} -D -G ${USER} -g "" ${USER}
if [ "$ENABLE_JNI" = "ON" ]; then apk add --no-cache openjdk8-jre-base; fi && \
if [ "$ENABLE_PCAP" = "ON" ]; then apk add --no-cache libpcap; fi && \
if [ "$ENABLE_USB_CAMERA" = "ON" ]; then apk add --no-cache libpng libusb; fi && \
+ if [ "$ENABLE_LUA_SCRIPTING" = "ON" ]; then apk add --no-cache lua; fi && \
Review comment:
That's a good point, I agree.
--
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] martinzink closed pull request #1244: MINIFICPP-1727 Add ENABLE_LUA_SCRIPTING flag to docker
Posted by GitBox <gi...@apache.org>.
martinzink closed pull request #1244:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1244
--
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