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 2021/06/08 13:09:20 UTC
[GitHub] [nifi-minifi-cpp] szaszm opened a new pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
szaszm opened a new pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104
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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] 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)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] 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.
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r648912740
##########
File path: nanofi/examples/CMakeLists.txt
##########
@@ -19,57 +19,35 @@
cmake_minimum_required(VERSION 3.16)
-include_directories(/include)
-
-include(CheckCXXCompilerFlag)
-if (WIN32)
- if ((MSVC_VERSION GREATER "1900") OR (MSVC_VERSION EQUAL "1900"))
- CHECK_CXX_COMPILER_FLAG("/std:c++14" _cpp_latest_flag_supported)
- if (_cpp_latest_flag_supported)
- add_compile_options("/std:c++14")
- endif()
- endif()
-else()
-CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
-CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X)
-if(COMPILER_SUPPORTS_CXX11)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
-elseif(COMPILER_SUPPORTS_CXX0X)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
-else()
- message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.")
-endif()
-
-endif()
+include(CppVersion)
+set_cpp_version()
if (NOT WIN32)
+ add_executable(generate_flow generate_flow.c)
-add_executable(generate_flow generate_flow.c)
-
-add_executable(terminate_handler terminate_handler.c)
-
-target_wholearchive_library(generate_flow minifi-http-curl)
-target_link_libraries(generate_flow nanofi Threads::Threads)
+ add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(terminate_handler nanofi Threads::Threads)
+ target_wholearchive_library(generate_flow minifi-http-curl)
+ target_link_libraries(generate_flow nanofi Threads::Threads)
-#add_executable(hash_file hash_file.c)
+ target_link_libraries(terminate_handler nanofi Threads::Threads)
-#arget_link_libraries(hash_file nanofi Threads::Threads )
+ #add_executable(hash_file hash_file.c)
-add_executable(transmit_flow transmit_flow.c)
+ #arget_link_libraries(hash_file nanofi Threads::Threads )
Review comment:
We could write a Jira ticket for it to investigate and fix the issue later.
--
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649327881
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,13 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x && (\"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ||\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")")
Review comment:
My version ("for each patch: check if the patch has been already applied, and if not, then apply it") makes more sense to me, but this is subjective. Having read it five more times, I can see now that your (or OSSPUUID's) version of "try to apply both patches, and if at least one of them fails, then check if both patches have been already applied at this point, and if not, then give an error" is logically the same, so I'm OK with either.
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649413324
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,13 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x && (\"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ||\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")")
Review comment:
I agree that your version might be easier to follow, so I've applied your suggestion. Thanks.
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649045495
##########
File path: nanofi/examples/CMakeLists.txt
##########
@@ -19,57 +19,35 @@
cmake_minimum_required(VERSION 3.16)
-include_directories(/include)
-
-include(CheckCXXCompilerFlag)
-if (WIN32)
- if ((MSVC_VERSION GREATER "1900") OR (MSVC_VERSION EQUAL "1900"))
- CHECK_CXX_COMPILER_FLAG("/std:c++14" _cpp_latest_flag_supported)
- if (_cpp_latest_flag_supported)
- add_compile_options("/std:c++14")
- endif()
- endif()
-else()
-CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
-CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X)
-if(COMPILER_SUPPORTS_CXX11)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
-elseif(COMPILER_SUPPORTS_CXX0X)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
-else()
- message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.")
-endif()
-
-endif()
+include(CppVersion)
+set_cpp_version()
if (NOT WIN32)
+ add_executable(generate_flow generate_flow.c)
-add_executable(generate_flow generate_flow.c)
-
-add_executable(terminate_handler terminate_handler.c)
-
-target_wholearchive_library(generate_flow minifi-http-curl)
-target_link_libraries(generate_flow nanofi Threads::Threads)
+ add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(terminate_handler nanofi Threads::Threads)
+ target_wholearchive_library(generate_flow minifi-http-curl)
+ target_link_libraries(generate_flow nanofi Threads::Threads)
-#add_executable(hash_file hash_file.c)
+ target_link_libraries(terminate_handler nanofi Threads::Threads)
-#arget_link_libraries(hash_file nanofi Threads::Threads )
+ #add_executable(hash_file hash_file.c)
-add_executable(transmit_flow transmit_flow.c)
+ #arget_link_libraries(hash_file nanofi Threads::Threads )
Review comment:
I don't really see the point of further pursuing this as long as nanofi is barely maintained.
This is where hash_file was introduced: #499
And this is where it was removed: #514 [MINIFICPP-767 Fix issue with hash](https://issues.apache.org/jira/browse/MINIFICPP-767)
--
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649470530
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,13 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x && (\"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ||\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")")
Review comment:
Having read it a sixth time, they're not logically the same: if patch1 has already been applied but patch2 hasn't, then
- the version in https://github.com/apache/nifi-minifi-cpp/pull/1104/commits/56ec54bc0626ed56648464ac35c99e289a2dcd51 will not try to apply patch2, and result in an error
- the version in https://github.com/apache/nifi-minifi-cpp/pull/1104/commits/6762d136fe4dbcd44296933587cf6d1e748d20ba will skip patch1 and apply patch2, and result in success.
So it's not just a readability improvement.
--
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649752419
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,12 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x &&\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\") &&\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ")
Review comment:
Sorry, slight f***p on my part -- there was an extra `")` in the middle.
```suggestion
(\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ")
```
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r648457266
##########
File path: nanofi/examples/CMakeLists.txt
##########
@@ -19,57 +19,35 @@
cmake_minimum_required(VERSION 3.16)
-include_directories(/include)
-
-include(CheckCXXCompilerFlag)
-if (WIN32)
- if ((MSVC_VERSION GREATER "1900") OR (MSVC_VERSION EQUAL "1900"))
- CHECK_CXX_COMPILER_FLAG("/std:c++14" _cpp_latest_flag_supported)
- if (_cpp_latest_flag_supported)
- add_compile_options("/std:c++14")
- endif()
- endif()
-else()
-CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
-CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X)
-if(COMPILER_SUPPORTS_CXX11)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
-elseif(COMPILER_SUPPORTS_CXX0X)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
-else()
- message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.")
-endif()
-
-endif()
+include(CppVersion)
+set_cpp_version()
if (NOT WIN32)
+ add_executable(generate_flow generate_flow.c)
-add_executable(generate_flow generate_flow.c)
-
-add_executable(terminate_handler terminate_handler.c)
-
-target_wholearchive_library(generate_flow minifi-http-curl)
-target_link_libraries(generate_flow nanofi Threads::Threads)
+ add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(terminate_handler nanofi Threads::Threads)
+ target_wholearchive_library(generate_flow minifi-http-curl)
+ target_link_libraries(generate_flow nanofi Threads::Threads)
-#add_executable(hash_file hash_file.c)
+ target_link_libraries(terminate_handler nanofi Threads::Threads)
-#arget_link_libraries(hash_file nanofi Threads::Threads )
+ #add_executable(hash_file hash_file.c)
-add_executable(transmit_flow transmit_flow.c)
+ #arget_link_libraries(hash_file nanofi Threads::Threads )
Review comment:
I don't know the reason, just fixed the indentation here after changing the compiler flags above.
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r648459053
##########
File path: nanofi/examples/CMakeLists.txt
##########
@@ -19,57 +19,35 @@
cmake_minimum_required(VERSION 3.16)
-include_directories(/include)
-
-include(CheckCXXCompilerFlag)
-if (WIN32)
- if ((MSVC_VERSION GREATER "1900") OR (MSVC_VERSION EQUAL "1900"))
- CHECK_CXX_COMPILER_FLAG("/std:c++14" _cpp_latest_flag_supported)
- if (_cpp_latest_flag_supported)
- add_compile_options("/std:c++14")
- endif()
- endif()
-else()
-CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
-CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X)
-if(COMPILER_SUPPORTS_CXX11)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
-elseif(COMPILER_SUPPORTS_CXX0X)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
-else()
- message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.")
-endif()
-
-endif()
+include(CppVersion)
+set_cpp_version()
if (NOT WIN32)
+ add_executable(generate_flow generate_flow.c)
-add_executable(generate_flow generate_flow.c)
-
-add_executable(terminate_handler terminate_handler.c)
-
-target_wholearchive_library(generate_flow minifi-http-curl)
-target_link_libraries(generate_flow nanofi Threads::Threads)
+ add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(terminate_handler nanofi Threads::Threads)
+ target_wholearchive_library(generate_flow minifi-http-curl)
+ target_link_libraries(generate_flow nanofi Threads::Threads)
-#add_executable(hash_file hash_file.c)
+ target_link_libraries(terminate_handler nanofi Threads::Threads)
-#arget_link_libraries(hash_file nanofi Threads::Threads )
+ #add_executable(hash_file hash_file.c)
-add_executable(transmit_flow transmit_flow.c)
+ #arget_link_libraries(hash_file nanofi Threads::Threads )
Review comment:
It looks like the whole executable is commented out for whatever reason.
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649297953
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,13 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x && (\"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ||\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")")
Review comment:
I copied this from OSSPUUID. The end result is the same, but if you think this is better for some reason, I can apply the change.
--
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r648451862
##########
File path: nanofi/examples/CMakeLists.txt
##########
@@ -19,57 +19,35 @@
cmake_minimum_required(VERSION 3.16)
-include_directories(/include)
-
-include(CheckCXXCompilerFlag)
-if (WIN32)
- if ((MSVC_VERSION GREATER "1900") OR (MSVC_VERSION EQUAL "1900"))
- CHECK_CXX_COMPILER_FLAG("/std:c++14" _cpp_latest_flag_supported)
- if (_cpp_latest_flag_supported)
- add_compile_options("/std:c++14")
- endif()
- endif()
-else()
-CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
-CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X)
-if(COMPILER_SUPPORTS_CXX11)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
-elseif(COMPILER_SUPPORTS_CXX0X)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
-else()
- message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.")
-endif()
-
-endif()
+include(CppVersion)
+set_cpp_version()
if (NOT WIN32)
+ add_executable(generate_flow generate_flow.c)
-add_executable(generate_flow generate_flow.c)
-
-add_executable(terminate_handler terminate_handler.c)
-
-target_wholearchive_library(generate_flow minifi-http-curl)
-target_link_libraries(generate_flow nanofi Threads::Threads)
+ add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(terminate_handler nanofi Threads::Threads)
+ target_wholearchive_library(generate_flow minifi-http-curl)
+ target_link_libraries(generate_flow nanofi Threads::Threads)
-#add_executable(hash_file hash_file.c)
+ target_link_libraries(terminate_handler nanofi Threads::Threads)
-#arget_link_libraries(hash_file nanofi Threads::Threads )
+ #add_executable(hash_file hash_file.c)
-add_executable(transmit_flow transmit_flow.c)
+ #arget_link_libraries(hash_file nanofi Threads::Threads )
Review comment:
(old code) Can we add a comment why this is commented out?
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649412583
##########
File path: .github/workflows/ci.yml
##########
@@ -154,7 +111,7 @@ jobs:
run: |
PATH %PATH%;C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64
PATH %PATH%;C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\Roslyn
- win_build_vs.bat build /2019 /64 /CI /S /A /PDH /K /L /R
+ win_build_vs.bat build /64 /CI /S /A /PDH /K /L /R
Review comment:
Thanks, fixed in 86e3a86
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649413324
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,13 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x && (\"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ||\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")")
Review comment:
I agree that your version might be easier to follow, so changed it. Thanks.
--
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] arpadboda commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r655095282
##########
File path: .gitignore
##########
@@ -54,6 +54,7 @@ target
thirdparty/**/*.o
thirdparty/**/*.a
libminifi/test/**/*.a
+libminifi/test/corecomponentstate/
Review comment:
👍
--
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] arpadboda closed pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104
--
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] szaszm commented on a change in pull request #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649843969
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,12 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x &&\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\") &&\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ")
Review comment:
No worries. I didn't notice either. :) Thanks for reacting quickly.
--
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649049403
##########
File path: nanofi/examples/CMakeLists.txt
##########
@@ -19,57 +19,35 @@
cmake_minimum_required(VERSION 3.16)
-include_directories(/include)
-
-include(CheckCXXCompilerFlag)
-if (WIN32)
- if ((MSVC_VERSION GREATER "1900") OR (MSVC_VERSION EQUAL "1900"))
- CHECK_CXX_COMPILER_FLAG("/std:c++14" _cpp_latest_flag_supported)
- if (_cpp_latest_flag_supported)
- add_compile_options("/std:c++14")
- endif()
- endif()
-else()
-CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
-CHECK_CXX_COMPILER_FLAG("-std=c++0x" COMPILER_SUPPORTS_CXX0X)
-if(COMPILER_SUPPORTS_CXX11)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
-elseif(COMPILER_SUPPORTS_CXX0X)
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
-else()
- message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++11 support. Please use a different C++ compiler.")
-endif()
-
-endif()
+include(CppVersion)
+set_cpp_version()
if (NOT WIN32)
+ add_executable(generate_flow generate_flow.c)
-add_executable(generate_flow generate_flow.c)
-
-add_executable(terminate_handler terminate_handler.c)
-
-target_wholearchive_library(generate_flow minifi-http-curl)
-target_link_libraries(generate_flow nanofi Threads::Threads)
+ add_executable(terminate_handler terminate_handler.c)
-target_link_libraries(terminate_handler nanofi Threads::Threads)
+ target_wholearchive_library(generate_flow minifi-http-curl)
+ target_link_libraries(generate_flow nanofi Threads::Threads)
-#add_executable(hash_file hash_file.c)
+ target_link_libraries(terminate_handler nanofi Threads::Threads)
-#arget_link_libraries(hash_file nanofi Threads::Threads )
+ #add_executable(hash_file hash_file.c)
-add_executable(transmit_flow transmit_flow.c)
+ #arget_link_libraries(hash_file nanofi Threads::Threads )
Review comment:
Yeah, that's okay then, thanks!
--
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 #1104: MINIFICPP-1426 MSVC: Use /std:c++latest /permissive- and drop VS 2017
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #1104:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1104#discussion_r649261591
##########
File path: .github/workflows/ci.yml
##########
@@ -154,7 +111,7 @@ jobs:
run: |
PATH %PATH%;C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64
PATH %PATH%;C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\Roslyn
- win_build_vs.bat build /2019 /64 /CI /S /A /PDH /K /L /R
+ win_build_vs.bat build /64 /CI /S /A /PDH /K /L /R
Review comment:
`/Z` (Azure) was only enabled in the other Windows build; if we'll only have this one, we should enable it here.
##########
File path: cmake/BundledAzureSdkCpp.cmake
##########
@@ -16,8 +16,13 @@
# under the License.
function(use_bundled_libazure SOURCE_DIR BINARY_DIR)
- set(PATCH_FILE "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
- set(PC "${Patch_EXECUTABLE}" -R -p1 -s -f --dry-run -i "${PATCH_FILE}" || "${Patch_EXECUTABLE}" -p1 -i "${PATCH_FILE}")
+ set(PATCH_FILE1 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/azure-sdk-for-cpp-old-compiler.patch")
+ set(PATCH_FILE2 "${SOURCE_DIR}/thirdparty/azure-sdk-cpp-for-cpp/fix-illegal-qualified-name-in-member.patch")
+ set(PC bash -c "set -x && (\"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ||\
+ (\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" &&\
+ \"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")")
Review comment:
Isn't this backwards? We want to "dry-run" the reverse patch first, and only run the real patch if the dry-run failed:
```suggestion
set(PC bash -c "set -x &&\
(\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE1}\" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE1}\") &&\
(\"${Patch_EXECUTABLE}\" -p1 -R --dry-run -i \"${PATCH_FILE2}\")" || \"${Patch_EXECUTABLE}\" -p1 -N -i \"${PATCH_FILE2}\") ")
```
##########
File path: cmake/CppVersion.cmake
##########
@@ -0,0 +1,42 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+function(set_cpp_version)
+ if (MSVC)
+ if ((MSVC_VERSION GREATER "1910") OR (MSVC_VERSION EQUAL "1910"))
+ add_compile_options("/std:c++latest")
+ add_compile_options("/permissive-")
+ else()
+ message(STATUS "The compiler ${CMAKE_CXX_COMPILER} has no C++17 support. Please use a different C++ compiler.")
Review comment:
This error message is misleading, as we aren't checking C++17 support directly.
```suggestion
message(STATUS "The Visual Studio C++ compiler ${CMAKE_CXX_COMPILER} is not supported. Please use Visual Studio 2017 or newer.")
```
--
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