MyPyC integration#1
Merged
Merged
Conversation
…ement for improved type safety and performance - Updated instruction tables (ii_reg_i_offset.py, ii_reg_ii_imm.py, iii_reg.py, wo_args.py) to include type hints and constructor initialization. - Changed return types of get_props methods to return lists instead of tuples for consistency. - Enhanced memory management in memory.py with better type handling and optimizations. - Introduced type checking for program.py and pvm.py to ensure better type safety. - Added setup.py for MyPyC compilation to optimize performance. - Created type stubs for tsrkit_asm to improve type hinting and IDE support.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the tsrkit_pvm codebase by implementing MyPyC integration for improved type safety and performance. The changes prepare the codebase for compilation with MyPyC (a Python-to-C compiler) by adding comprehensive type hints throughout the instruction tables and core modules.
- Adds type hints and constructor initialization to all instruction table classes
- Changes return types from tuples to lists for consistency across instruction tables
- Introduces a setup.py file for MyPyC compilation configuration
- Creates type stubs for improved IDE support and external library compatibility
Reviewed Changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tsrkit_pvm/stubs/tsrkit_asm.pyi | Type stub file for tsrkit_asm module |
| tsrkit_pvm/recompiler/init.py | Simplified module with proper type hints |
| tsrkit_pvm/interpreter/pvm.py | Added type hints and removed deprecated PVM alias |
| tsrkit_pvm/interpreter/program.py | Enhanced with type hints and proper initialization |
| tsrkit_pvm/interpreter/memory.py | Comprehensive type safety improvements |
| tsrkit_pvm/interpreter/instructions/tables/*.py | All instruction tables updated with constructors and type hints |
| tsrkit_pvm/core/*.py | Core modules enhanced with type safety |
| tsrkit_pvm/common/*.py | Utility modules updated with proper typing |
| setup.py | New MyPyC compilation configuration |
| pyproject.toml | Updated license field |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…ved performance and clarity
- Added TYPE_CHECKING imports to enhance type hinting in assembler tables. - Introduced constructor parameters for Instructions classes to manage state. - Optimized REC_Program initialization with a single-pass structure building. - Enhanced VMContext with a direct binary layout for faster encoding/decoding. - Improved memory management in Recompiler, ensuring proper cleanup of allocated buffers. - Updated segwrap library loading with robust error handling and initialization checks. - Added mypy configuration for gradual type checking migration. - Refined gas computation and instruction processing for efficiency. - Streamlined encoding and storage methods in VMContext for performance.
…into feat/mypyc
- Updated `InstructionsWoArgs` class to improve error handling and memory usage. - Enhanced `CyInstMapper` with better dispatching and block execution logic. - Removed dynamic PVM implementation selection; defaulting to standard interpreter. - Consolidated instruction mapping and execution logic in `CyBlockInfo` and `CyCompiledInstruction`. - Introduced Cython declaration files for `cy_block`, `cy_memory`, and `cy_program` to optimize performance. - Implemented unified instruction table interface for better extensibility. - Added comprehensive error handling in `PvmError` class. - Removed obsolete `libsegwrap.so` file and updated segwrap package path. - Improved memory management and caching strategies in `CyProgram`.
…e opcode dispatch
…unctions - Updated `get_props` method in `InstructionsWoArgs` to include `skip_index` parameter for better instruction processing. - Modified `trap_fn` to raise `PvmError` instead of returning a panic tuple. - Enhanced `CyInstMapper` to utilize `program._exec_blocks` for caching compiled blocks, improving performance. - Added debug print statements in various instruction tables to trace execution flow and register values. - Implemented new Cython utility functions in `cy_utils` for optimized memory and bit manipulation operations. - Improved memory handling in `INT_Memory` with additional debug outputs for read/write operations. - Updated `INT_Program` to define `_exec_blocks` as a dictionary of `BlockInfo` for better block management.
- Updated Cython compiler directives to enhance performance by disabling unnecessary checks and enabling optimizations across multiple instruction tables. - Changed function definitions from `cdef` to `cdef inline` for various arithmetic and comparison functions to improve inlining and reduce function call overhead. - Streamlined the handling of immediate values and register operations in the instruction tables for better clarity and efficiency. - Removed redundant comments and improved code readability by consolidating similar patterns. - Adjusted return statements in instruction functions to use a more concise format.
- Updated `InstructionsWoArgs` class to return `InstructionProps` instead of a tuple in `get_props` method. - Changed panic handling in `trap_fn` to raise `PvmExit` instead of `PvmError`. - Modified `fallthrough_fn` to return a single `uint32_t` instead of a tuple. - Enhanced `CyInstMapper` to utilize a more efficient dispatch table initialization, reducing Python overhead. - Streamlined opcode handling by directly accessing `CyTableEntry` from the dispatch table. - Improved memory management by maintaining a keep-alive list for table objects and entries.
This commit prepares tsrkit-pvm for distribution on PyPI with pre-built wheels for multiple platforms, eliminating the need for downstream projects to build from source. ## Changes ### Package Metadata (pyproject.toml) - Bump version to 0.2.0 - Update Python requirement to >=3.12,<3.13 - Add PyPI classifiers for all supported platforms - Add project URLs (homepage, repository, issues) - Update tsrkit-types dependency to use PyPI version (>=1.0.0,<2.0.0) - Add keywords for better discoverability ### Build System (setup.py) - Add platform detection for recompiler support (Linux x86_64 only) - Conditionally compile segwrap.c extension only on compatible platforms - Add informative build messages for platform compatibility - Recompiler requires Linux x86_64 due to: - x86 machine code generation (not ARM-compatible) - Linux-specific headers (seccomp.h, filter.h) - Linux syscalls (prctl, seccomp) ### Source Distribution (MANIFEST.in) - NEW - Include all necessary source files (.py, .pyx, .pxd, .c, .h) - Include documentation and license files - Exclude build artifacts and cache files ### CI/CD (.github/workflows/publish-pypi.yml) - NEW - Build wheels for all target platforms: - Linux: x86_64 (with recompiler), aarch64 (without recompiler) - macOS: x86_64, arm64 (without recompiler) - Windows: x86_64 (without recompiler) - Use cibuildwheel for consistent cross-platform builds - QEMU support for ARM64 cross-compilation - Build on PRs to main for testing - Auto-publish to PyPI on version tags (v*) - Python 3.12 wheels only ## Platform Support Matrix | Platform | Arch | Cython | Recompiler | |----------------|---------|--------|------------| | Linux | x86_64 | ✓ | ✓ | | Linux | aarch64 | ✓ | ✗ | | macOS | x86_64 | ✓ | ✗ | | macOS | arm64 | ✓ | ✗ | | Windows | x86_64 | ✓ | ✗ | ## Testing Wheels will be built on every PR to main for validation. To publish to PyPI, create and push a version tag (e.g., v0.2.0). Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
feat: prepare tsrkit-pvm for PyPI publishing with multi-platform wheels
## Critical Fixes 1. **License Classifier Conflict (PEP 639)** - Remove "License :: OSI Approved :: MIT License" from classifiers - Keep only "license = 'MIT'" field - Modern setuptools prohibits duplicate license specification - Fixes: setuptools.errors.InvalidConfigError 2. **Build System Dependencies** - Replace mypy with cython>=3.1.4 in [build-system] requires - Cython is required for building extensions during sdist creation - mypy is only needed as a dev dependency 3. **Python Version in sdist Build** - Add explicit Python 3.12 setup in build_sdist job - Install build dependencies (build, cython, setuptools, wheel) - Use 'python -m build --sdist' instead of 'pipx run build --sdist' - Ensures correct Python version and available dependencies ## Testing Workflow now properly builds: - Wheels for all 5 platforms (Linux x64/aarch64, macOS x64/arm64, Windows x64) - Source distribution with Python 3.12 and Cython support Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
Replace empty CIBW_TEST_COMMAND with CIBW_TEST_SKIP to properly skip tests. Empty string was causing invalid --only parameter error on Windows runners. Changes: - Remove CIBW_TEST_COMMAND: "" (causes empty string parameter issues) - Add CIBW_TEST_SKIP: "*" (proper way to skip all tests in cibuildwheel) Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
The cibuildwheel action was receiving empty string parameters (--config-file '""' and --only '""') which caused failures on Windows runners. Changes: - Upgrade to cibuildwheel@v2.21.3 (latest, better Windows support) - Remove 'with:' block entirely - rely solely on CIBW_* env vars - Don't explicitly pass config-file or only parameters The action should use its defaults when parameters are omitted, not when they're set to empty strings. Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
Modern setuptools (>=77.0.0) requires packaging>=24.2 but it wasn't being installed in the build environment, causing macOS ARM64 builds to fail. Error was: ImportError: Cannot import `packaging.licenses`. Setuptools>=77.0.0 requires "packaging>=24.2" to work properly. Changes: - Add packaging>=24.2 to [build-system] requires in pyproject.toml - Add packaging>=24.2 to CIBW_BEFORE_BUILD in workflow - Add packaging>=24.2 to sdist build dependencies - Upgrade pip, setuptools, wheel explicitly in CIBW_BEFORE_BUILD Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
The build was failing with a constraint conflict: The user requested packaging>=24.2 The user requested (constraint) packaging==24.1 cibuildwheel has a constraint file pinning packaging==24.1, so we can't explicitly request packaging>=24.2. Solution: - Upgrade setuptools requirement to >=77.0.0 (which needs packaging>=24.2) - Remove explicit packaging>=24.2 requirement - Let setuptools pull in the correct packaging version as its own dependency - setuptools>=77.0.0 declares packaging>=24.2 in its dependencies, so it will be installed correctly without conflicting with constraints This approach avoids the constraint conflict while ensuring we have the correct versions of all dependencies. Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
setuptools>=77.0.0 requires packaging>=24.2, but cibuildwheel's constraint file pins packaging==24.1, creating an unsolvable conflict. The error was: ImportError: Cannot import `packaging.licenses`. Setuptools>=77.0.0 requires "packaging>=24.2" to work properly. Solution: Use setuptools>=61.0,<77.0.0 to avoid the packaging>=24.2 requirement entirely. Versions before 77.0.0 don't validate license expressions using the packaging library, so they work fine with the existing constraint file. Changes: - Pin setuptools to >=61.0,<77.0.0 in [build-system] requires - Pin setuptools to >=61.0,<77.0.0 in CIBW_BEFORE_BUILD - Pin setuptools to >=61.0,<77.0.0 in sdist build dependencies This avoids the packaging version conflict while still using a modern version of setuptools (61.0 was released May 2022). Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
The license field was using the old string format `license = "MIT"` which
is no longer valid in modern pyproject.toml (PEP 621).
Error was:
configuration error: `project.license` must be valid exactly by one definition
GIVEN VALUE: "MIT"
PEP 621 requires license to be either:
- {text = "MIT"} for inline license text
- {file = "LICENSE"} for a file reference
Changed from: license = "MIT"
Changed to: license = {text = "MIT"}
Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
On Windows, you cannot upgrade pip while pip is running, which causes: ERROR: To modify pip, please run the following command: python.exe -m pip install --upgrade pip ... The pip version that comes with cibuildwheel is already recent and sufficient for building wheels, so we don't need to upgrade it. Changed from: pip install --upgrade pip "setuptools>=61.0,<77.0.0" wheel cython Changed to: pip install "setuptools>=61.0,<77.0.0" wheel cython Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
Windows cp1252 encoding can't handle Unicode characters in print statements, causing build failures: UnicodeEncodeError: 'charmap' codec can't encode character '\u2297' in position 0: character maps to <undefined> Replaced all Unicode symbols in setup.py with ASCII equivalents: - ✓ (U+2713 CHECK MARK) -> [+] - ✗ (U+2717 BALLOT X) -> [!] - ⊗ (U+2297 CIRCLED TIMES) -> [-] This ensures the build process works on all platforms including Windows without requiring special encoding handling. Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
…iler modes work on Linux x86_64 ## Issues Fixed 1. **Missing tsrkit-asm dependency**: The recompiler imports tsrkit_asm but it wasn't in dependencies, causing import errors at runtime. 2. **Cython extensions overwriting segwrap extension**: On Linux x86_64, the segwrap C extension was being built but then overwritten when Cython extensions were added. This meant only interpreter mode worked, not recompiler mode. ## Changes ### pyproject.toml - Add tsrkit-asm>=0.1.0 to dependencies (required by recompiler) ### setup.py - Use `ext_modules.extend()` instead of `ext_modules = cythonize()` to preserve segwrap - Use `mypyc_modules = []` to avoid overwriting in MyPyC path - Update messages to clarify available runtime modes: - Linux x86_64: Both PVM_MODE=interpreter and PVM_MODE=recompiler available - Other platforms: Only PVM_MODE=interpreter available - Add summary showing total extensions built ## Result On Linux x86_64 systems with PVM_BUILD_MODE=cython: - Builds segwrap C extension (for recompiler mode) - Builds all 21 Cython extensions (for interpreter mode) - Users can choose at runtime: PVM_MODE=recompiler or PVM_MODE=interpreter On other platforms with PVM_BUILD_MODE=cython: - Builds all 21 Cython extensions (for interpreter mode) - Users run with: PVM_MODE=interpreter Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
Add comprehensive testing of both PVM runtime modes after wheel builds: ## Test Coverage ### All Platforms - Test PVM_MODE=interpreter (Cython) with CyPVM import and instantiation - Simple smoke test to verify the wheel is functional ### Linux x86_64 Only - Test PVM_MODE=recompiler with Recompiler import and instantiation - Marked as continue-on-error since tsrkit-asm is not yet on PyPI - Will pass once tsrkit-asm is published ## Test Structure Each platform tests: 1. Import the appropriate PVM class (CyPVM or Recompiler) 2. Create a simple Program instance 3. Instantiate the PVM with the program 4. Verify no errors during basic initialization ## Expected Behavior **Current (tsrkit-asm not published)**: - ✓ All interpreter tests pass - ⚠ Recompiler test fails with ImportError (expected) **After tsrkit-asm published**: - ✓ All interpreter tests pass - ✓ Recompiler test passes on Linux x86_64 The publish job now depends on test_wheels completing, ensuring we only publish wheels that pass basic functionality tests. Co-Authored-By: Claude (claude-sonnet-4-5-2) <noreply@anthropic.com>
Update from macos-13 to macos-15-intel for Intel macOS builds. macos-13 runners have been retired, and macos-15-intel is the last supported Intel-based macOS runner image.
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.
…ement for improved type safety and performance