You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "adamdebreceni (via GitHub)" <gi...@apache.org> on 2023/08/02 10:55:35 UTC

[GitHub] [nifi-minifi-cpp] adamdebreceni opened a new pull request, #1623: MINIFICPP-2116 - Support GCC13

adamdebreceni opened a new pull request, #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623

   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] adamdebreceni commented on a diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1289592041


##########
cmake/BundledAzureSdkCpp.cmake:
##########
@@ -55,14 +59,17 @@ function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
 
     set(AZURE_SDK_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
         -DWARNINGS_AS_ERRORS=OFF
-        -DCMAKE_INSTALL_PREFIX=${INSTALL_DIR})
+        -DCMAKE_INSTALL_PREFIX=${INSTALL_DIR}
+        -DDISABLE_AZURE_CORE_OPENTELEMETRY=ON
+        -DBUILD_TRANSPORT_CURL=ON
+        -DCMAKE_CXX_FLAGS=-DCURL_STATICLIB)

Review Comment:
   moved this directive to the curl target



-- 
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 #1623: MINIFICPP-2116 - Support GCC13

Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1286725730


##########
thirdparty/libwebsockets/enum-int.patch:
##########


Review Comment:
   I don't think this is an openssl implementation simply the usage side, but @lordgamez might be able to shed some more light on it (as the author of `libwebsockets/openssl3.patch`)



-- 
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 diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1281790474


##########
cmake/GoogleCloudCpp.cmake:
##########
@@ -66,3 +66,5 @@ if (WIN32)
 else()
     target_compile_options(google_cloud_cpp_storage PUBLIC -Wno-error=deprecated-declarations)
 endif()
+
+target_compile_options(google_cloud_cpp_rest_internal PUBLIC -Wno-error)

Review Comment:
   I think this should go in a similar platform check as above, since `-Wno-error` is not a valid MSVC flag.



##########
thirdparty/libwebsockets/enum-int.patch:
##########


Review Comment:
   Can we patch libwebsockets to use our own OpenSSL build?



##########
.github/workflows/gcc13-compat.yml:
##########
@@ -0,0 +1,46 @@
+name: "MiNiFi-CPP verify gcc13 compatibility"
+on: [workflow_dispatch]
+jobs:
+  ubuntu_22_04:
+    name: "ubuntu-22.04-gcc-13"
+    runs-on: ubuntu-22.04
+    timeout-minutes: 120
+    steps:
+      - id: checkout
+        uses: actions/checkout@v3
+      - id: install_deps
+        run: |
+          sudo apt update
+          sudo apt install -y ccache libfl-dev libpcap-dev libboost-all-dev openjdk-8-jdk maven libusb-1.0-0-dev libpng-dev libgps-dev libsqliteodbc lua5.3 liblua5.3-dev flake8 parallel
+          echo "PATH=/usr/lib/ccache:$PATH" >> $GITHUB_ENV
+          echo -e "127.0.0.1\t$HOSTNAME" | sudo tee -a /etc/hosts > /dev/null
+      - name: build
+        run: |
+          eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"
+          brew install gcc@13

Review Comment:
   You can use the Toolchain test PPA to install GCC 13: https://launchpad.net/~ubuntu-toolchain-r/+archive/ubuntu/test



-- 
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 diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1288655860


##########
cmake/BundledAzureSdkCpp.cmake:
##########
@@ -55,14 +59,17 @@ function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
 
     set(AZURE_SDK_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
         -DWARNINGS_AS_ERRORS=OFF
-        -DCMAKE_INSTALL_PREFIX=${INSTALL_DIR})
+        -DCMAKE_INSTALL_PREFIX=${INSTALL_DIR}
+        -DDISABLE_AZURE_CORE_OPENTELEMETRY=ON
+        -DBUILD_TRANSPORT_CURL=ON
+        -DCMAKE_CXX_FLAGS=-DCURL_STATICLIB)

Review Comment:
   Would it work if we used `target_compile_definitions` instead, or does that not get passed down? Also, do we not need this definition when including Azure SDK headers?



-- 
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 #1623: MINIFICPP-2116 - Support GCC13

Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1286717477


