PIP-209: Compile Python client wrapper#1
Conversation
BewareMyPower
left a comment
BewareMyPower
left a comment
There was a problem hiding this comment.
I think we still needs to import https://github.com/apache/pulsar-client-cpp as the submodule.
If the new feature is ready in C++ client, we don't have to wait a new C++ client release. It would also be better for those want to build Python library from source.
|
In addition, some tests in |
+1 |
|
I'm going to work for the submodule way. The workflow (for CI in future) is:
Regarding the dependency on |
Will this cause the python version to be unstable? Are all versions of python built on top of unreleased versions of c++? |
|
@shibd No. It's a submodule, we need an extra PR to update the commit of the submodule if the upstream C++ client has introduced a new feature. See https://github.com/grpc/grpc/tree/master/third_party for example. |
|
Suddenly curious, will we print log the version that depends on C++ at startup? Case Log: |
I think it's unnecessary. Just like whether would you print the version of all dependencies in Java. |
| ############################################################################### | ||
| if [ ! -f apache-pulsar-${PULSAR_VERSION}-src/.done ]; then | ||
| echo "Building Pulsar C++ client - ${PULSAR_VERSION}" | ||
| curl -O -L https://archive.apache.org/dist/pulsar/pulsar-${PULSAR_VERSION}/apache-pulsar-${PULSAR_VERSION}-src.tar.gz |
There was a problem hiding this comment.
we should probably add some validation of the integrity of the archive
and also provide a way to pass a custom .tar.gz, in order to ease development/testing with a different version
maybe making the full URL configurable will help
we can do these improvements as a follow up patch
There was a problem hiding this comment.
Yes, we could be checking the expected hash of the archive
BewareMyPower
left a comment
BewareMyPower
left a comment
There was a problem hiding this comment.
Since this PR involved code changes, could you copy the .clang-format file from the pulsar-client-cpp repo?
CMakeLists.txt
Outdated
| target_link_libraries(_pulsar -Wl,-all_load pulsarStatic ${PYTHON_WRAPPER_LIBS} ${COMMON_LIBS} ${ICU_LIBS}) | ||
| target_link_libraries(_pulsar -Wl,-all_load ${PYTHON_WRAPPER_LIBS} ${COMMON_LIBS} ${ICU_LIBS}) | ||
| else () | ||
| if (NOT MSVC) | ||
| set (CMAKE_SHARED_LINKER_FLAGS " -static-libgcc -static-libstdc++") | ||
| endif() | ||
| target_link_libraries(_pulsar pulsarStatic ${PYTHON_WRAPPER_LIBS} ${COMMON_LIBS}) | ||
| target_link_libraries(_pulsar ${PYTHON_WRAPPER_LIBS} ${COMMON_LIBS}) |
There was a problem hiding this comment.
COMMON_LIBS can be removed because it's originally defined in CMakeLists.txt of pulsar-client-cpp. ICU_LIBS can be removed as well. (I never found a reference)
5 participants
No description provided.