Skip to content

Commit

Permalink
Fixing fbthrift and folly cython bindings build
Browse files Browse the repository at this point in the history
Summary:
This diff addresses a collection of issues with our thrift-py3 build story in open source.
- Cython 0.29 bug cython/cython#2985 prevents us from loading the generated binary python extensions. I've tested 0.28.6 and it works perfectly.
- Adds ssl to the extensions (since we need SSLContext in get_client, we must have access to the module)
- No need to special case folly_pic. There is really no easy way to get the python bindings to work without using shared library libfolly.so. So, In our build (logdevice) we will build folly as a shared library, this also reduces the complexity everywhere.
- We also need iobuf as extension for thrift to work, added that to setup.py
- Refactored some of the CMake code that had hardcoded file names to use globbing to future-proof this as much as possible.
- Moved `python/lang/cast.pxd` to match the python module name `folly.cast`, so now it lives in `python/cast.pxd`. This simplifies the globbing as well.
- Moved `setup.py` to live in the `/python` directory.
- Added `*.py` in setup.py to pickup any raw python code (including `__init__.py`)

Reviewed By: yfeldblum

Differential Revision: D18685009

fbshipit-source-id: 749b30942a3e5e9e314b5248cc0aa90c6ac1581f
  • Loading branch information
AhmedSoliman authored and facebook-github-bot committed Nov 29, 2019
1 parent 3a7fa5d commit 33e3376
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 3 deletions.
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ endif(lib_only OR build_all)
if(thriftpy3)
find_package(PythonInterp 3.6 REQUIRED)
find_package(PythonLibs 3 REQUIRED)
find_package(Cython 0.29 REQUIRED)
find_package(Cython 0.28 REQUIRED)
endif()

# Add the test dependencies
Expand Down
2 changes: 1 addition & 1 deletion thrift/lib/cpp2/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ if(thriftpy3)
PUBLIC
fizz::fizz
wangle::wangle
Folly::folly_pic
Folly::folly
sodium
${LIBGFLAGS_LIBRARY}
)
Expand Down
1 change: 1 addition & 0 deletions thrift/lib/py3/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ set(cpp_tgts
"thrift/py3/serializer.cpp"
"thrift/py3/client.cpp"
"thrift/py3/server.cpp"
"thrift/py3/ssl.cpp"
)
foreach(_mod ${cpp_tgts})
get_filename_component(_f ${_mod} NAME_WE)
Expand Down
3 changes: 2 additions & 1 deletion thrift/lib/py3/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,14 @@ def build_extension(self, ext):
Extension("thrift.py3.serializer", sources=["thrift/py3/serializer.so"]),
Extension("thrift.py3.client", sources=["thrift/py3/client.so"]),
Extension("thrift.py3.server", sources=["thrift/py3/server.so"]),
Extension("thrift.py3.ssl", sources=["thrift/py3/ssl.so"]),
]
setup(
name="thrift",
cmdclass={"build_ext": copy_cmake_built_libs_build_ext},
version="0.0.1",
packages=["thrift", "thrift.py3"],
package_data={"": ["*.pxd", "*.h", "__init__.pyx"]},
package_data={"": ["*.pxd", "*.h", "__init__.pyx", "*.py"]},
zip_safe=False,
ext_modules=extensions,
)

0 comments on commit 33e3376

Please sign in to comment.