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/01 21:04:03 UTC
[GitHub] [arrow] kou commented on a change in pull request #12406: ARROW-15667: [R] Test development build with ARROW_BUILD_STATIC=OFF
kou commented on a change in pull request #12406:
URL: https://github.com/apache/arrow/pull/12406#discussion_r817134344
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -2281,6 +2286,11 @@ if(ARROW_WITH_RE2)
resolve_dependency(re2 HAVE_ALT TRUE)
if(${re2_SOURCE} STREQUAL "SYSTEM")
get_target_property(RE2_LIB re2::re2 IMPORTED_LOCATION)
+ if(${RE2_LIB} STREQUAL "RE2_LIB-NOTFOUND")
+ # Sometimes this is set instead
+ get_target_property(RE2_LIB re2::re2 IMPORTED_LOCATION_RELEASE)
+ endif()
+
Review comment:
re2 installed by MSYS2 is built with `-DCMAKE_BUILD_TYPE=Release`. So `re2::re2` has only `IMPORTED_LOCATION_RELEASE` for this case. It's better that we give priority to `IMPORTED_LOCATION_${CONFIG}` over `IMPORTED_LOCATION`. And we can simplify `if(${RE2_LIB} STREQUAL "RE2_LIB-NOTFOUND")` to `if(NOT RE2_LIB)` because `-NOTFOUND` value is false in CMake https://cmake.org/cmake/help/latest/command/if.html#basic-expressions :
```suggestion
get_target_property(RE2_LIB re2::re2 IMPORTED_LOCATION_${UPPERCAE_BUILD_TYPE})
if(NOT RE2_LIB)
get_target_property(RE2_LIB re2::re2 IMPORTED_LOCATION_RELEASE)
endif()
if(NOT RE2_LIB)
get_target_property(RE2_LIB re2::re2 IMPORTED_LOCATION)
endif()
```
##########
File path: r/configure.win
##########
@@ -85,36 +85,31 @@ function configure_dev() {
echo "*** Using locally built Arrow at $ARROW_HOME"
RWINLIB=$(cygpath $ARROW_HOME)
- PKG_CFLAGS="-I${RWINLIB}/include -DARROW_R_WITH_ARROW"
- PKG_LIBS=" -L${RWINLIB}/lib -L$MSYSTEM_PREFIX/lib -larrow -larrow_bundled_dependencies -lz -lole32 ${MIMALLOC_LIBS} ${OPENSSL_LIBS}"
- # These are already in arrow_bundled_dependencies:
- # -lutf8proc -lthrift -lsnappy -lzstd -llz4
+ export PKG_CONFIG_PATH=$(cygpath $ARROW_HOME)/lib/pkgconfig:$(cygpath $MSYSTEM_PREFIX)/lib
+ PKG_CONFIG_LIBS="arrow"
Review comment:
`PKG_CONFIG_PACKAGES` is better because pkg-config calls each argument as package not library.
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -1716,6 +1716,11 @@ if(ARROW_MIMALLOC)
IMPORTED_LOCATION "${MIMALLOC_STATIC_LIB}"
INTERFACE_INCLUDE_DIRECTORIES
"${MIMALLOC_INCLUDE_DIR}")
+ if(WIN32)
+ set_property(TARGET mimalloc::mimalloc
+ APPEND
+ PROPERTY INTERFACE_LINK_LIBRARIES "bcrypt.lib" "psapi")
Review comment:
Could you also add `.lib` to `psali` explicitly?
```suggestion
PROPERTY INTERFACE_LINK_LIBRARIES "bcrypt.lib" "psapi.lib")
```
--
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