##########
thirdparty/libwebsockets/enum-int.patch:
##########


Review Comment:
   removed patch and disabled werror



-- 
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 #1623: MINIFICPP-2116 - Support GCC13

Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1286792619


##########
thirdparty/libwebsockets/enum-int.patch:
##########


Review Comment:
   The libwebsockets is using our openssl build. The goal of the `libwebsockets/openssl3.patch` is to not use the `CHECK_FUNCTION_EXISTS` functions for checking the availability of specific openssl functions as they try to find them on the system. As we want to use our own lib instead of the system provided one, we simply set those openssl functions to be available because they are provided by our openssl build.
   
   I thought the reason for the `thirdparty/libwebsockets/enum-int.patch` was to avoid a warning only occurring on GCC13 that's why I suggested disabling `Werror` on this third-party library.



-- 
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 #1623: MINIFICPP-2116 - Support GCC13

Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1284386490


##########
thirdparty/libwebsockets/enum-int.patch:
##########


Review Comment:
   Is this issue gcc13 specific or some other warning due to the type difference? If it's a gcc13 specific error we could include it in the gcc13 patch if it's a warning can we turn off the werror in the 3rd party lib?



##########
thirdparty/azure-sdk-cpp/gcc13.patch:
##########


Review Comment:
   Could we upstream this to the azure sdk as a community contribution?



-- 
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 diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1281760440


##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -535,9 +535,9 @@ void ListSFTP::listByTrackingTimestamps(
       /* Determine the minimum reliable timestamp based on precision */
       auto minimum_reliable_timestamp = now - listing_lag;
       if (remote_system_timestamp_precision == TARGET_SYSTEM_TIMESTAMP_PRECISION_SECONDS) {
-        std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);
+        minimum_reliable_timestamp = std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);

Review Comment:
   This seems like an actual bugfix :+1: , should we file a Jira for this? (and maybe even some tests(in the future)?)



-- 
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 pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#issuecomment-1669661119

   changed it to a cached variable (similar to the variables above this) which fixes the issue


-- 
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 #1623: MINIFICPP-2116 - Support GCC13

Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1286692993


##########
thirdparty/azure-sdk-cpp/gcc13.patch:
##########


Review Comment:
   opened a PR https://github.com/Azure/azure-sdk-for-cpp/pull/4857



-- 
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 diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1281760440


##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -535,9 +535,9 @@ void ListSFTP::listByTrackingTimestamps(
       /* Determine the minimum reliable timestamp based on precision */
       auto minimum_reliable_timestamp = now - listing_lag;
       if (remote_system_timestamp_precision == TARGET_SYSTEM_TIMESTAMP_PRECISION_SECONDS) {
-        std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);
+        minimum_reliable_timestamp = std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);

