kou commented on a change in pull request #7818:
URL: https://github.com/apache/arrow/pull/7818#discussion_r461245013
##########
File path: cpp/src/arrow/CMakeLists.txt
##########
@@ -490,6 +490,10 @@ if(ARROW_BUILD_STATIC AND WIN32)
target_compile_definitions(arrow_static PUBLIC ARROW_STATIC)
endif()
+if(NOT ARROW_UTF8PROC_USE_SHARED AND WIN32)
+ target_compile_definitions(arrow_static PRIVATE UTF8PROC_STATIC)
Review comment:
Could you try `INTERFACE_COMPILER_DEFINITIONS`?
https://github.com/apache/arrow/pull/7818#discussion_r459082935
##########
File path: cpp/cmake_modules/Findutf8proc.cmake
##########
@@ -15,10 +15,16 @@
# specific language governing permissions and limitations
# under the License.
+if(ARROW_UTF8PROC_USE_SHARED)
+ set(UTF8PROC_NAMES utf8proc)
+else()
+ set(UTF8PROC_NAMES utf8proc utf8proc_static)
+endif()
Review comment:
How about more strict logic like our `FindZSTD.cmake`?
https://github.com/apache/arrow/blob/master/cpp/cmake_modules/Fondest.cmake#L18-L34
Because `_static` suffix is only appended for MSVC:
https://github.com/JuliaStrings/utf8proc/blob/master/CMakeLists.txt#L32-L34
```suggestion
if(ARROW_UTFPROC8_USE_SHARED)
set(UTFPROC8_NAMES)
if(CMAKE_IMPORTRARY_SUFFIX)
list(APPEND UTFPROC8_NAMES
"${CMAKE_IMPORT_LIBRARY_PREFIX}utfproc8${CMAKE_IMPORT_LIBRARY_SUFFIX}")
endif()
list(APPEND UTFPROC8_NAMES
"${CMAKE_SHARED_LIBRARY_PREFIX}utfproc8${CMAKE_SHARED_LIBRARY_SUFFIX}")
else()
if(MSVC AND NOT DEFINED UTFPROC8_MSVC_STATIC_LIB_SUFFIX)
set(UTFPROC8_MSVC_STATIC_LIB_SUFFIX "_static")
endif()
set(UTFPROC8_STATIC_LIB_SUFFIX
"${UTFPROC8_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(UTFPROC8_NAMES
"${CMAKE_STATIC_LIBRARY_PREFIX}utfproc8${UTFPROC8_STATIC_LIB_SUFFIX}")
endif()
```
##########
File path: cpp/cmake_modules/Findutf8proc.cmake
##########
@@ -32,7 +38,7 @@ if(utf8proc_ROOT)
else()
find_library(
UTF8PROC_LIB
- NAMES utf8proc
+ NAMES ${UTF8PROC_NAMES}
"${CMAKE_SHARED_LIBRARY_PREFIX}utf8proc${CMAKE_SHARED_LIBRARY_SUFFIX}"
Review comment:
ditto
##########
File path: cpp/cmake_modules/Findutf8proc.cmake
##########
@@ -15,10 +15,16 @@
# specific language governing permissions and limitations
# under the License.
+if(ARROW_UTF8PROC_USE_SHARED)
+ set(UTF8PROC_NAMES utf8proc)
+else()
+ set(UTF8PROC_NAMES utf8proc utf8proc_static)
+endif()
+
if(utf8proc_ROOT)
find_library(
UTF8PROC_LIB
- NAMES utf8proc
+ NAMES ${UTF8PROC_NAMES}
"${CMAKE_SHARED_LIBRARY_PREFIX}utf8proc${CMAKE_SHARED_LIBRARY_SUFFIX}"
Review comment:
Could you remove this?
It will find a shared library when we specify
`-DARROW_UTFPROC8_USE_SHARED=NO`.
##########
File path: cpp/src/arrow/CMakeLists.txt
##########
@@ -490,6 +490,10 @@ if(ARROW_BUILD_STATIC AND WIN32)
target_compile_definitions(arrow_static PUBLIC ARROW_STATIC)
endif()
+if(NOT ARROW_UTF8PROC_USE_SHARED AND WIN32)
+ target_compile_definitions(arrow_static PRIVATE UTF8PROC_STATIC)
Review comment:
Yes.
##########
File path: cpp/cmake_modules/Findutf8proc.cmake
##########
@@ -15,10 +15,16 @@
# specific language governing permissions and limitations
# under the License.
+if(ARROW_UTF8PROC_USE_SHARED)
+ set(UTF8PROC_NAMES utf8proc)
+else()
+ set(UTF8PROC_NAMES utf8proc utf8proc_static)
+endif()
Review comment:
How about more strict logic like our `FindZSTD.cmake`?
https://github.com/apache/arrow/blob/master/cpp/cmake_modules/Fondest.cmake#L18-L34
Because `_static` suffix is only appended for MSVC:
https://github.com/JuliaStrings/utf8proc/blob/master/CMakeLists.txt#L32-L34
```suggestion
if(ARROW_UTF8PROC_USE_SHARED)
set(UTF8PROC_NAMES)
if(CMAKE_IMPORTRARY_SUFFIX)
list(APPEND UTF8PROC_NAMES
"${CMAKE_IMPORT_LIBRARY_PREFIX}utf8proc${CMAKE_IMPORT_LIBRARY_SUFFIX}")
endif()
list(APPEND UTF8PROC_NAMES
"${CMAKE_SHARED_LIBRARY_PREFIX}utf8proc${CMAKE_SHARED_LIBRARY_SUFFIX}")
else()
if(MSVC AND NOT DEFINED UTF8PROC_MSVC_STATIC_LIB_SUFFIX)
set(UTF8PROC_MSVC_STATIC_LIB_SUFFIX "_static")
endif()
set(UTF8PROC_STATIC_LIB_SUFFIX
"${UTF8PROC_MSVC_STATIC_LIB_SUFFIX}${CMAKE_STATIC_LIBRARY_SUFFIX}")
set(UTF8PROC_NAMES
"${CMAKE_STATIC_LIBRARY_PREFIX}utf8proc${UTF8PROC_STATIC_LIB_SUFFIX}")
endif()
```
----------------------------------------------------------------
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
