-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
THRIFT-5564: add GitHub action for python 2.x and 3.x #2787
Conversation
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 |
the 3.x one failed, we probably should also set |
could it be because of some path handling logic not being mirrored in the GitHub workflow code? |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some small comments, great to have this in the CI as well 👍🏻
runs-on: ubuntu-20.04 | ||
strategy: | ||
matrix: | ||
python-version: ["2.x", "3.x"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop Python v2. Python 2 has been sunsetted 1st of January 2020: https://www.python.org/doc/sunset-python-2/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a matrix to test against different versions of Python 3 as well:
This way you can also be more explicit in the supported Python versions (sometimes stuff breaks). Instead of:
Line 136 in 1e3d90d
'Programming Language :: Python :: 3', |
You can be more explicit in versions:
https://github.com/apache/airflow/blob/8d81963c014398a7ab14505fd8e27e432f1aaf5c/setup.cfg#L39-L42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 #2588
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? |
6a6c0ce
to
02b09b0
Compare
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 |
@fishy how do you think of marking 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comments can totally be done in a follow up PR instead. I certainly agree that this is already much better than what we have today.
# run: sudo make -C lib/py install-exec-hook | ||
|
||
- name: Run make check for python | ||
run: make -C lib/py check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
cross-test: | ||
needs: | ||
- lib-java-kotlin | ||
- lib-swift | ||
- lib-rust | ||
- lib-go | ||
- lib-python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔕 oh I see setDaemon
was deprecated since python 3.10.
[skip ci]
anywhere in the commit message to free up build resources.