Skip to content

Python housekeeping #3231

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Nov 19, 2025
Merged

Python housekeeping #3231

merged 15 commits into from
Nov 19, 2025

Conversation

@CJCombrink
Copy link
Contributor

@CJCombrink CJCombrink commented Nov 10, 2025 •
edited
Loading

Some Python housekeeping in trying to make some progress on various python related tasks to get ultimately to attempt to implement Python UUID support

The following is addressed:

  1. Add back the python tests to the build.yaml pipeline
  2. Update the cmake support for Python according to newer cmake functions.
  3. Disabled SSL tests completely
    • Incorrectly assumed SSL issues were related to old Python versions but given 1 above it was clear that SSL is an issue
    • SSL Tests in the lib were already disabled
    • Created: THRIFT-5900
  4. Disabled type_hint and enum tests
  5. Replaced deprecated calls on the test side
  • Did you create an Apache Jira ticket? (Request account here, 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.

@CJCombrink
Copy link
Contributor Author

CJCombrink commented Nov 11, 2025

I see due to this change the AppVeyor build now uses Python 3.8 instead of the one specified in the config... will need to investigate

@CJCombrink CJCombrink marked this pull request as draft November 11, 2025 06:06
@CJCombrink CJCombrink marked this pull request as ready for review November 11, 2025 07:32
@CJCombrink
Copy link
Contributor Author

CJCombrink commented Nov 11, 2025 •
edited
Loading

Found Fixed the issue in the Appveyor builds with cmake not using the correct python

CJCombrink
CJCombrink commented Nov 11, 2025
View reviewed changes
@CJCombrink
Copy link
Contributor Author

CJCombrink commented Nov 11, 2025

This hole is deeper than I have anticipated.
I have now enabled the Python tests in the github action and now the enum tests are also tripping up here.
It appears that #3232 will need to merge as part of this effort, I was hoping to keep it separate

@Jens-G Jens-G added the python label Nov 13, 2025
@Jens-G
Copy link
Member

Jens-G commented Nov 14, 2025

Is this complete?

@CJCombrink
Copy link
Contributor Author

CJCombrink commented Nov 14, 2025

Is this complete?

Yes, I was hoping to get #3237 merged, so that I can rebase this branch before merge.

Perhaps @fishy might also want to give it a quick review if possible?

@zsy056 zsy056 mentioned this pull request Nov 14, 2025
Open
5 tasks
@CJCombrink
FIx cmake warning on CMP0148
419a2f7 
- find_package(PythonInterp) and find_package(PythonLibs) were deprecated in 3.12 and removed in 3.27
- The project minimum is at 3.16 already
@CJCombrink
Fix call to deprecated test methods
b715e86 
- Deprecated in Python 3.2 and removed in 3.11
@CJCombrink
Fix collection test
02a0073 
- collections.abc was added in 3.3 collections.Hashable removed in 3.10
@CJCombrink
Add option to search the local lib path
bf02698 
- Locally and in Windows actions the second option is needed
@CJCombrink
Use the v0.16 thrift for type_hints also
68863b9 
- Full UUID support not added and this seems to work for some cases but not all, thus removing until full UUID is added
@CJCombrink
Missed actually using the new variables from the new calls to get Python
8b2f67a
@CJCombrink
Print Python version
64aca80 
- And add message if interpreter not found
- Remove the QUIET to get some idea of what cmake is doing
- Trying to debug issue on Appveyor
@CJCombrink
Try to force python version
ad0d3fd 
- The change in find_package favours the latest Python
- Force cmake to use the one specified
- Put back the echo again since it is needed
@CJCombrink
Put back the actual python tests
0a02e60
@CJCombrink
Also use the 0.16 file for the makefile
e17d8e3 
- Same as per the cmake side
@CJCombrink
Pin python building also to 3.13
39a2fb8 
- No need for the check in the step any more since not supporting python 2
@CJCombrink
Skip ssl tests
ea14067 
- See https://issues.apache.org/jira/browse/THRIFT-5901
@CJCombrink
Also skip the type_hints and enum tests
c913f1a
@CJCombrink
Reworked the local_libpath() using the code form _import_local_thrift
5725c30 
- Common problem, common code
@CJCombrink
Fix issue with deprecated function in test
a748a0a 
- assert_   was removed in Python 3 and later removed
- Fixed the script to error out correctly when the tests fail
   - The RunClientServer.py test did not fail as expected on this error
@CJCombrink
Copy link
Contributor Author

CJCombrink commented Nov 17, 2025

@Jens-G @fishy
The work here should be done, after review and CI builds it can be merged if there are no objections.

@Jens-G Jens-G merged commit f8ce26c into apache:master Nov 19, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@CJCombrink @Jens-G