You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Jimexist (via GitHub)" <gi...@apache.org> on 2023/04/14 15:26:49 UTC
[GitHub] [thrift] Jimexist opened a new pull request, #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Jimexist opened a new pull request, #2787:
URL: https://github.com/apache/thrift/pull/2787
<!-- Explain the changes in the pull request below: -->
- closes #2596
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on a diff in pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1509415653
> the 3.x one failed, we probably should also set [`fail-fast`](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast) to false so that one failure won't auto cancel other jobs.
> ModuleNotFoundError: No module named 'thrift'
could it be because of some path handling logic not being mirrored in the GitHub workflow 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.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jimexist merged pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist merged PR #2787:
URL: https://github.com/apache/thrift/pull/2787
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] fishy commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1508924468
the 3.x one failed, we probably should also set [`fail-fast`](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast) to false so that one failure won't auto cancel other jobs.
--
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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1511331030
i am not actually an expert in python library code of thrift and not sure why ssl is still failing. anyone wish to take a look?
--
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
[GitHub] [thrift] Jimexist commented on a diff in pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on code in PR #2787:
URL: https://github.com/apache/thrift/pull/2787#discussion_r1168337093
##########
.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"]
Review Comment:
> I would drop Python v2. Python 2 has been sunsetted 1st of January 2020: https://www.python.org/doc/sunset-python-2/
feel free to review https://github.com/apache/thrift/pull/2588
--
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
[GitHub] [thrift] fishy commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1511840231
>>ModuleNotFoundError: No module named 'thrift'
>could it be because of some path handling logic not being mirrored in the GitHub workflow code?
possibly. I'm getting the same error locally when trying to run the tests for python3 as well, so I guess some setup steps are missing but I don't know which (I'm not familiar with the python bootstraps either).
--
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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1508882976
> I don't know why this didn't trigger github actions, probably there are something wrong in the yaml makes it failed to parse the config?
yes it was an indentation but which is now fixed
--
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
[GitHub] [thrift] fishy commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "fishy (via GitHub)" <gi...@apache.org>.
fishy commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1508853437
I don't know why this didn't trigger github actions, probably there are something wrong in the yaml makes it failed to parse the config?
--
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
[GitHub] [thrift] Fokko commented on a diff in pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2787:
URL: https://github.com/apache/thrift/pull/2787#discussion_r1168281038
##########
.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"]
Review Comment:
It would be nice to have a matrix to test against different versions of Python 3 as well:
https://github.com/apache/iceberg/blob/9dd00fab1dc160bb531091cbdda8f19ef5b3a32e/.github/workflows/python-ci.yml#L39-L42
This way you can also be more explicit in the supported Python versions (sometimes stuff breaks). Instead of:
https://github.com/apache/thrift/blob/1e3d90d8fd4160d538b7a4d902169eae5155e08a/lib/py/setup.py#L136
You can be more explicit in versions:
https://github.com/apache/airflow/blob/8d81963c014398a7ab14505fd8e27e432f1aaf5c/setup.cfg#L39-L42
##########
.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"]
Review Comment:
I would drop Python v2. Python 2 has been sunsetted 1st of January 2020: https://www.python.org/doc/sunset-python-2/
--
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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1509721834
> @fishy i have no knowledge of python lib tests, can you tell the cause here? https://github.com/apache/thrift/actions/runs/4707202819/jobs/8349103781?pr=2787
turns out i need to backport a list of libraries for python 2
--
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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1509720030
@fishy i have no knowledge of python lib tests, can you tell the cause here? https://github.com/apache/thrift/actions/runs/4707202819/jobs/8349103781?pr=2787
--
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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1514496774
> > > ModuleNotFoundError: No module named 'thrift'
>
> > could it be because of some path handling logic not being mirrored in the GitHub workflow code?
>
> possibly. I'm getting the same error locally when trying to run the tests for python3 as well, so I guess some setup steps are missing but I don't know which (I'm not familiar with the python bootstraps either).
it's due to a naming convention change which i have fixed in this pull request. but now am hitting ssl errors which i have no idea how to move pass
--
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
[GitHub] [thrift] Jimexist commented on pull request #2787: THRIFT-5564: add GitHub action for python 2.x and 3.x
Posted by "Jimexist (via GitHub)" <gi...@apache.org>.
Jimexist commented on PR #2787:
URL: https://github.com/apache/thrift/pull/2787#issuecomment-1514497977
@fishy how do you think i can mark the failing tests skipped and merge this pull request first. this pull request is at least marginally better than the status quote which is without **any** CI.
--
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