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 2020/12/04 10:50:09 UTC
[GitHub] [nifi-minifi-cpp] hunyadi-dev opened a new pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
hunyadi-dev opened a new pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950
Also, disable building duktape and lua support for civetweb.
Seems like civetweb had an issue using the available compiler flags. I opened a PR for them [here](https://github.com/civetweb/civetweb/pull/948), until they release a version with it, we can use the fix as a 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536135645
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb-cpp.${SUFFIX}"
Review comment:
yeah -- that makes me wonder if adding `-DCMAKE_INSTALL_LIBDIR=lib` (as is done in BundledLibRdKafka) would be a better solution
but I think either way is fine
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536091892
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb-cpp.${SUFFIX}"
Review comment:
I think you need to do the same on line 80, as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536020151
##########
File path: thirdparty/civetweb/civetweb.patch
##########
@@ -1,61 +1,45 @@
-diff --git a/CMakeLists.txt b/CMakeLists.txt
-index 2c08bd28..ccb4fd7f 100644
---- a/CMakeLists.txt
-+++ b/CMakeLists.txt
-@@ -101,7 +101,7 @@ message(STATUS "Lua CGI support - ${CIVETWEB_ENABLE_LUA}")
- # Enable installing CivetWeb executables
- option(CIVETWEB_INSTALL_EXECUTABLE "Enable installing CivetWeb executable" ON)
- mark_as_advanced(FORCE CIVETWEB_INSTALL_EXECUTABLE) # Advanced users can disable
--message(STATUS "Executable installation - ${CIVETWEB_INSTALL_EXECUTABLE}")
-+message(STATUS "Executable installation - ${CIVETWEB_INSTALL_EXECUTABLE}")
-
- # Allow builds to complete with warnings (do not set -Werror)
- # CivetWeb Linux support is stable:
-@@ -294,8 +294,8 @@ if (MINGW)
- endif()
- if (NOT CIVETWEB_ALLOW_WARNINGS)
- add_c_compiler_flag(-Werror)
-+ add_c_compiler_flag(/WX)
- endif()
--add_c_compiler_flag(/WX)
- add_c_compiler_flag(-pedantic-errors)
- add_c_compiler_flag(-fvisibility=hidden)
- add_c_compiler_flag(-fstack-protector-strong RELEASE)
-diff --git a/src/civetweb.c b/src/civetweb.c
-index c0ccbaa2..c692d1c8 100644
---- a/src/civetweb.c
-+++ b/src/civetweb.c
-@@ -14202,7 +14202,7 @@ ssl_get_protocol(int version_id)
- * https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_info_callback.html
- * https://linux.die.net/man/3/ssl_set_info_callback */
- static void
--ssl_info_callback(SSL *ssl, int what, int ret)
-+ssl_info_callback(const SSL *ssl, int what, int ret)
- {
- (void)ret;
-
-@@ -16237,10 +16237,13 @@ worker_thread_run(struct worker_thread_args *thread_args)
- mg_free(conn->request_info.client_cert);
- conn->request_info.client_cert = 0;
- }
-- }
-+ } else {
-+ /* make sure the connection is cleaned up on SSL failure */
-+ close_connection(conn);
-+ }
- #endif
-- } else {
-- /* process HTTP connection */
-+ } else {
-+ /* process HTTP connection */
- process_new_connection(conn);
- }
-
-@@ -17249,7 +17252,6 @@ mg_get_system_info_impl(char *buffer, int buflen)
- #pragma GCC diagnostic push
- /* Disable bogus compiler warning -Wdate-time */
- #pragma GCC diagnostic ignored "-Wall"
--#pragma GCC diagnostic ignored "-Werror"
- #endif
- mg_snprintf(NULL,
- NULL,
+From cf90b37da9432cf957f4f2e3ce837d6c73647802 Mon Sep 17 00:00:00 2001
+From: Adam Hunyadi <hu...@gmail.com>
+Date: Fri, 4 Dec 2020 11:26:12 +0100
+Subject: [PATCH] Fix checks for available compiler flags
+
+---
+ cmake/AddCCompilerFlag.cmake | 5 ++---
+ cmake/AddCXXCompilerFlag.cmake | 5 ++---
+ 2 files changed, 4 insertions(+), 6 deletions(-)
+
Review comment:
This was auto-generated by git. Will remove the redundant parts but I did not think this would be manually read.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#issuecomment-740009712
Changing this to the safer v1.12 in #952, closing this PR.
----------------------------------------------------------------
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.
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 #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536175275
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
Review comment:
[GNUInstallDirs](https://cmake.org/cmake/help/v3.0/module/GNUInstallDirs.html) should be included to use CMAKE_INSTALL_* variables on unix systems and CMAKE_INSTALL_LIBDIR should be set explicitly on Windows.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536093777
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb-cpp.${SUFFIX}"
Review comment:
Good catch!!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev closed pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
hunyadi-dev closed pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536093777
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb-cpp.${SUFFIX}"
Review comment:
Good catch! Also, line 90 and 98. :S
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb-cpp.${SUFFIX}"
Review comment:
Good catch! Also, on line 90 and 98. :S
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536135645
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb-cpp.${SUFFIX}"
Review comment:
yeah -- that makes me wonder if adding `-DCMAKE_INSTALL_LIBDIR=lib` (as is done in BundledLibRdKafka) would be a better solution
but I think either way is fine
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536243355
##########
File path: cmake/BundledCivetWeb.cmake
##########
@@ -30,8 +30,8 @@ function(use_bundled_civetweb SOURCE_DIR BINARY_DIR)
endif()
set(BYPRODUCTS
- "lib/${PREFIX}civetweb.${SUFFIX}"
- "lib/${PREFIX}civetweb-cpp.${SUFFIX}"
+ "${CMAKE_INSTALL_LIBDIR}/${PREFIX}civetweb.${SUFFIX}"
Review comment:
Updated, now GNUInstallDirs are included when not on windows and libdir is set accordingly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #950: MINIFICPP-1405 - Update civetweb version and correct build flags used by it
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #950:
URL: https://github.com/apache/nifi-minifi-cpp/pull/950#discussion_r536014970
##########
File path: thirdparty/civetweb/civetweb.patch
##########
@@ -1,61 +1,45 @@
-diff --git a/CMakeLists.txt b/CMakeLists.txt
-index 2c08bd28..ccb4fd7f 100644
---- a/CMakeLists.txt
-+++ b/CMakeLists.txt
-@@ -101,7 +101,7 @@ message(STATUS "Lua CGI support - ${CIVETWEB_ENABLE_LUA}")
- # Enable installing CivetWeb executables
- option(CIVETWEB_INSTALL_EXECUTABLE "Enable installing CivetWeb executable" ON)
- mark_as_advanced(FORCE CIVETWEB_INSTALL_EXECUTABLE) # Advanced users can disable
--message(STATUS "Executable installation - ${CIVETWEB_INSTALL_EXECUTABLE}")
-+message(STATUS "Executable installation - ${CIVETWEB_INSTALL_EXECUTABLE}")
-
- # Allow builds to complete with warnings (do not set -Werror)
- # CivetWeb Linux support is stable:
-@@ -294,8 +294,8 @@ if (MINGW)
- endif()
- if (NOT CIVETWEB_ALLOW_WARNINGS)
- add_c_compiler_flag(-Werror)
-+ add_c_compiler_flag(/WX)
- endif()
--add_c_compiler_flag(/WX)
- add_c_compiler_flag(-pedantic-errors)
- add_c_compiler_flag(-fvisibility=hidden)
- add_c_compiler_flag(-fstack-protector-strong RELEASE)
-diff --git a/src/civetweb.c b/src/civetweb.c
-index c0ccbaa2..c692d1c8 100644
---- a/src/civetweb.c
-+++ b/src/civetweb.c
-@@ -14202,7 +14202,7 @@ ssl_get_protocol(int version_id)
- * https://www.openssl.org/docs/man1.1.0/ssl/SSL_set_info_callback.html
- * https://linux.die.net/man/3/ssl_set_info_callback */
- static void
--ssl_info_callback(SSL *ssl, int what, int ret)
-+ssl_info_callback(const SSL *ssl, int what, int ret)
- {
- (void)ret;
-
-@@ -16237,10 +16237,13 @@ worker_thread_run(struct worker_thread_args *thread_args)
- mg_free(conn->request_info.client_cert);
- conn->request_info.client_cert = 0;
- }
-- }
-+ } else {
-+ /* make sure the connection is cleaned up on SSL failure */
-+ close_connection(conn);
-+ }
- #endif
-- } else {
-- /* process HTTP connection */
-+ } else {
-+ /* process HTTP connection */
- process_new_connection(conn);
- }
-
-@@ -17249,7 +17252,6 @@ mg_get_system_info_impl(char *buffer, int buflen)
- #pragma GCC diagnostic push
- /* Disable bogus compiler warning -Wdate-time */
- #pragma GCC diagnostic ignored "-Wall"
--#pragma GCC diagnostic ignored "-Werror"
- #endif
- mg_snprintf(NULL,
- NULL,
+From cf90b37da9432cf957f4f2e3ce837d6c73647802 Mon Sep 17 00:00:00 2001
+From: Adam Hunyadi <hu...@gmail.com>
+Date: Fri, 4 Dec 2020 11:26:12 +0100
+Subject: [PATCH] Fix checks for available compiler flags
+
+---
+ cmake/AddCCompilerFlag.cmake | 5 ++---
+ cmake/AddCXXCompilerFlag.cmake | 5 ++---
+ 2 files changed, 4 insertions(+), 6 deletions(-)
+
Review comment:
did you mean to include this?
also the 'index ...' lines and the 3 lines at the end are probably not needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org