You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/10 18:38:32 UTC
[GitHub] [arrow] save-buffer opened a new pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
save-buffer opened a new pull request #12605:
URL: https://github.com/apache/arrow/pull/12605
Was hitting some issue on Xcode with it not being able to find the correct version of gtest (it's suffixed with "d" if it's debug and release otherwise). I think the issue is that Xcode figures out for itself whether to build Debug or Release _after_ CMake runs, which means that Xcode is stuck only being able to build in one of the two.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r837017716
##########
File path: cpp/thirdparty/versions.txt
##########
@@ -121,7 +121,7 @@ DEPENDENCIES=(
"ARROW_GLOG_URL glog-${ARROW_GLOG_BUILD_VERSION}.tar.gz https://github.com/google/glog/archive/${ARROW_GLOG_BUILD_VERSION}.tar.gz"
"ARROW_GOOGLE_CLOUD_CPP_URL google-cloud-cpp-${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz https://github.com/googleapis/google-cloud-cpp/archive/refs/tags/${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz"
"ARROW_GRPC_URL grpc-${ARROW_GRPC_BUILD_VERSION}.tar.gz https://github.com/grpc/grpc/archive/${ARROW_GRPC_BUILD_VERSION}.tar.gz"
- "ARROW_GTEST_URL gtest-${ARROW_GTEST_BUILD_VERSION}.tar.gz https://github.com/google/googletest/archive/release-${ARROW_GTEST_BUILD_VERSION}.tar.gz"
+ "ARROW_GTEST_URL {ARROW_GTEST_BUILD_VERSION}.tar.gz https://github.com/google/googletest/archive/${ARROW_GTEST_BUILD_VERSION}.tar.gz"
Review comment:
Just adding `googletest-` is sufficient right? I've added that back in
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r825249922
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
Ah yeah then that might be the same problem. Visual Studio probably also sets the build type independently of configuration.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824944536
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
Could be - what do you use to build on Windows? Visual Studio?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824048731
##########
File path: cpp/CMakeLists.txt
##########
@@ -47,6 +47,10 @@ if(POLICY CMP0074)
cmake_policy(SET CMP0074 NEW)
endif()
+if(POLICY CMP0114)
+ cmake_policy(SET CMP0114 NEW)
Review comment:
Would you mind adding a comment explaining why this is necessary? So that future readers are not confused by this.
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
I don't understand, why is it always "d" for XCode?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1064381201
@kou I was told you could help review this, would you mind taking a look? 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824063473
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
As far as I can tell, it's because Xcode selects whether to build in debug or release mode after configuration, so if it's not set then it will succeed building in Release but not in Debug because it'll try to find `libgtest.dylib` instead of `libgtestd.dylib`. Without this change, if I specified `-DCMAKE_BUILD_TYPE=Debug`, Xcode would be able to build in debug mode but not release mode.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1075680519
(I'm trying to reproduce this on Linux with `-G "Ninja Multi-Config"`. Please wait for a week... I don't have enough time to work on this for now...)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1081181649
Switching to latest commit seems to work for me.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1081282028
@westonpace do you want to try this on Windows before we merge this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1064373972
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r826247125
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
Review comment:
Now it seems to be not evaluating it
```
ninja: error: 'googletest_ep-prefix/lib/libgtest$<$<CONFIG:Debug>:d>.dylib', needed by 'debug/libarrow_testing.800.0.0.dylib', missing and no known rule to make it
```
I think generator expressions may be unnecessary overall, everything is known at configuration-time, so there's no need to evaluate anything at build-time. Multiconfiguration generators always want the flag set, and single-configuration generators only want it set in debug mode.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1074554560
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824214174
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
Hmm...I get this error in Windows if I do `cmake --build .` instead of `cmake --config Debug --build .`
Coming from Linux this is confusing since I never have to specify `--config` but perhaps the issue here is related?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r825249933
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
Any chance you can test out this fix 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824069074
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
I think it'll build GTest the correct way, but it won't name it properly.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824066800
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
Do you mean it always builds GTest in debug mode?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r836899422
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1850,12 +1849,6 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
- else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
- endif()
-
Review comment:
Could you also remove `${CMAKE_GTEST_DEBUG_EXTENSION}` from `_GTEST_LIBRARY_SUFFIX` and `_GTEST_RUNTIME_SUFFIX`?
##########
File path: cpp/thirdparty/versions.txt
##########
@@ -121,7 +121,7 @@ DEPENDENCIES=(
"ARROW_GLOG_URL glog-${ARROW_GLOG_BUILD_VERSION}.tar.gz https://github.com/google/glog/archive/${ARROW_GLOG_BUILD_VERSION}.tar.gz"
"ARROW_GOOGLE_CLOUD_CPP_URL google-cloud-cpp-${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz https://github.com/googleapis/google-cloud-cpp/archive/refs/tags/${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz"
"ARROW_GRPC_URL grpc-${ARROW_GRPC_BUILD_VERSION}.tar.gz https://github.com/grpc/grpc/archive/${ARROW_GRPC_BUILD_VERSION}.tar.gz"
- "ARROW_GTEST_URL gtest-${ARROW_GTEST_BUILD_VERSION}.tar.gz https://github.com/google/googletest/archive/release-${ARROW_GTEST_BUILD_VERSION}.tar.gz"
+ "ARROW_GTEST_URL {ARROW_GTEST_BUILD_VERSION}.zip https://github.com/google/googletest/archive/${ARROW_GTEST_BUILD_VERSION}.zip"
Review comment:
Could you use `.tar.gz` like others?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r836956797
##########
File path: cpp/thirdparty/versions.txt
##########
@@ -121,7 +121,7 @@ DEPENDENCIES=(
"ARROW_GLOG_URL glog-${ARROW_GLOG_BUILD_VERSION}.tar.gz https://github.com/google/glog/archive/${ARROW_GLOG_BUILD_VERSION}.tar.gz"
"ARROW_GOOGLE_CLOUD_CPP_URL google-cloud-cpp-${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz https://github.com/googleapis/google-cloud-cpp/archive/refs/tags/${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz"
"ARROW_GRPC_URL grpc-${ARROW_GRPC_BUILD_VERSION}.tar.gz https://github.com/grpc/grpc/archive/${ARROW_GRPC_BUILD_VERSION}.tar.gz"
- "ARROW_GTEST_URL gtest-${ARROW_GTEST_BUILD_VERSION}.tar.gz https://github.com/google/googletest/archive/release-${ARROW_GTEST_BUILD_VERSION}.tar.gz"
+ "ARROW_GTEST_URL {ARROW_GTEST_BUILD_VERSION}.tar.gz https://github.com/google/googletest/archive/${ARROW_GTEST_BUILD_VERSION}.tar.gz"
Review comment:
Could you add `googletest-` prefix for easy to know package name:
```console
$ curl -L -v https://github.com/google/googletest/archive/af29db7ec28d6df1c7f0f745186884091e602e07.tar.gz 2>&1 | grep filename=
< content-disposition: attachment; filename=googletest-af29db7ec28d6df1c7f0f745186884091e602e07.tar.gz
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824975337
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
Both visual studio and command line (with visual studio generator)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r825492030
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
Review comment:
As for generator expression, I got a lot of repeating of this:
```
CMake Error at /opt/homebrew/Cellar/cmake/3.22.3/share/cmake/Modules/ExternalProject.cmake:2406:EVAL:2 (add_custom_command):
Error evaluating generator expression:
$<$<CONFIG:Debug>,d>
Expression did not evaluate to a known generator expression
```
Not too sure what to do about it, so I've not switched to generator expression yet.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1080173576
Oh thank you for looking into this! I think that's a great solution too. If we switch to the GTest head, we can just get rid of that whole block dealing with the d suffix right?
Also looking at GTest's readme, I see that it says
> GoogleTest now follows the [Abseil Live at Head philosophy](https://abseil.io/about/philosophy#upgrade-support)[](https://github.com/apache/arrow/pull/12605#documentation-updates). We recommend using the latest commit in the main branch in your projects.
Maybe we could consider just always using the latest commit?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r836951571
##########
File path: cpp/thirdparty/versions.txt
##########
@@ -121,7 +121,7 @@ DEPENDENCIES=(
"ARROW_GLOG_URL glog-${ARROW_GLOG_BUILD_VERSION}.tar.gz https://github.com/google/glog/archive/${ARROW_GLOG_BUILD_VERSION}.tar.gz"
"ARROW_GOOGLE_CLOUD_CPP_URL google-cloud-cpp-${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz https://github.com/googleapis/google-cloud-cpp/archive/refs/tags/${ARROW_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz"
"ARROW_GRPC_URL grpc-${ARROW_GRPC_BUILD_VERSION}.tar.gz https://github.com/grpc/grpc/archive/${ARROW_GRPC_BUILD_VERSION}.tar.gz"
- "ARROW_GTEST_URL gtest-${ARROW_GTEST_BUILD_VERSION}.tar.gz https://github.com/google/googletest/archive/release-${ARROW_GTEST_BUILD_VERSION}.tar.gz"
+ "ARROW_GTEST_URL {ARROW_GTEST_BUILD_VERSION}.zip https://github.com/google/googletest/archive/${ARROW_GTEST_BUILD_VERSION}.zip"
Review comment:
Switched it.
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1850,12 +1849,6 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
- else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
- endif()
-
Review comment:
Done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1081275213
There was also a failure with jemalloc in debug builds now after I rebased. It seems that we were appending `--enable-debug` after appending the `EP_LOG_OPTIONS`, which made `log_output_on_failure` be set to `1;--enable-debug`, which caused a failure. I fixed that also
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r826247125
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
Review comment:
Now it seems to be not evaluating it
```
ninja: error: 'googletest_ep-prefix/lib/libgtest$<$<CONFIG:Debug>:d>.dylib', needed by 'debug/libarrow_testing.800.0.0.dylib', missing and no known rule to make it
```
I think generator expressions may be unnecessary overall, everything is known at configuration-time, so there's no need to evaluate anything at build-time. Multiconfiguration generators always want the flag set, and single-configuration generators only want it in release mode.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1081370662
The appveyor failure looks like a problem. It's a slightly different link error than the one I'm getting but I think in spirit it is the same. The arrow testing library gets built as a shared object and references some of gtest. Those parts of gtest do not seem to be properly exporting their symbols.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1080222187
> If we switch to the GTest head, we can just get rid of that whole block dealing with the d suffix right?
I think so. Could you try it?
> Also looking at GTest's readme, I see that it says
>
> > GoogleTest now follows the [Abseil Live at Head philosophy](https://abseil.io/about/philosophy#upgrade-support). We recommend using the latest commit in the main branch in your projects.
>
> Maybe we could consider just always using the latest commit?
Ah, I didn't notice it.
Could you use the latest commit?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace removed a comment on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
westonpace removed a comment on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1074554560
I'm not sure this change, as it stands, does anything. But I am no cmake expert. `GENERATOR_IS_MULTI_CONFIG` is a global property and not a variable. So, there is no variable named `GENERATOR_IS_MULTI_CONFIG` and so it defaults to `""`. So it will never affect the if statement because it will always be false.
On my Windows machine it doesn't matter if I'm using the VS generator or the Ninja generator, it still sets `CMAKE_GTEST_DEBUG_EXTENSION` based on `CMAKE_BUILD_TYPE`.
On the other hand, if I do (note this is what we do later on in the line 1941 link @kou provided)...
```
get_property(_GENERATOR_IS_MULTI_CONFIG GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
if(_GENERATOR_IS_MULTI_CONFIG)
```
...then it does recognize the property. However, if I then try and build in release mode, it fails because it can't find the file. So all this does is switch multi-config generators from "always fails when building debug" to "always fails when building release". You claim `Multiconfiguration generators always want the flag set` but I'm not sure that statement is true. A multi-configuration generator wants `gtestd.dll` if run with `--config Debug` and it wants `gtest.dll` if run with `--config Release`. There is no way to know, at configuration time, what will be needed at runtime.
So if we are going to build gtest at configuration time (which we need to do if we want to use gtest for test discovery) then we either need to build both versions if it is multi-config or we need to always build one version but only use that version for test discovery and not for a runtime dependency.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r825491934
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
Review comment:
`GENERATOR_IS_MULTI_CONFIG` seems to work on Xcode for me, I've switched to it. @westonpace could you give it a go on Visual Studio?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r825526081
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
Review comment:
Ah, sorry. I had a typo: `,` -> `:`: `$<$<CONFIG:Debug>:d>`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a change in pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r825298738
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
Review comment:
How about using `GENERATOR_IS_MULTI_CONFIG` https://cmake.org/cmake/help/latest/prop_gbl/GENERATOR_IS_MULTI_CONFIG.html ?
We already use it: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L1941
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1844,10 +1844,14 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ if(NOT CMAKE_GENERATOR STREQUAL Xcode)
+ if(CMAKE_BUILD_TYPE MATCHES DEBUG)
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
+ else()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ endif()
else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
+ set(CMAKE_GTEST_DEBUG_EXTENSION "d")
Review comment:
How about using generator expression https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html ? We can use generator expression in `ExternalProject_Add`'s arguments: https://cmake.org/cmake/help/latest/module/ExternalProject.html
```diff
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index ff48ac7ee5..44cab1d6b1 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -1849,11 +1849,7 @@ macro(build_gtest)
set(GTEST_VENDORED TRUE)
set(GTEST_CMAKE_CXX_FLAGS ${EP_CXX_FLAGS})
- if(CMAKE_BUILD_TYPE MATCHES DEBUG)
- set(CMAKE_GTEST_DEBUG_EXTENSION "d")
- else()
- set(CMAKE_GTEST_DEBUG_EXTENSION "")
- endif()
+ set(CMAKE_GTEST_DEBUG_EXTENSION "$<$<CONFIG:Debug>,d>")
if(APPLE)
set(GTEST_CMAKE_CXX_FLAGS ${GTEST_CMAKE_CXX_FLAGS} -DGTEST_USE_OWN_TR1_TUPLE=1
@@ -1894,9 +1890,9 @@ macro(build_gtest)
set(GTEST_CMAKE_ARGS
${EP_COMMON_TOOLCHAIN}
-DBUILD_SHARED_LIBS=ON
- -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
+ -DCMAKE_BUILD_TYPE=$<CONFIG>
-DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS}
- -DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS}
+ -DCMAKE_CXX_FLAGS_$<UPPER_CASE:$<CONFIG>>=${GTEST_CMAKE_CXX_FLAGS}
-DCMAKE_INSTALL_LIBDIR=lib
-DCMAKE_INSTALL_NAME_DIR=${GTEST_INSTALL_NAME_DIR}
-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] save-buffer commented on a change in pull request #12605: ARROW-15911: [C++] Minor CMake fixes for MacOS/XCode
Posted by GitBox <gi...@apache.org>.
save-buffer commented on a change in pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#discussion_r824067899
##########
File path: cpp/CMakeLists.txt
##########
@@ -47,6 +47,10 @@ if(POLICY CMP0074)
cmake_policy(SET CMP0074 NEW)
endif()
+if(POLICY CMP0114)
+ cmake_policy(SET CMP0114 NEW)
Review comment:
Added a comment, I don't know the exact details, just that CMake complains vocally with a lot of yellow text if it's not set (or set to OLD).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #12605: ARROW-15911: [C++] Find correct GTest on macOS with Xcode
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #12605:
URL: https://github.com/apache/arrow/pull/12605#issuecomment-1079571688
It seems that recent GoogleTest removes `d` postfix: https://github.com/google/googletest/pull/3732
The change isn't released yet. How about using the current main for bundled GoogleTest?
We can change to use a released version again once GoogleTest 1.12.0 is released.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org