Conversation
merlimat
requested review from
BewareMyPower,
Demogorgon314,
RobertIndie and
nodece
BewareMyPower
left a comment
BewareMyPower
left a comment
There was a problem hiding this comment.
We should use static Boost.Python library.
| tar xfz boost_${BOOST_VERSION_UNDESRSCORE}.tar.gz && \ | ||
| cd boost_${BOOST_VERSION_UNDESRSCORE} && \ | ||
| ./bootstrap.sh --with-libraries=python && \ | ||
| ./b2 -d0 address-model=64 cxxflags=-fPIC link=shared threading=multi variant=release install && \ |
There was a problem hiding this comment.
| ./b2 -d0 address-model=64 cxxflags=-fPIC link=shared threading=multi variant=release install && \ | |
| ./b2 -d0 address-model=64 cxxflags=-fPIC link=static threading=multi variant=release install && \ |
There was a problem hiding this comment.
It’s on purpose. The Linux build is linking to the shared libs and auditwheel will include them into the wheel archive.
It’s actually preferable to use the libpulsar.so (with all the other deps) because it will hide the symbols inside the so. If we use the .a, everything is visible and subject to symbols conflict (eg if you load another Python extension that uses this symbols)
There was a problem hiding this comment.
Also, there is a check for the resulting wheels to be loaded in a fresh container. That will protect against missing dependencies or linking errors
There was a problem hiding this comment.
Yeah, I see auditwheel includes the libboost_python.so and libpulsar.so.
(eg if you load another Python extension that uses this symbols)
But I don't understand when will this conflict happen. The deps or libpulsar.so are still statically linked. Could you explain more?
There was a problem hiding this comment.
Anyway. It's not a blocker for this PR. This way works as it's protected by test. I will check the macOS build soon.
There was a problem hiding this comment.
When we link libpulsar.so, all symbols are by default hidden inside, except for the ones we want to expose (pulsar public API). This avoid conflicts if the same binary is linking with a different version of protobuf or ssl.
In the .a it’s not possible to control the symbols visibility.
There was a problem hiding this comment.
Thanks for your explanation! It's clear to me now.
I have another question that should macOS link to libpulsar.dylib as well? Currently when LINK_STATIC is ON, the libpulsar.a will be linked. And the 3rd party dependencies are all static libraries.
There was a problem hiding this comment.
The problem is there is no “auditwheel” for Mac. Maybe we can find a way to manually add the dylib inside the wheel in the same way
There was a problem hiding this comment.
Got it. But I think the current packaging way for macOS should be okay for now though it has the possibility for symbol conflict. BTW, I found https://pypi.org/project/delocate/ might be similar to auditwheel.
|
LGTM. Great job! |
### Motivation Currently the Python client cannot be built with the default apt sources on Ubuntu 20.04, whose default CMake version is 3.16. However, even after I installed CMake 3.24 on Ubuntu 20.04, CMake would still fail to find Boost.Python. ### Modifications To fix the Boost.Python not found issue, find the `Python` component instead of `Python3` when finding Boost. In addition, when finding Python3, find the `Development` component instead of the `Development.Module`, which is a sub-component of `Development`. See https://cmake.org/cmake/help/latest/module/FindPython3.html. After that, the minimum required CMake version becomes back to 3.12 from 3.18, which was upgraded in apache#11.
2 participants
Motivation
Added CI jobs to build wheel files for Mac and Linux.
Modifications
CMakeList.txtfor easier detection of Boost-Python