[GitHub] [thrift] fishy commented on a diff in pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x

["fishy (via GitHub)" <gi...@apache.org>]

fishy commented on code in PR #2787:
URL: https://github.com/apache/thrift/pull/2787#discussion_r1171613064


##########
.github/workflows/build.yml:
##########
@@ -344,12 +344,79 @@ jobs:
       - name: Run make test_recursive for rust
         run: make -C lib/rs/test_recursive check
 
+  lib-python:
+    needs: compiler
+    runs-on: ubuntu-20.04
+    strategy:
+      matrix:
+        python-version: ["2.x", "3.x"]
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update -yq
+          sudo apt-get install -y --no-install-recommends $BUILD_DEPS
+          sudo apt-get install -y --no-install-recommends curl openssl ca-certificates
+
+      - name: Set up Python
+        uses: actions/setup-python@v3
+        with:
+          python-version: ${{ matrix.python-version }}
+
+      - name: Python setup
+        run: |
+          python -m pip install --upgrade pip setuptools wheel flake8 tornado twisted zope.interface
+          python --version
+          pip --version
+
+      - name: Python 2.x backport setup
+        if: matrix.python-version == '2.x'
+        run: |
+          python -m pip install --upgrade ipaddress backports.ssl_match_hostname
+
+      - name: Run bootstrap
+        run: ./bootstrap.sh
+
+      - name: Run configure 2.x
+        if: matrix.python-version == '2.x'
+        run: |
+          ./configure $(echo $CONFIG_ARGS_FOR_LIBS | sed 's/without-python/with-python/')
+
+      - name: Run configure 3.x
+        if: matrix.python-version != '2.x'
+        run: |
+          ./configure $(echo $CONFIG_ARGS_FOR_LIBS | sed 's/without-py3/with-py3/')
+
+      - uses: actions/download-artifact@v3
+        with:
+          name: thrift-compiler
+          path: compiler/cpp
+
+      - name: Run thrift-compiler
+        run: |
+          chmod a+x compiler/cpp/thrift
+          compiler/cpp/thrift -version
+
+      - name: Run make for python
+        run: make -C lib/py
+
+      - name: Run make install for python
+        run: sudo make -C lib/py install
+
+      # - name: Run make install-exec-hook for python
+      #   run: sudo make -C lib/py install-exec-hook
+
+      - name: Run make check for python
+        run: make -C lib/py check

Review Comment:
   we actually have two sets of tests, one under `lib/py` (which is the one here) and another under `test/py`. can you also add a step to run `make -C test/py check`?



##########
lib/py/src/server/TNonblockingServer.py:
##########
@@ -268,7 +268,7 @@ def prepare(self):
         self.socket.listen()
         for _ in range(self.threads):
             thread = Worker(self.tasks)
-            thread.setDaemon(True)
+            thread.daemon = True

Review Comment:
   🔕 oh I see `setDaemon` was deprecated since python 3.10.



##########
.github/workflows/build.yml:
##########
@@ -344,12 +344,79 @@ jobs:
       - name: Run make test_recursive for rust
         run: make -C lib/rs/test_recursive check
 
+  lib-python:
+    needs: compiler
+    runs-on: ubuntu-20.04
+    strategy:
+      matrix:
+        python-version: ["2.x", "3.x"]
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update -yq
+          sudo apt-get install -y --no-install-recommends $BUILD_DEPS
+          sudo apt-get install -y --no-install-recommends curl openssl ca-certificates
+
+      - name: Set up Python
+        uses: actions/setup-python@v3
+        with:
+          python-version: ${{ matrix.python-version }}
+
+      - name: Python setup
+        run: |
+          python -m pip install --upgrade pip setuptools wheel flake8 tornado twisted zope.interface
+          python --version
+          pip --version
+
+      - name: Python 2.x backport setup
+        if: matrix.python-version == '2.x'
+        run: |
+          python -m pip install --upgrade ipaddress backports.ssl_match_hostname
+
+      - name: Run bootstrap
+        run: ./bootstrap.sh
+
+      - name: Run configure 2.x
+        if: matrix.python-version == '2.x'
+        run: |
+          ./configure $(echo $CONFIG_ARGS_FOR_LIBS | sed 's/without-python/with-python/')
+
+      - name: Run configure 3.x
+        if: matrix.python-version != '2.x'
+        run: |
+          ./configure $(echo $CONFIG_ARGS_FOR_LIBS | sed 's/without-py3/with-py3/')
+
+      - uses: actions/download-artifact@v3
+        with:
+          name: thrift-compiler
+          path: compiler/cpp
+
+      - name: Run thrift-compiler
+        run: |
+          chmod a+x compiler/cpp/thrift
+          compiler/cpp/thrift -version
+
+      - name: Run make for python
+        run: make -C lib/py
+
+      - name: Run make install for python
+        run: sudo make -C lib/py install
+
+      # - name: Run make install-exec-hook for python
+      #   run: sudo make -C lib/py install-exec-hook
+
+      - name: Run make check for python
+        run: make -C lib/py check
+
   cross-test:
     needs:
       - lib-java-kotlin
       - lib-swift
       - lib-rust
       - lib-go
+      - lib-python

Review Comment:
   we actually also need to add python (I'm not sure if it's gonna be `py3` or `python`) to line 483 (`PRECROSS_LANGS`) to actually run the cross tests with 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.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

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