Re: [PATCH rdma-core v3 4/6] pyverbs: Add dma-buf based MR support

On Fri, Nov 27, 2020 at 12:55:41PM -0800, Jianxin Xiong wrote:

+function(rdma_multifile_module PY_MODULE MODULE_NAME LINKER_FLAGS)

I think just replace rdma_cython_module with this? No good reason I can see to have two APIs?

• set(ALL_CFILES "")
• foreach(SRC_FILE ${ARGN})
• get_filename_component(FILENAME ${SRC_FILE} NAME_WE)
• get_filename_component(DIR ${SRC_FILE} DIRECTORY)
• get_filename_component(EXT ${SRC_FILE} EXT)
• if (DIR)

•  set(SRC_PATH "${CMAKE_CURRENT_SOURCE_DIR}/${DIR}")
  

• else()

•  set(SRC_PATH "${CMAKE_CURRENT_SOURCE_DIR}")
  

• endif()
• if (${EXT} STREQUAL ".pyx")

•  set(PYX "${SRC_PATH}/${FILENAME}.pyx")
  

•  set(CFILE "${CMAKE_CURRENT_BINARY_DIR}/${FILENAME}.c")
  

•  include_directories(${PYTHON_INCLUDE_DIRS})
  

•  add_custom_command(
  

•    OUTPUT "${CFILE}"
  

•    MAIN_DEPENDENCY "${PYX}"
  

•    COMMAND ${CYTHON_EXECUTABLE} "${PYX}" -o "${CFILE}"
  

•    "-I${PYTHON_INCLUDE_DIRS}"
  

•    COMMENT "Cythonizing ${PYX}"
  

•  )
  

•  set(ALL_CFILES "${ALL_CFILES};${CFILE}")
  

• elseif(${EXT} STREQUAL ".c")

•  set(CFILE_ORIG "${SRC_PATH}/${FILENAME}.c")
  

•  set(CFILE "${CMAKE_CURRENT_BINARY_DIR}/${FILENAME}.c")
  

•  if (NOT ${CFILE_ORIG} STREQUAL ${CFILE})
  

•    rdma_create_symlink("${CFILE_ORIG}" "${CFILE}")
  

•  endif()
  

Why does this need the create_symlink? The compiler should work OK from the source file?

•  set(ALL_CFILES "${ALL_CFILES};${CFILE}")
  

• elseif(${EXT} STREQUAL ".h")

•  set(HFILE_ORIG "${SRC_PATH}/${FILENAME}.h")
  

•  set(HFILE "${CMAKE_CURRENT_BINARY_DIR}/${FILENAME}.h")
  

•  if (NOT ${HFILE_ORIG} STREQUAL ${HFILE})
  

•    rdma_create_symlink("${HFILE_ORIG}" "${HFILE}")
  

Here too? You probably don't need to specify h files at all, at worst they should only be used with publish_internal_headers

•  endif()
  

• else()

•  continue()
  

• endif()
• endforeach()
• string(REGEX REPLACE "\.so$" "" SONAME "${MODULE_NAME}${CMAKE_PYTHON_SO_SUFFIX}")
• add_library(${SONAME} SHARED ${ALL_CFILES})
• set_target_properties(${SONAME} PROPERTIES
• COMPILE_FLAGS "${CMAKE_C_FLAGS} -fPIC -fno-strict-aliasing -Wno-unused-function -Wno-redundant-decls -Wno-shadow -Wno-cast-function-type -Wno-implicit-fallthrough -Wno-unknown-warning -Wno-unknown-warning-option -Wno-deprecated-declarations ${NO_VAR_TRACKING_FLAGS}"

Ugh, you copy and pasted this, but it shouldn't have existed in the first place. Compiler arguments like this should not be specified manually. I should fix it..

Also you should cc edward on all this pyverbs stuff, he knows it all very well

It all looks reasonable to me

Jason