Apply review comments

cpp/cmake_modules/FindNumPy.cmake

@@ -38,26 +38,8 @@
 #
 #============================================================================
 
-if(${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.15.0")
-    # FindPython3Alt.cmake also looks for NumPy, use it.
-    if(NumPy_FIND_REQUIRED)
-        find_package(Python3Alt REQUIRED)
-    else()
-        find_package(Python3Alt)
-    endif()
-    get_target_property(NUMPY_INCLUDE_DIRS Python3::NumPy INTERFACE_INCLUDE_DIRECTORIES)
-    message(STATUS "Found NumPy: ${NUMPY_INCLUDE_DIRS}")
-    return()
-endif()
-
-# Legacy code below (CMake < 3.15.0)
-
-# Finding NumPy involves calling the Python interpreter
-if(NumPy_FIND_REQUIRED)
-    find_package(PythonInterp REQUIRED)
-else()
-    find_package(PythonInterp)
-endif()
+# Legacy code for CMake < 3.15.0.  The primary point of entry should be
+# FindPython3Alt.cmake.
 
 if(NOT PYTHONINTERP_FOUND)
     set(NUMPY_FOUND FALSE)

cpp/cmake_modules/FindPython3Alt.cmake

@@ -15,21 +15,30 @@
 # specific language governing permissions and limitations
 # under the License.
 
-# This module finds the libraries corresponding to the Python 3 interpeter,
-# and sets the following variables:
+# This module finds the libraries corresponding to the Python 3 interpreter
+# and the NumPy package, and sets the following variables:
 # - PYTHON_EXECUTABLE
 # - PYTHON_INCLUDE_DIRS
 # - PYTHON_LIBRARIES
 # - PYTHON_OTHER_LIBS
+# - NUMPY_INCLUDE_DIRS
 
 # Need CMake 3.15 or later for Python3_FIND_STRATEGY
 if(${CMAKE_VERSION} VERSION_LESS "3.15.0")
-  # Use deprecated FindPythonInterp package
+  # Use deprecated Python- and NumPy-finding code
   if(Python3Alt_FIND_REQUIRED)
     find_package(PythonLibsNew REQUIRED)
+    find_package(NumPy REQUIRED)
   else()
     find_package(PythonLibsNew)
+    find_package(NumPy)
   endif()
+  find_package_handle_standard_args(Python3Alt
+                                    REQUIRED_VARS
+                                    PYTHON_EXECUTABLE
+                                    PYTHON_LIBRARIES
+                                    PYTHON_INCLUDE_DIRS
+                                    NUMPY_INCLUDE_DIRS)
   return()
 endif()
 
@@ -48,8 +57,33 @@ set(PYTHON_INCLUDE_DIRS ${Python3_INCLUDE_DIRS})
 set(PYTHON_LIBRARIES ${Python3_LIBRARIES})
 set(PYTHON_OTHER_LIBS)
 
-message(STATUS "Found Python 3 libraries: ${PYTHON_LIBRARIES}")
+get_target_property(NUMPY_INCLUDE_DIRS Python3::NumPy INTERFACE_INCLUDE_DIRECTORIES)
 
-function(PYTHON_ADD_MODULE _NAME)
-  python3_add_library(${_NAME} ${ARGN})
+# CMake's python3_add_library() doesn't apply the required extension suffix,
+# detect it ourselves.
+# (https://gitlab.kitware.com/cmake/cmake/issues/20408)
+execute_process(COMMAND "${PYTHON_EXECUTABLE}" "-c"
+                        "import sysconfig; print(sysconfig.get_config_var('EXT_SUFFIX'))"
+                RESULT_VARIABLE _PYTHON_RESULT
+                OUTPUT_VARIABLE _PYTHON_STDOUT
+                ERROR_VARIABLE _PYTHON_STDERR)
+
+if(NOT _PYTHON_RESULT MATCHES 0)
+  if(Python3Alt_FIND_REQUIRED)
+    message(FATAL_ERROR "Python 3 config failure:\n${_PYTHON_STDERR}")
+  endif()
+endif()
+
+string(STRIP ${_PYTHON_STDOUT} _EXT_SUFFIX)
+
+function(PYTHON_ADD_MODULE name)
+  python3_add_library(${name} MODULE ${ARGN})
+  set_target_properties(${name} PROPERTIES SUFFIX ${_EXT_SUFFIX})
 endfunction()
+
+find_package_handle_standard_args(Python3Alt
+                                  REQUIRED_VARS
+                                  PYTHON_EXECUTABLE
+                                  PYTHON_LIBRARIES
+                                  PYTHON_INCLUDE_DIRS
+                                  NUMPY_INCLUDE_DIRS)

cpp/cmake_modules/UseCython.cmake

@@ -58,119 +58,127 @@
 #=============================================================================
 
 # Configuration options.
-set( CYTHON_ANNOTATE OFF
-  CACHE BOOL "Create an annotated .html file when compiling *.pyx." )
-set( CYTHON_NO_DOCSTRINGS OFF
-  CACHE BOOL "Strip docstrings from the compiled module." )
-set( CYTHON_FLAGS "" CACHE STRING
-  "Extra flags to the cython compiler." )
-mark_as_advanced( CYTHON_ANNOTATE CYTHON_NO_DOCSTRINGS CYTHON_FLAGS)
+set(CYTHON_ANNOTATE OFF CACHE BOOL "Create an annotated .html file when compiling *.pyx.")
+set(CYTHON_NO_DOCSTRINGS OFF CACHE BOOL "Strip docstrings from the compiled module.")
+set(CYTHON_FLAGS "" CACHE STRING "Extra flags to the cython compiler.")
+mark_as_advanced(CYTHON_ANNOTATE CYTHON_NO_DOCSTRINGS CYTHON_FLAGS)
 
-find_package( PythonLibsNew REQUIRED )
+find_package(Python3Alt REQUIRED)
 
 # (using another C++ extension breaks coverage)
-set( CYTHON_CXX_EXTENSION "cpp" )
-set( CYTHON_C_EXTENSION "c" )
+set(CYTHON_CXX_EXTENSION "cpp")
+set(CYTHON_C_EXTENSION "c")
 
 # Create a *.c or *.cpp file from a *.pyx file.
 # Input the generated file basename.  The generate files will put into the variable
 # placed in the "generated_files" argument. Finally all the *.py and *.pyx files.
-function( compile_pyx _name pyx_target_name generated_files pyx_file)
+function(compile_pyx
+         _name
+         pyx_target_name
+         generated_files
+         pyx_file)
   # Default to assuming all files are C.
-  set( cxx_arg "" )
-  set( extension ${CYTHON_C_EXTENSION} )
-  set( pyx_lang "C" )
-  set( comment "Compiling Cython C source for ${_name}..." )
+  set(cxx_arg "")
+  set(extension ${CYTHON_C_EXTENSION})
+  set(pyx_lang "C")
+  set(comment "Compiling Cython C source for ${_name}...")
 
-  get_filename_component( pyx_file_basename "${pyx_file}" NAME_WE )
+  get_filename_component(pyx_file_basename "${pyx_file}" NAME_WE)
 
   # Determine if it is a C or C++ file.
-  get_source_file_property( property_is_cxx ${pyx_file} CYTHON_IS_CXX )
-  if( ${property_is_cxx} )
-    set( cxx_arg "--cplus" )
-    set( extension ${CYTHON_CXX_EXTENSION} )
-    set( pyx_lang "CXX" )
-    set( comment "Compiling Cython CXX source for ${_name}..." )
+  get_source_file_property(property_is_cxx ${pyx_file} CYTHON_IS_CXX)
+  if(${property_is_cxx})
+    set(cxx_arg "--cplus")
+    set(extension ${CYTHON_CXX_EXTENSION})
+    set(pyx_lang "CXX")
+    set(comment "Compiling Cython CXX source for ${_name}...")
   endif()
-  get_source_file_property( pyx_location ${pyx_file} LOCATION )
+  get_source_file_property(pyx_location ${pyx_file} LOCATION)
 
-  set ( output_file "${_name}.${extension}" )
+  set(output_file "${_name}.${extension}")
 
   # Set additional flags.
-  if( CYTHON_ANNOTATE )
-    set( annotate_arg "--annotate" )
+  if(CYTHON_ANNOTATE)
+    set(annotate_arg "--annotate")
   endif()
 
-  if( CYTHON_NO_DOCSTRINGS )
-    set( no_docstrings_arg "--no-docstrings" )
+  if(CYTHON_NO_DOCSTRINGS)
+    set(no_docstrings_arg "--no-docstrings")
   endif()
 
   if(NOT WIN32)
-      if( "${CMAKE_BUILD_TYPE}" STREQUAL "Debug" OR
-            "${CMAKE_BUILD_TYPE}" STREQUAL "RelWithDebInfo" )
-          set( cython_debug_arg "--gdb" )
-      endif()
+    if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug"
+       OR "${CMAKE_BUILD_TYPE}" STREQUAL "RelWithDebInfo")
+      set(cython_debug_arg "--gdb")
+    endif()
   endif()
 
   # Determining generated file names.
-  get_source_file_property( property_is_public ${pyx_file} CYTHON_PUBLIC )
-  get_source_file_property( property_is_api ${pyx_file} CYTHON_API )
-  if( ${property_is_api} )
-      set( _generated_files "${output_file}" "${_name}.h" "${name}_api.h")
-  elseif( ${property_is_public} )
-      set( _generated_files "${output_file}" "${_name}.h")
+  get_source_file_property(property_is_public ${pyx_file} CYTHON_PUBLIC)
+  get_source_file_property(property_is_api ${pyx_file} CYTHON_API)
+  if(${property_is_api})
+    set(_generated_files "${output_file}" "${_name}.h" "${name}_api.h")
+  elseif(${property_is_public})
+    set(_generated_files "${output_file}" "${_name}.h")
   else()
-      set( _generated_files "${output_file}")
+    set(_generated_files "${output_file}")
   endif()
-  set_source_files_properties( ${_generated_files} PROPERTIES GENERATED TRUE )
+  set_source_files_properties(${_generated_files} PROPERTIES GENERATED TRUE)
 
-  if (NOT WIN32)
+  if(NOT WIN32)
     # Cython creates a lot of compiler warning detritus on clang
-    set_source_files_properties(${_generated_files} PROPERTIES
-      COMPILE_FLAGS -Wno-unused-function)
+    set_source_files_properties(${_generated_files} PROPERTIES COMPILE_FLAGS
+                                -Wno-unused-function)
   endif()
 
-  set( ${generated_files} ${_generated_files} PARENT_SCOPE )
+  set(${generated_files} ${_generated_files} PARENT_SCOPE)
 
   # Add the command to run the compiler.
-  add_custom_target(${pyx_target_name}
-    COMMAND ${PYTHON_EXECUTABLE} -m cython ${cxx_arg}
-    ${annotate_arg} ${no_docstrings_arg} ${cython_debug_arg}
-    ${CYTHON_FLAGS}
-    # Necessary for Cython code coverage
-    --working ${CMAKE_CURRENT_SOURCE_DIR}
-    --output-file "${CMAKE_CURRENT_BINARY_DIR}/${output_file}"
-    "${CMAKE_CURRENT_SOURCE_DIR}/${pyx_file}"
+  add_custom_target(
+    ${pyx_target_name}
+    COMMAND ${PYTHON_EXECUTABLE}
+            -m
+            cython
+            ${cxx_arg}
+            ${annotate_arg}
+            ${no_docstrings_arg}
+            ${cython_debug_arg}
+            ${CYTHON_FLAGS}
+            # Necessary for Cython code coverage
+            --working
+            ${CMAKE_CURRENT_SOURCE_DIR}
+            --output-file
+            "${CMAKE_CURRENT_BINARY_DIR}/${output_file}"
+            "${CMAKE_CURRENT_SOURCE_DIR}/${pyx_file}"
     DEPENDS ${pyx_location}
-    # Do not specify byproducts for now since they don't work with the older
-    # version of cmake available in the apt repositories.
-    #BYPRODUCTS ${_generated_files}
-    COMMENT ${comment}
-    )
+            # Do not specify byproducts for now since they don't work with the older
+            # version of cmake available in the apt repositories.
+            #BYPRODUCTS ${_generated_files}
+    COMMENT ${comment})
 
   # Remove their visibility to the user.
-  set( corresponding_pxd_file "" CACHE INTERNAL "" )
-  set( header_location "" CACHE INTERNAL "" )
-  set( pxd_location "" CACHE INTERNAL "" )
+  set(corresponding_pxd_file "" CACHE INTERNAL "")
+  set(header_location "" CACHE INTERNAL "")
+  set(pxd_location "" CACHE INTERNAL "")
 endfunction()
 
 # cython_add_module( <name> src1 src2 ... srcN )
 # Build the Cython Python module.
-function( cython_add_module _name pyx_target_name generated_files)
-  set( pyx_module_source "" )
-  set( other_module_sources "" )
-  foreach( _file ${ARGN} )
-    if( ${_file} MATCHES ".*\\.py[x]?$" )
-      list( APPEND pyx_module_source ${_file} )
+function(cython_add_module _name pyx_target_name generated_files)
+  set(pyx_module_source "")
+  set(other_module_sources "")
+  foreach(_file ${ARGN})
+    if(${_file} MATCHES ".*\\.py[x]?$")
+      list(APPEND pyx_module_source ${_file})
     else()
-      list( APPEND other_module_sources ${_file} )
+      list(APPEND other_module_sources ${_file})
     endif()
   endforeach()
-  compile_pyx( ${_name} ${pyx_target_name} _generated_files ${pyx_module_source} )
-  set( ${generated_files} ${_generated_files} PARENT_SCOPE )
-  include_directories( ${PYTHON_INCLUDE_DIRS} )
-  python_add_module( ${_name} ${_generated_files} ${other_module_sources} )
-  add_dependencies( ${_name} ${pyx_target_name})
+  compile_pyx(${_name} ${pyx_target_name} _generated_files ${pyx_module_source})
+  set(${generated_files} ${_generated_files} PARENT_SCOPE)
+  include_directories(${PYTHON_INCLUDE_DIRS})
+  python_add_module(${_name} ${_generated_files} ${other_module_sources})
+  add_dependencies(${_name} ${pyx_target_name})
 endfunction()
 
-include( CMakeParseArguments )
+include(CMakeParseArguments)

cpp/src/arrow/python/CMakeLists.txt

@@ -20,7 +20,6 @@
 #
 
 find_package(Python3Alt REQUIRED)
-find_package(NumPy REQUIRED)
 
 add_custom_target(arrow_python-all)
 add_custom_target(arrow_python)

python/CMakeLists.txt

@@ -190,9 +190,8 @@ if(PYARROW_USE_TENSORFLOW)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_GLIBCXX_USE_CXX11_ABI=0")
 endif()
 
-# Python and libraries
-find_package(PythonLibsNew REQUIRED)
-find_package(NumPy REQUIRED)
+# Python and Numpy libraries
+find_package(Python3Alt REQUIRED)
 include(UseCython)
 
 include_directories(SYSTEM ${NUMPY_INCLUDE_DIRS} ${PYTHON_INCLUDE_DIRS} src)
@@ -577,7 +576,7 @@ foreach(module ${CYTHON_EXTENSIONS})
                                      "CYTHON_TRACE=1;CYTHON_TRACE_NOGIL=1")
   endif()
 
