THRIFT-2059: Support for Enums in Python >= 3.4

[kainjow]

Currently in Python, the generated enum classes for enum thrift objects, are not being used in deserialization (or serialization). This means that the string representations of enums are never used. Moving one step further than the patch contained in THRIFT-2059, this creates python IntEnum classes and retrieves the string representation on deserialization while storing the integer representation on serialization.

[Recreated original PR from #1788 by @constd]

• Did you create an Apache Jira ticket? (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.

[kainjow]

@fishy @alisaifee @Jens-G

[kainjow]

Technically this can work in older versions of Python with enum34.

[kainjow]

A few failures, but seems like flaky tests. I spotted this in 2 runs:

Testing script: /usr/bin/python /thrift/src/test/py/FastbinaryTest.py
----
Traceback (most recent call last):
  File "/thrift/src/test/py/FastbinaryTest.py", line 42, in <module>
    from DebugProtoTest import Srv
ImportError: No module named DebugProtoTest
*** FAILED ***
LIBDIR: /thrift/src/lib/py/build/lib.linux-x86_64-2.7
PY_GEN: gen-py-enum
SCRIPT: FastbinaryTest.py
Traceback (most recent call last):
  File "./RunClientServer.py", line 323, in <module>
    sys.exit(main())
  File "./RunClientServer.py", line 302, in main
    runScriptTest(options.libdir, options.gen_base, genpydir, script)
  File "./RunClientServer.py", line 103, in runScriptTest
    raise Exception("Script subprocess failed, retcode=%d, args: %s" % (ret, ' '.join(script_args)))
Exception: Script subprocess failed, retcode=1, args: /usr/bin/python /thrift/src/test/py/FastbinaryTest.py
�[0;31mFAIL�[m: RunClientServer.py
�[0;31m==================�[m
�[0;31m1 of 1 test failed�[m
�[0;31m==================�[m
Makefile:532: recipe for target 'check-TESTS' failed
make[4]: *** [check-TESTS] Error 1
make[4]: Leaving directory '/thrift/src/test/py'
Makefile:658: recipe for target 'check-am' failed
make[3]: *** [check-am] Error 2
make[3]: Leaving directory '/thrift/src/test/py'
Makefile:661: recipe for target 'check' failed
make[2]: *** [check] Error 2
make[2]: Leaving directory '/thrift/src/test/py'
Makefile:622: recipe for target 'check-recursive' failed
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory '/thrift/src/test'
Makefile:680: recipe for target 'check-recursive' failed
make: *** [check-recursive] Error 1
travis_time:end:05c23b76:start=1642914601939318320,finish=1642916011287231557,duration=1409347913237,event=script
�[0K�[31;1mThe command "build/docker/run.sh" exited with 2.�[0m


Done. Your build exited with 1.

and did hit this locally. I'd have to run the test command twice for it to pass. I think there's some CMake dependency issue. Can continue to look at it for a future PR.

[fishy]

I do not have enough experience messing with the python compiler 😅

We do have plan to drop support for python 2, or python3 before 3.4 (as they are all EOL now), so if we do this after that we don't really have to do any special handling and can just support it blindly. but doing it now means that we still need to handle the case of <3.4 that IntEnum is not available?

[fishy]

I'm pretty sure these 2 travis failures do not show before this pr:

• https://app.travis-ci.com/github/apache/thrift/jobs/556647214
• https://app.travis-ci.com/github/apache/thrift/jobs/556647215

[kainjow]

We do have plan to drop support for python 2, or python3 before 3.4 (as they are all EOL now), so if we do this after that we don't really have to do any special handling and can just support it blindly. but doing it now means that we still need to handle the case of <3.4 that IntEnum is not available?

Yes that's right. But it might also be good to keep this as an option when introduced, and then in a later version make it the new default just to make sure it's stable.

I'm pretty sure these 2 travis failures do not show before this pr:

• https://app.travis-ci.com/github/apache/thrift/jobs/556647214
• https://app.travis-ci.com/github/apache/thrift/jobs/556647215

ah, I only updated the CMake tests, looks like there's also equivalent for Make.

[kainjow]

Looks like tests are passing, but some failures with js and erl.

[Jens-G]

Nice. Didn't notice that it was 1000 commits :-(

[kainjow]

You could consider turning off Merge commits on the repo and only enable Squash.

…

On Sat, Mar 5, 2022, at 9:20 AM, Jens Geyer wrote: Nice. Didn't notice that it was 1000 commits — Reply to this email directly, view it on GitHub <#2489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACCFOYL35JZ5YR7O3GKEZLU6OJXDANCNFSM5J4WASJA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[Jens-G]

You could consider squashing as well, right?