THRIFT-5537: Drop support for end-of-life python versions (including python 2)#2
Open
greggdonovan wants to merge 49 commits intomasterfrom
Open
THRIFT-5537: Drop support for end-of-life python versions (including python 2)#2greggdonovan wants to merge 49 commits intomasterfrom
greggdonovan wants to merge 49 commits intomasterfrom
Conversation
The system bison (2.3) doesn't support the %code directive. Add homebrew's bison to GITHUB_PATH so it persists across all steps. Co-Authored-By: Claude <noreply@anthropic.com>
This commit adds type checking validation for thrift-generated Python code using Astral's ty type checker to ensure our Python 3.10+ type hints are correct and complete. New test infrastructure: - type_check_test.thrift: Comprehensive IDL covering all thrift features (enums, typedefs, structs, unions, exceptions, services, constants) - test_type_check.py: Test runner that auto-installs ty via uv, generates code, runs ty check, and validates imports/behavior - py.typed marker generation in t_py_generator.cc for PEP 561 compliance The test validates: - ty check passes with zero errors on all generated code - py.typed marker exists in generated packages - All generated types are importable - Enums are IntEnum subclasses with correct values - Structs can be instantiated with type-correct arguments - Exceptions inherit from TException Also includes Python library modernization: - Simplified SSL/TLS handling for Python 3.10+ - Removed Python 2 compatibility code - Updated type annotations throughout Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Configure ty to find the thrift library by passing --extra-search-path pointing to the build directory. This fixes the unresolved-import errors in CI where ty couldn't locate the thrift module. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The enum, type_hints, and no_utf8strings options were removed from the Python generator as they are now the default behavior for Python 3.10+. This commit updates the test infrastructure to remove: - gen-py-enum, gen-py-type_hints, gen-py-no_utf8strings test directories - Corresponding generation rules in generate.cmake and Makefile.am - References in RunClientServer.py genpydirs default Also fixes test_ciphers SSL test to skip NULL cipher tests on Windows where the SSL library handles invalid cipher specifications differently. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make Iface class extend Protocol for proper abstract method support - Use ellipsis (...) instead of pass for interface stub methods - Add generated code directory to ty search paths for relative imports - Add installed package location detection for CI environments Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
For frozen structs/exceptions, type checkers need to see attributes declared at class level to understand they exist when set via super().__setattr__() in __init__. This fixes ty errors like: - unresolved-attribute: Object of type `Self@__hash__` has no attribute `message` - invalid-parameter-default: Default value of type `None` is not assignable Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All __init__ parameters and instance attributes now consistently use `| None` type hints since: 1. All parameters default to None for backward compatibility 2. Validation of required fields happens at runtime in validate() This fixes ty errors: - invalid-parameter-default: Default value of type `None` is not assignable to annotated parameter type `str` - invalid-assignment: Cannot assign `str | None` to `str` Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
82537e0 to
fa45165
Compare
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace pip install uv with the official GitHub Action which is faster (prebuilt binaries) and properly maintained by the uv team. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The NULL cipher mismatch test was failing on some platforms because
ssl_version only sets the minimum TLS version, allowing TLS 1.3 to
negotiate. TLS 1.3 has its own cipher suites that aren't affected by
set_ciphers('NULL'), causing the connection to succeed when it should
fail.
Fix by using ssl_context with both minimum_version and maximum_version
set to TLS 1.2, ensuring TLS 1.3 cannot bypass the cipher check.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test_normalconnection test was flaky on slower CI runners because it only waited 2 seconds for the server thread to exit after calling close_server(). Increased the timeout to 10 seconds to allow more time for server shutdown on slower systems. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test was calling stop() and close() in quick succession, but close() destroys the socket pair that wake_up() uses to signal the server to exit from select(). This race condition caused the server thread to hang on macOS CI runners. Fix by calling stop() first, waiting for the server thread to exit, and only then calling close() to release resources. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Owner
Author
|
@greptile |
Greptile OverviewGreptile SummaryThis PR successfully removes Python 2 support and modernizes the Thrift Python library to require Python 3.10+. The changes span 93 files with comprehensive updates across build configuration, code generation, runtime library, and tests. Key Changes:
Testing & Quality:
Breaking Changes: Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| lib/py/setup.py | Updated Python version requirements to 3.10+, removed Python 2 backport dependencies |
| configure.ac | Removed py3 separate build target, unified on Python 3.10+ requirement |
| lib/py/src/transport/TSSLSocket.py | Enforced TLS 1.2+ minimum, removed Python 2.6/2.7 compatibility code, modernized SSL context handling |
| lib/py/src/transport/sslcompat.py | Simplified to TLS version enforcement utilities, removed legacy hostname validation |
| compiler/cpp/src/thrift/generate/t_py_generator.cc | Removed py2/py3 generator options, enforced IntEnum, added type hints, improved enum handling |
| lib/py/src/ext/module.cpp | Removed Python 2 C API code, simplified to Python 3 only |
| lib/py/test/test_sslsocket.py | Added comprehensive TLS tests, enforced TLS 1.2+, improved test stability |
| lib/py/test/thrift_TNonblockingServer.py | Fixed test shutdown sequence, added timeouts, improved cleanup ordering |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Build as Build System
participant Compiler as Thrift Compiler
participant PyLib as Python Library
participant Tests as Test Suite
Note over Dev,Tests: Python 2 Removal Process
Dev->>Build: Update configure.ac (Python 3.10+)
Dev->>Build: Update setup.py (python_requires='>=3.10')
Dev->>Compiler: Remove py:old_style, py:new_style options
Dev->>Compiler: Enforce IntEnum for all enums
Dev->>Compiler: Add type hints to generated code
Dev->>PyLib: Remove Python 2 C API code
Dev->>PyLib: Simplify SSL/TLS (enforce TLS 1.2+)
Dev->>PyLib: Remove backports.ssl_match_hostname
Dev->>PyLib: Use modern SSLContext APIs
Dev->>Tests: Add comprehensive type checking tests
Dev->>Tests: Update SSL tests for TLS 1.2+
Dev->>Tests: Fix server shutdown sequences
Dev->>Tests: Remove 280+ py3-specific test entries
Build->>Compiler: Generate Python code
Compiler-->>Build: Modern Python 3.10+ code with type hints
Build->>Tests: Run test suite
Tests->>PyLib: Validate TLS 1.2+ enforcement
Tests->>PyLib: Validate type hints with ty
Tests-->>Build: All tests pass
Build-->>Dev: Python 2 successfully removed
2 tasks
- Fix TSocket.py: use b'' instead of '' for empty bytes on ECONNRESET - Fix TJSONProtocol.py: correct 'senf' typo to 'self' in 4 properties - Fix t_py_generator.cc: update Python keywords (remove exec/print, add async/await) for Python 3.10+ - Fix TSSLSocket.py: enable hostname verification for CERT_OPTIONAL - Fix debian/rules: remove Python 2 pyversions, use python3 directly - Fix Vagrantfile: upgrade Ubuntu 14.04 to 22.04, JDK 8 to 17 - Fix util.py: raise ImportError instead of returning None - Fix TNonblockingServer.py: move poll registration to prepare(), use elif for mutually exclusive connection states Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The code generator now allows __traceback__, __context__, __cause__, and __suppress_context__ on immutable exceptions (not just slots). Update the test to verify settability instead of checking slots. Also includes timeout improvements for test stability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)
Not yet - but will do after review. It's a large PR so I left separate commits to make it a bit easier to grok.
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?