[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message
vvysotskyi commented on a change in pull request #2071: URL: https://github.com/apache/drill/pull/2071#discussion_r429391914 ## File path: contrib/native/client/CMakeLists.txt ## @@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW) cmake_policy(SET CMP0048 NEW) enable_testing() +# +# required version for dependencies +# +set (BOOST_MINIMUM_VERSION 1.54.0) +set (PROTOBUF_MINIMUM_VERSION 3.6.1) Review comment: Yes, it can be merged, but after resolving conflicts. 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: us...@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message
vvysotskyi commented on a change in pull request #2071: URL: https://github.com/apache/drill/pull/2071#discussion_r419654433 ## File path: contrib/native/client/CMakeLists.txt ## @@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW) cmake_policy(SET CMP0048 NEW) enable_testing() +# +# required version for dependencies +# +set (BOOST_MINIMUM_VERSION 1.54.0) +set (PROTOBUF_MINIMUM_VERSION 3.6.1) Review comment: protoc compiler is required to be installed when regenerating c++ protobuf files, not java classes, therefore I pointed to that PRs in my previous comment. But this change doesn't make things worse, so it may be merged as it is. 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: us...@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message
vvysotskyi commented on a change in pull request #2071: URL: https://github.com/apache/drill/pull/2071#discussion_r418734189 ## File path: contrib/native/client/CMakeLists.txt ## @@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW) cmake_policy(SET CMP0048 NEW) enable_testing() +# +# required version for dependencies +# +set (BOOST_MINIMUM_VERSION 1.54.0) +set (PROTOBUF_MINIMUM_VERSION 3.6.1) Review comment: Here is an example of the issue when another version was used for generating protobuf files: https://github.com/apache/drill/pull/2000#issuecomment-592024872. Your change would significantly simplify finding the root cause of such issues. By the way, here are changes in protobuf files for updating to 3.11.1 version: https://github.com/apache/drill/commit/7e6fc81ef414cdeca8120c20e12fcce21893f7bf#diff-b5a38f368686e0d3202ec29cd8276d6f So it may be possible that these newer changes use some new methods introduced after 3.6. Regarding validating windows build, I agree that it can be done separately. 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: us...@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message
vvysotskyi commented on a change in pull request #2071: URL: https://github.com/apache/drill/pull/2071#discussion_r418724733 ## File path: contrib/native/client/CMakeLists.txt ## @@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW) cmake_policy(SET CMP0048 NEW) enable_testing() +# +# required version for dependencies +# +set (BOOST_MINIMUM_VERSION 1.54.0) +set (PROTOBUF_MINIMUM_VERSION 3.6.1) Review comment: It may cause issues for other places since protobuf files were regenerated using 3.11 version. Looks like updating `readme.win.txt` instruction was missed and should be fixed to be up-to-date. 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: us...@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message
vvysotskyi commented on a change in pull request #2071: URL: https://github.com/apache/drill/pull/2071#discussion_r418484316 ## File path: contrib/native/client/CMakeLists.txt ## @@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW) cmake_policy(SET CMP0048 NEW) enable_testing() +# +# required version for dependencies +# +set (BOOST_MINIMUM_VERSION 1.54.0) +set (PROTOBUF_MINIMUM_VERSION 3.6.1) Review comment: The current version uses protobuf 3.11 version (see readme.linux). 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: us...@infra.apache.org
