You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/29 08:33:55 UTC
[GitHub] [pulsar-client-python] BewareMyPower opened a new pull request, #3: PIP-209: Support build Python client wrapper
BewareMyPower opened a new pull request, #3:
URL: https://github.com/apache/pulsar-client-python/pull/3
### Modifications
- Add [pulsar-client-cpp](https://github.com/apache/pulsar-client-cpp) as the submodule
- Add `build-client-cpp.sh` to build the Pulsar C++ client.
- Fix the CMakeLists.txt to make Python client build successfully. Instead of linking the `pulsarStatic` target, link to the static library built by `build-client-cpp.sh`.
- Add README to describe how to build Python client.
Currently the Python client uses some C++ headers not exposed (i.e. in the `pulsar-client-cpp/lib` directory). This PR added the `pulsar-client-cpp` path prefix. We should remove these includes in future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] tisonkun commented on pull request #3: Use git submodule to download pulsar-client-cpp dependency
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1264207597
> thinking again
Agree. Submodule increase understanding burdens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1263071984
Since most of the codes will be the same with #1, I marked this PR as draft and will make further changes after #1 is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower closed pull request #3: Use git submodule to download pulsar-client-cpp dependency
Posted by GitBox <gi...@apache.org>.
BewareMyPower closed pull request #3: Use git submodule to download pulsar-client-cpp dependency
URL: https://github.com/apache/pulsar-client-python/pull/3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1262362362
@RobertIndie @merlimat @eolivelli Could you take a second look? I believe this PR is more universal than #1 that focuses on the script to build Python client on macOS. This PR also avoids many code changes (include CMakeLists.txt)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] merlimat commented on pull request #3: Use git submodule to download pulsar-client-cpp dependency
Posted by GitBox <gi...@apache.org>.
merlimat commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1263708417
> When we release a Python client, we can specify the tag like
Exactly, but then it would be the same as depending on a released version of C++ client. Having the submodule won't help here.
> if we want to support a new feature, we have to release the C++ client first.
Yes, if the feature is significant enough to grant a C++ release, then we just do the 2 releases (and also NodeJS and maybe other wrappers).
If the feature is not big enough, we can wait to include other changes in C++ before releasing. We would still be in a completely different situation from where we are today, where the Python release is tied to a huge Pulsar release that includes everything.
If we automate all the steps, we are now in a position to do C++ release with minimal effort, therefore we can do them very frequently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] merlimat commented on a diff in pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#discussion_r983734637
##########
README.md:
##########
@@ -0,0 +1,72 @@
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+
+# Pulsar Python client library
Review Comment:
The README should be focused on users, rather than developers.
We should list all the platforms/OS/Python versions that are supported, how to install the wheels and how to get started.
We can a have a link to a developer page where there are instructions to compile it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1263096492
> We now have the freedom of doing releases with much less overhead and we can release new features much quicker than before.
I'm wondering how to manage the releases for C++ client now. If the main branch must depend on a released C++ client, then, if we want to support a new feature, we have to release the C++ client first. Then **each C++ feature requires a release**. I'm afraid that it's not a good way for release even if the release could be much quicker than before. We might have:
- pulsar-cpp-3.0.0-pre-feature1
- pulsar-cpp-3.0.0-pre-feature2
- pulsar-cpp-3.0.0-pre-feature3
- ...
Adding a git tag for them might be good (but I think it could still create a lot of tags). But should we create each binary uploaded for **a single feature**? @merlimat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on a diff in pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#discussion_r984173337
##########
README.md:
##########
@@ -0,0 +1,72 @@
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+
+# Pulsar Python client library
Review Comment:
The documents here are only related to changes of this PR. We should use another PR to add more content into README.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on a diff in pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#discussion_r983285929
##########
README.md:
##########
@@ -0,0 +1,72 @@
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+
+# Pulsar Python client library
+<!-- TOC depthFrom:2 depthTo:3 withLinks:1 updateOnSave:1 orderedList:0 -->
+
+## How to build it from source
Review Comment:
The submodule download logic is included in `build-client-cpp.sh`. To get it started, users don't have to know these details, as well as the the CMake options in this script.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] Demogorgon314 commented on a diff in pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#discussion_r983280815
##########
README.md:
##########
@@ -0,0 +1,72 @@
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
+
+# Pulsar Python client library
+<!-- TOC depthFrom:2 depthTo:3 withLinks:1 updateOnSave:1 orderedList:0 -->
+
+## How to build it from source
Review Comment:
We can add one step to let users know when we clone this repo, we also need to include submodules.
```
git clone --recurse-submodules https://github.com/apache/pulsar-client-python.git
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on pull request #3: Use git submodule to download pulsar-client-cpp dependency
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1263868678
> Having the submodule won't help here.
I have thought that we can depend on an unstable version of pulsar-client-cpp in main branch, then for those want some specific features (or bug fixes), they can build the Python client from source.
But just thinking again, they can also build the unreleased C++ client from source. Then build the unreleased Python client from source based on the C++ client built by themselves. But this way should not be encouraged anyway unless the demands are very urgent.
It looks like Pulsar and BK, but C++ client can be released very quickly, not like BK. So I will close this PR.
FYI, @codelipenghui @Demogorgon314 @shibd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-python] BewareMyPower commented on pull request #3: PIP-209: Support build Python client wrapper
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #3:
URL: https://github.com/apache/pulsar-client-python/pull/3#issuecomment-1263062569
Hi @merlimat, here are my points for some of your concerns.
For the most important issue (IMO) you've mentioned
> We must not use a random version from C++ client main branch.
The main branch is not guaranteed to be stable. We should only depend the stable version of C++ client for formal releases. For example, git submodule can specify a tag of a formal C++ client release. When we release a Python client, we can specify the tag like
```
[submodule "pulsar-client-cpp"]
path = pulsar-client-cpp
url = https://github.com/apache/pulsar-client-cpp.git
tag = v3.0.0-pre-release-1
```
The main branch should also relies on a tag. However, there is no tag in the new created pulsar-client-cpp repo. Should we create one and rely on it? Or I think we can reset to a specific commit in the script here.
----
> When we're building a source tar.gz we would have to include C++ code
> The Python wrapper shouldn't depend on any internal code from the C++ library, just the public API
I agree. Just want to remove the dependency in another PR. I will do that in this PR soon.
> When building Python client, we shouldn't be building the C++ client lib. Instead we should just fetch the binaries that are already available (eg: RPM, Deb, APK, etc.. )
However, it makes the release or Python client slow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org