Review Comment:
   This seems like an actual bugfix :+1: , should we file a Jira for this? (and maybe even some 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 closed pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez closed pull request #1623: MINIFICPP-2116 - Support GCC13
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623


-- 
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 pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#issuecomment-1669498554

   After trying to merge this, I'm still getting the error:
   ```
   /home/szaszm/nifi-minifi-cpp-3/build-gcc-make/_deps/websockets-src/lib/tls/openssl/openssl-server.c:713:1: error: conflicting types for ‘lws_tls_server_abort_connection’ due to enum/integer mismatch; have ‘int(struct lws *)’ [-Werror=enum-int-mismatch]
     713 | lws_tls_server_abort_connection(struct lws *wsi)
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from /home/szaszm/nifi-minifi-cpp-3/build-gcc-make/_deps/websockets-src/lib/tls/./private-lib-tls.h:143,
                    from /home/szaszm/nifi-minifi-cpp-3/build-gcc-make/_deps/websockets-src/lib/core/./private-lib-core.h:265,
                    from /home/szaszm/nifi-minifi-cpp-3/build-gcc-make/_deps/websockets-src/lib/tls/openssl/openssl-server.c:25:
   /home/szaszm/nifi-minifi-cpp-3/build-gcc-make/_deps/websockets-src/lib/tls/./private-network.h:171:1: note: previous declaration of ‘lws_tls_server_abort_connection’ with type ‘enum lws_ssl_capable_status(struct lws *)’
     171 | lws_tls_server_abort_connection(struct lws *wsi);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors
   ```


-- 
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 diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1281765596


##########
.github/workflows/gcc13-compat.yml:
##########
@@ -0,0 +1,54 @@
+name: "MiNiFi-CPP verify gcc13 compatibility"
+on: [workflow_dispatch]
+env:
+  DOCKER_CMAKE_FLAGS: -DDOCKER_VERIFY_THREAD=3 -DUSE_SHARED_LIBS= -DSTRICT_GSL_CHECKS=AUDIT -DCI_BUILD=ON -DDISABLE_JEMALLOC=ON -DENABLE_AWS=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_MQTT=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON \
+    -DENABLE_SPLUNK=ON -DENABLE_GCP=ON -DENABLE_OPC=ON -DENABLE_PYTHON_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON -DENABLE_KUBERNETES=ON -DENABLE_TEST_PROCESSORS=ON -DENABLE_PROMETHEUS=ON \
+    -DENABLE_ELASTICSEARCH=OFF -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache
+  SCCACHE_GHA_ENABLE: true
+jobs:
+  ubuntu_22_04:
+    name: "ubuntu-22.04-gcc-13"
+    runs-on: ubuntu-22.04
+    timeout-minutes: 120
+    steps:
+      - id: checkout
+        uses: actions/checkout@v3
+      - id: install_deps
+        run: |
+          sudo apt update
+          sudo apt install -y ccache libfl-dev libpcap-dev libboost-all-dev openjdk-8-jdk maven libusb-1.0-0-dev libpng-dev libgps-dev libsqliteodbc lua5.3 liblua5.3-dev flake8 parallel
+          echo "PATH=/usr/lib/ccache:$PATH" >> $GITHUB_ENV
+          echo -e "127.0.0.1\t$HOSTNAME" | sudo tee -a /etc/hosts > /dev/null
+      - name: build
+        run: |
+          eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"
+          brew install gcc@13
+          export CC=gcc-13
+          export CXX=g++-13
+          ./bootstrap.sh -e -t
+          cd build
+          cmake -DUSE_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Release -DCI_BUILD=ON -DSTRICT_GSL_CHECKS=AUDIT -DFAIL_ON_WARNINGS=ON -DENABLE_AWS=ON -DENABLE_AZURE=ON -DENABLE_BUSTACHE=ON -DENABLE_COAP=ON \

Review Comment:
   I reckon we wont run this too frequently and #1603 is merged, we could replace this long list with a simple 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] adamdebreceni commented on a diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1281761250


##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -535,9 +535,9 @@ void ListSFTP::listByTrackingTimestamps(
       /* Determine the minimum reliable timestamp based on precision */
       auto minimum_reliable_timestamp = now - listing_lag;
       if (remote_system_timestamp_precision == TARGET_SYSTEM_TIMESTAMP_PRECISION_SECONDS) {
-        std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);
+        minimum_reliable_timestamp = std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);

Review Comment:
   create a jira for it: https://issues.apache.org/jira/browse/MINIFICPP-2176



##########
extensions/sftp/processors/ListSFTP.cpp:
##########
@@ -535,9 +535,9 @@ void ListSFTP::listByTrackingTimestamps(
       /* Determine the minimum reliable timestamp based on precision */
       auto minimum_reliable_timestamp = now - listing_lag;
       if (remote_system_timestamp_precision == TARGET_SYSTEM_TIMESTAMP_PRECISION_SECONDS) {
-        std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);
+        minimum_reliable_timestamp = std::chrono::floor<std::chrono::seconds>(minimum_reliable_timestamp);

Review Comment:
   created a jira for it: https://issues.apache.org/jira/browse/MINIFICPP-2176



-- 
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 diff in pull request #1623: MINIFICPP-2116 - Support GCC13

Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1623:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1623#discussion_r1284906363


##########
thirdparty/libwebsockets/enum-int.patch:
##########


Review Comment:
   Why did it need to be patched then? If it's unused, it probably shouldn't be built.



-- 
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