> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Monday, November 30, 2020 8:08 AM
> To: Xiong, Jianxin <jianxin.xiong@xxxxxxxxx>
> Cc: linux-rdma@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leon@xxxxxxxxxx>; Sumit Semwal <sumit.semwal@xxxxxxxxxx>; Christian Koenig <christian.koenig@xxxxxxx>; Vetter, Daniel
> <daniel.vetter@xxxxxxxxx>
> Subject: 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?
rdma_cython_module can handle many modules, but this one is for a single module.
If you agree, I can merge the two by slightly tweaking the logic: each module starts
with a .pyx file, followed by 0 or more .c and .h files.
>
> > + 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?
You are right, the link for .c is not necessary, but the link for .h is needed.
>
> > + 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
Without the .h link, the compiler fail to find the header file (both dmabuf_alloc.c and the generated "dmabuf.c" contain #include "dmabuf_alloc.h").
>
> > + 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
Will add Edward next time. He commented a lot on the PR at github. The current github PR
is in sync with this version.
>
> It all looks reasonable to me
>
> Jason
