You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2021/12/12 00:39:05 UTC
[GitHub] [thrift] alisaifee opened a new pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
alisaifee opened a new pull request #2487:
URL: https://github.com/apache/thrift/pull/2487
# Description
When using the building functions for the fallback scenario in `ProtocolBase<Impl>::readBytes`, a `SystemError` is raised in python 3.10 (this was only a warning till python 3.9) due to the use of "#yi" for the output buffer argument. The removal of the warning and dropped support is described in [Python issue 40943: https://bugs.python.org/issue40943].
Extra test cases have been added to cover serialization/deserialization both with compact & binary protocol both with and without the c-extension
- [x] Did you create an [THRIFT-5488](https://issues.apache.org/jira/projects/THRIFT/issues/THRIFT-5488) ticket? (not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [x] 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 change in pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#discussion_r767225934
##########
File path: lib/py/test/test_thrift_file/TestServer.thrift
##########
@@ -16,8 +16,12 @@
# specific language governing permissions and limitations
# under the License.
#
-
+
+struct Message {
+ 1: optional string body,
+ 2: optional i64 num,
+}
service TestServer{
- string add_and_get_msg(1:string msg)
+ Message add_and_get_msg(1:Message msg)
Review comment:
I don't think this change is explained in either the PR description of the ticket?
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-995415411
Unfortunately the error couldn't be reproduced on CI since the python version (3.6.9) doesn't actually contain the warning. Regardless, I think having warnings as errors should be beneficial - especially once the CI pythons are updated.
--
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 a change in pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#discussion_r767225934
##########
File path: lib/py/test/test_thrift_file/TestServer.thrift
##########
@@ -16,8 +16,12 @@
# specific language governing permissions and limitations
# under the License.
#
-
+
+struct Message {
+ 1: optional string body,
+ 2: optional i64 num,
+}
service TestServer{
- string add_and_get_msg(1:string msg)
+ Message add_and_get_msg(1:Message msg)
Review comment:
I don't think this change is explained in either the PR description of the ticket?
--
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] alisaifee commented on a change in pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on a change in pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#discussion_r768765167
##########
File path: lib/py/test/test_thrift_file/TestServer.thrift
##########
@@ -16,7 +16,11 @@
# specific language governing permissions and limitations
# under the License.
#
-
+
+struct Message {
+ 1: optional string body,
+ 2: optional i64 num,
+}
Review comment:
This struct is used in the test cases here: https://github.com/apache/thrift/pull/2487/files#diff-1364ef1b9a20b93088778203e7f99d70ecb498ba6736e11466979edc229f4600R36
--
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] alisaifee commented on a change in pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on a change in pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#discussion_r767304872
##########
File path: lib/py/test/test_thrift_file/TestServer.thrift
##########
@@ -16,8 +16,12 @@
# specific language governing permissions and limitations
# under the License.
#
-
+
+struct Message {
+ 1: optional string body,
+ 2: optional i64 num,
+}
service TestServer{
- string add_and_get_msg(1:string msg)
+ Message add_and_get_msg(1:Message msg)
Review comment:
Removed in b2672adfea84051c38b7d124f4599b544d11ebbd
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-995722592
> In that case, I'd prefer to revert the CI changes. I don't want the CI to start fail suddenly when Ubuntu decided to upgrade python3 in their version from 3.6 to 3.7.
That's a very good point that I hadn't considered at all. I've reverted that commit (force pushed) so that there's a clean single commit for you to merge.
> Sorry about the back-and-forth.
No worries at all! Thank you for the review.
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-996170131
> @alisaifee Can you rebase your branch on top of the latest master (with your CI fix for rust merged), so that the CI output of this PR would be more meaningful?
I already did that! Sadly there's still one erlang test case failing which I couldn't figure out. The difference between the CI failure now versus before the rust fix, is that the tests actually run now (previously they were failing in the test compilation stage)
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-991959949
I realized I hadn't added any details about using the unit tests to verify the behavior before and after the patch - added now.
Regarding the CI - the build is failing at the moment due to a rust dependency issue (`clap`: `use of unstable library feature 'matches_macro'`) which I also get locally. However, I didn't see the where the python unit tests would be run. Adding `-Werror` does sound like a good direction though!
--
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 #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-995502244
> Unfortunately the error couldn't be reproduced on CI since the python version (3.6.9) doesn't actually contain the warning. Regardless, I think having warnings as errors should be beneficial - especially once the CI pythons are updated.
In that case, I'd prefer to revert the CI changes. I don't want the CI to start fail suddenly when Ubuntu decided to upgrade python3 in their version from 3.6 to 3.7.
We _might_ want to consider the CI change in a separated PR so it can be reverted, but that still sounds quite risky to me, as when that upgrade in ubuntu actually happens no one would remember this is the commit to be reverted.
Sorry about the back-and-forth.
--
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 a change in pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#discussion_r768296304
##########
File path: lib/py/test/test_thrift_file/TestServer.thrift
##########
@@ -16,7 +16,11 @@
# specific language governing permissions and limitations
# under the License.
#
-
+
+struct Message {
+ 1: optional string body,
+ 2: optional i64 num,
+}
Review comment:
this change should also be reverted?
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-995329084
Are there any specific changes I should make to this patch?
Since the cross tests are now running on master (still one pending erlang related failure), I could also figure out how to add the `-Werror` or `PYTHONWARNINGS=error` in that CI step?
--
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 merged pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
fishy merged pull request #2487:
URL: https://github.com/apache/thrift/pull/2487
--
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] alisaifee commented on a change in pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on a change in pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#discussion_r767304682
##########
File path: lib/py/test/test_thrift_file/TestServer.thrift
##########
@@ -16,8 +16,12 @@
# specific language governing permissions and limitations
# under the License.
#
-
+
+struct Message {
+ 1: optional string body,
+ 2: optional i64 num,
+}
service TestServer{
- string add_and_get_msg(1:string msg)
+ Message add_and_get_msg(1:Message msg)
Review comment:
My mistake, this shouldn't have made it to the PR as it is not relevant to this issue.
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-991968065
@fishy I just verified that after getting the cross tests to run after patching the rust version issue in #2488 the issue can be reproduced by a failure in the cross tests for py3-threader_unframed_binary & compact. Using `PYTHONWARNINGS=error make cross` with python 3.9 also reproduces the same failures without this patch (albeit also for py2) - but is cleared when the patch is applied.
--
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 #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-996094113
@alisaifee Can you rebase your branch on top of the latest master (with your CI fix for rust merged), so that the CI output of this PR would be more meaningful?
--
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] alisaifee commented on pull request #2487: THRIFT-5488: Define PY_SSIZE_T_CLEAN to fix error with using PyObject_CallFunction in python 3.10
Posted by GitBox <gi...@apache.org>.
alisaifee commented on pull request #2487:
URL: https://github.com/apache/thrift/pull/2487#issuecomment-995390430
Turns out adding environment variables for specific targets for the cross tests was already built in :tada:. I've created a PR against master with just the `PYTHONWARNINGS` set to `error` to compare against this branch at #2490
--
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