PIP-209: Compile Python client wrapper by merlimat · Pull Request #1 · apache/pulsar-client-python

merlimat · 2022-09-29T04:43:03Z

No description provided.

BewareMyPower

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.

BewareMyPower · 2022-09-29T06:17:20Z

In addition, some tests in pulsar_test.py might already rely on the unreleased C++ APIs as you said here.

Demogorgon314 · 2022-09-29T06:28:26Z

I think we still needs to import https://github.com/apache/pulsar-client-cpp as the submodule.

+1

BewareMyPower · 2022-09-29T07:03:51Z

I'm going to work for the submodule way. The workflow (for CI in future) is:

Clone the pulsar-client-cpp submodule.
Add a build-client-cpp.sh to build the static library in pulsar-client-cpp.
Modify the CMakeLists.txt to find the C++ static library, instead of the previous pulsarStatic target.

Regarding the dependency on lib/*.h, we can remove them later.

shibd · 2022-09-29T07:26:37Z

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.

Will this cause the python version to be unstable? Are all versions of python built on top of unreleased versions of c++?

BewareMyPower · 2022-09-29T07:36:18Z

@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.

shibd · 2022-09-29T07:42:23Z

Suddenly curious, will we print log the version that depends on C++ at startup?

Case Log:

pulsar-client-python: x.x.x
depends pulsar-client-cpp: x.x.x or master@1d5321f

BewareMyPower · 2022-09-29T07:45:23Z

will we print log the version that depends on C++ at startup?

I think it's unnecessary. Just like whether would you print the version of all dependencies in Java.

eolivelli · 2022-09-29T09:46:13Z

+###############################################################################
+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


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

Yes, we could be checking the expected hash of the archive

BewareMyPower

Since this PR involved code changes, could you copy the .clang-format file from the pulsar-client-cpp repo?

BewareMyPower · 2022-09-30T04:30:12Z

- 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})


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)

PIP-209: Compile Python client wrapper
c46d801

merlimat mentioned this pull request Sep 29, 2022
PIP-209: Removed Python client wrapper apache/pulsar-client-cpp#1
Merged

BewareMyPower requested changes Sep 29, 2022
View reviewed changes

eolivelli reviewed Sep 29, 2022
View reviewed changes

BewareMyPower mentioned this pull request Sep 29, 2022
Use git submodule to download pulsar-client-cpp dependency #3
Closed

BewareMyPower requested changes Sep 30, 2022
View reviewed changes

Comment threadCMakeLists.txt Outdated

BewareMyPower requested changes Sep 30, 2022
View reviewed changes

Comment threadsrc/utils.h Outdated

BewareMyPower requested changes Sep 30, 2022
View reviewed changes

BewareMyPower reviewed Sep 30, 2022
View reviewed changes

Comment threadbuild-mac-wheels.sh

merlimat added 4 commits September 30, 2022 08:22

Removed Optional
718602e

Fixed CMakefile
7f289ec

Added __pycache__ to gitignore
112dd6f

Added clang-format support
0b3b5c0

BewareMyPower approved these changes Sep 30, 2022
View reviewed changes

merlimat merged commit 9f6fc69 into apache:mainSep 30, 2022

merlimat deleted the build branch September 30, 2022 19:35

BewareMyPower added this to the 3.0.0 milestone Dec 14, 2022

Uh oh!

Conversation

merlimat commented Sep 29, 2022

Uh oh!

BewareMyPower left a comment

Choose a reason for hiding this comment

Uh oh!

BewareMyPower commented Sep 29, 2022

Uh oh!

Demogorgon314 commented Sep 29, 2022

Uh oh!

BewareMyPower commented Sep 29, 2022

Uh oh!

shibd commented Sep 29, 2022

Uh oh!

BewareMyPower commented Sep 29, 2022

Uh oh!

shibd commented Sep 29, 2022

Uh oh!

BewareMyPower commented Sep 29, 2022

Uh oh!

eolivelliSep 29, 2022

Choose a reason for hiding this comment

Uh oh!

merlimatSep 30, 2022

Choose a reason for hiding this comment

Uh oh!

BewareMyPower left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BewareMyPowerSep 30, 2022

Choose a reason for hiding this comment

Uh oh!

merlimatSep 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants