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 2021/02/23 10:57:11 UTC

[GitHub] [pulsar] BewareMyPower opened a new pull request #9684: [CI][C++] Use Boost.Python for Python 3

BewareMyPower opened a new pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684


   Fixes #9682 
   
   ### Motivation
   
   Currently, ci-cpp-tests uses `pulsar-build` image that is from `ubuntu:16.04` to build C++/Python client. The image uses `libboost-all-dev` for CMake to find boost dependencies. However, the Boost.Python library from Ubuntu 16.04's apt source only supports Python 2.
   
   After the change of #9609 , CMake found Python 3 by default and then CMake cannot find the Boost Python library.
   
   ### Modifications
   
   - Modify the related `Dockerfile` to install Boost dependency from source.
   
   ### Verifying this change
   
   - [ ] Make sure that the change passes the CI checks.
   
   This change is a trivial rework / code cleanup without any test coverage.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784272714


   OK, I may open a new PR to do it. And I'll keep this PR and the associated branch to do the task of PIP 56.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784117364


   As I mentioned in #9682 , though I've tested the new image in my local environment, currently there's no way to verify the change of `Dockerfile`.
   
   I've just pushed the image to my dockerhub: https://hub.docker.com/r/bewaremypower/pulsar-build/tags?page=1&ordering=last_updated
   
   I'm not sure if it's proper to use the image for test. And after when all tests passed, those who have the permission of `apachepulsar`'s permissions push the latest image.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784120198


   There's another concern that is bintray will shut down after May 2021, see https://jfrog.com/blog/into-the-sunset-bintray-jcenter-gocenter-and-chartcenter/ for details.
   
   However, the [Boost official site](https://www.boost.org/users/history/version_1_75_0.html) uses bintray to download Boost source.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784265376


   @eolivelli Removing Python 2 support is a task of [PIP 56](https://github.com/apache/pulsar/wiki/PIP-56:-Python3-Migration) and [issue 8622](https://github.com/apache/pulsar/pull/8622). Before the change of `pulsar-build` image, there were some attempts to remove Python 2, like https://github.com/apache/pulsar/pull/8624, https://github.com/apache/pulsar/pull/9268
   
   If it's not proper to do it in this PR, we need to find another way to fix the cpp/python tests. Then we cannot use Boost.Python for Python3 and should let CMake find python2 instead of python3.
   
   But eventually, we need to remove the Python2 related code and tests. So I just wanted to do it in this PR before.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codelipenghui commented on pull request #9684: [WIP][PIP-56] Migrate the python2.7 to python3.7

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-1058893103


   The pr had no activity for 30 days, mark with Stale label.


-- 
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] BewareMyPower commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784158870


   The python tests may fail because the new image use `pip3` instead of `pip`, I'll fix it soon.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [WIP][PIP-56] Migrate the python2.7 to python3.7

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784676015


   Since I've opened #9690 to fix the CI issue, I keeps this PR for PIP-56, which will remove Python2 from pulsar.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on a change in pull request #9684: [CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#discussion_r580950383



##########
File path: pulsar-client-cpp/docker-build.sh
##########
@@ -26,7 +26,7 @@ set -e
 ROOT_DIR=$(git rev-parse --show-toplevel)
 cd $ROOT_DIR/pulsar-client-cpp
 
-BUILD_IMAGE_NAME="${BUILD_IMAGE_NAME:-apachepulsar/pulsar-build}"
+BUILD_IMAGE_NAME="${BUILD_IMAGE_NAME:-bewaremypower/pulsar-build}"

Review comment:
       Yeah, after the CI passed. @zymap will push the latest image to `apachepulsar`'s dockerhub and I'll use the latest image to test again.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower edited a comment on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784265376


   @eolivelli Removing Python2 support is a task of [PIP 56](https://github.com/apache/pulsar/wiki/PIP-56:-Python3-Migration) and [issue 8622](https://github.com/apache/pulsar/pull/8622). Before the change of `pulsar-build` image, there were some attempts to remove Python2, like https://github.com/apache/pulsar/pull/8624, https://github.com/apache/pulsar/pull/9268
   
   If it's not proper to do it in this PR, we need to find another way to fix the cpp/python tests. Then we cannot use Boost.Python for Python3 and should let CMake find Python2 instead of Python3.
   
   But eventually, we need to remove the Python2 related code and tests. So I just wanted to do it in this PR before.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower edited a comment on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784265376


   @eolivelli Removing Python2 support is a task of [PIP 56](https://github.com/apache/pulsar/wiki/PIP-56:-Python3-Migration) and [issue 8622](https://github.com/apache/pulsar/pull/8622). Before the change of `pulsar-build` image, there were some attempts to remove Python2, like https://github.com/apache/pulsar/pull/8624, https://github.com/apache/pulsar/pull/9268
   
   If it's not proper to do it in this PR, we need to find another way to fix the cpp/python tests. Then we cannot use Boost.Python for Python3 and should let CMake find Python2 instead of Python3.
   
   But eventually, we need to remove the Python2 related code and tests. So I just wanted to do it in this PR.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784204232


   I think it still needs some code changes to make tests pass for Python3. So I'll also remove Python2 in this PR.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] github-actions[bot] commented on pull request #9684: [WIP][PIP-56] Migrate the python2.7 to python3.7

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-1058893346


   @BewareMyPower:Thanks for your contribution. For this PR, do we need to update docs?
   (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)


-- 
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] eolivelli commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784381592


   @merlimat I believe that @zymap already did it a few hours ago and he unblocked the release.
   this PR is using the image from @BewareMyPower 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] BewareMyPower commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784250308


   After the latest commit, the python client tests (`pulsar_test.py`) could pass in my docker image. However, there're still a lot to do with python functions.
   
   For example:
   
   ```
   ======================================================================
   ERROR: test_python_instance (unittest.loader._FailedTest)
   ----------------------------------------------------------------------
   ImportError: Failed to import test module: test_python_instance
   Traceback (most recent call last):
     File "/usr/lib/python3.5/unittest/loader.py", line 428, in _find_test_path
       module = self._get_module_from_name(name)
     File "/usr/lib/python3.5/unittest/loader.py", line 369, in _get_module_from_name
       __import__(name)
     File "/pulsar/pulsar-functions/instance/target/python-instance/tests/test_python_instance.py", line 23, in <module>
       from mock import Mock
     File "/root/.local/lib/python3.5/site-packages/mock/__init__.py", line 7, in <module>
       import mock.mock as _mock
     File "/root/.local/lib/python3.5/site-packages/mock/mock.py", line 765
       mock_name = f'{self._extract_mock_name()}.{name}'
                                                       ^
   SyntaxError: invalid syntax
   ```
   
   It looks like the [`mock` module](https://pypi.org/project/mock/) uses the [f-strings](https://realpython.com/python-f-strings/), which was introduced from Python 3.6. Therefore it requires Python >= 3.6. However the Ubuntu 16.04's python3 version is 3.5. So we need to find an alternative module.
   
   On the other hand, I found [python functions code](https://github.com/apache/pulsar/tree/master/pulsar-functions/instance/src/main/python) still use some python2 legacy code.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] merlimat commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784379946


   @BewareMyPower @eolivelli I'm going to re-push the old image to docker hub to unblock CI while the issue is being fixed.
   
   We should also probably switch to a "versioned" build image, so that we can validate the switch in a PR.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] eolivelli commented on a change in pull request #9684: [CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#discussion_r580949227



##########
File path: pulsar-client-cpp/docker-build.sh
##########
@@ -26,7 +26,7 @@ set -e
 ROOT_DIR=$(git rev-parse --show-toplevel)
 cd $ROOT_DIR/pulsar-client-cpp
 
-BUILD_IMAGE_NAME="${BUILD_IMAGE_NAME:-apachepulsar/pulsar-build}"
+BUILD_IMAGE_NAME="${BUILD_IMAGE_NAME:-bewaremypower/pulsar-build}"

Review comment:
       please revert this part.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] eolivelli commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784253625


   I believe we should stick to python2.
   We cannot change the Python client and the Python support for Pulsar Functions.
   
   This new image must be 100% compatible with the previous one regarding python
   
   I initially added python3 in order to make the build of the image pass, because of "getpip" script.
   
   can you try to revert all of the changes related to python ?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] eolivelli commented on a change in pull request #9684: [CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#discussion_r580949771



##########
File path: pulsar-client-cpp/docker-build.sh
##########
@@ -26,7 +26,7 @@ set -e
 ROOT_DIR=$(git rev-parse --show-toplevel)
 cd $ROOT_DIR/pulsar-client-cpp
 
-BUILD_IMAGE_NAME="${BUILD_IMAGE_NAME:-apachepulsar/pulsar-build}"
+BUILD_IMAGE_NAME="${BUILD_IMAGE_NAME:-bewaremypower/pulsar-build}"

Review comment:
       I mean, after we see that CI passed




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] eolivelli commented on pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#issuecomment-784270476


   Agreed.
   
   I also agree that one good way is to force cmake to find python2


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] merlimat commented on a change in pull request #9684: [WIP][CI][C++] Use Boost.Python for Python 3

Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #9684:
URL: https://github.com/apache/pulsar/pull/9684#discussion_r581225767



##########
File path: build/docker/Dockerfile
##########
@@ -28,11 +28,19 @@ RUN apt-get update && \
     add-apt-repository ppa:openjdk-r/ppa && \
     apt-get update && \
     apt-get install -y tig g++ cmake libssl-dev libcurl4-openssl-dev \
-                liblog4cxx-dev libprotobuf-dev libboost-all-dev google-mock libgtest-dev \
+                liblog4cxx-dev libprotobuf-dev google-mock libgtest-dev \
                 libjsoncpp-dev libxml2-utils protobuf-compiler wget \
                 curl doxygen openjdk-8-jdk-headless openjdk-11-jdk-headless clang-format-5.0 \
                 gnupg2 golang-1.13-go zip unzip libzstd-dev libsnappy-dev python3-pip
 
+# Compile and install Boost because Boost.Python for Python3 is required
+RUN apt-get install -y curl
+RUN cd /usr/src && curl -O -L https://dl.bintray.com/boostorg/release/1.75.0/source/boost_1_75_0.tar.gz && \
+    tar zxf boost_1_75_0.tar.gz && cd boost_1_75_0 && \
+    ./bootstrap.sh --with-python=/usr/bin/python3 --with-libraries=program_options,filesystem,regex,thread,system,python && \

Review comment:
       We're actually using less runtime deps at this point: 
   
   ```suggestion
       ./bootstrap.sh --with-python=/usr/bin/python3 --with-libraries=program_options,python && \
   ```




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org