-  target_link_libraries(${module_name} ${LINK_LIBS})
+  target_link_libraries(${module_name} PRIVATE ${LINK_LIBS})
 
   # Generated files will be moved to the right directory by setup.py.
 endforeach(module)
@@ -585,25 +584,25 @@ endforeach(module)
 # Additional link libraries
 
 if(PYARROW_BUILD_CUDA)
-  target_link_libraries(_cuda ${CUDA_LINK_LIBS})
+  target_link_libraries(_cuda PRIVATE ${CUDA_LINK_LIBS})
 endif()
 
 if(PYARROW_BUILD_FLIGHT)
-  target_link_libraries(_flight ${FLIGHT_LINK_LIBS})
+  target_link_libraries(_flight PRIVATE ${FLIGHT_LINK_LIBS})
 endif()
 
 if(PYARROW_BUILD_DATASET)
-  target_link_libraries(_dataset ${DATASET_LINK_LIBS})
+  target_link_libraries(_dataset PRIVATE ${DATASET_LINK_LIBS})
 endif()
 
 if(PYARROW_BUILD_GANDIVA)
-  target_link_libraries(gandiva ${GANDIVA_LINK_LIBS})
+  target_link_libraries(gandiva PRIVATE ${GANDIVA_LINK_LIBS})
 endif()
 
 if(PYARROW_BUILD_PARQUET)
-  target_link_libraries(_parquet ${PARQUET_LINK_LIBS})
+  target_link_libraries(_parquet PRIVATE ${PARQUET_LINK_LIBS})
 endif()
 
 if(PYARROW_BUILD_PLASMA)
-  target_link_libraries(_plasma ${PLASMA_LINK_LIBS})
+  target_link_libraries(_plasma PRIVATE ${PLASMA_LINK_LIBS})
 endif()

run-cmake-format.py

@@ -60,6 +60,7 @@ patterns = [
     'cpp/cmake_modules/SetupCxxFlags.cmake',
     'cpp/cmake_modules/ThirdpartyToolchain.cmake',
     'cpp/cmake_modules/san-config.cmake',
+    'cpp/cmake_modules/UseCython.cmake',
     'cpp/src/**/CMakeLists.txt',
     'cpp/tools/**/CMakeLists.txt',
     'java/gandiva/CMakeLists.txt',