Uh oh!
There was an error while loading. Please reload this page.
refactor(pypi): don't use Python to patch wheels - #3808
Conversation
1. Refactored patch_whl.bzl — Extracted fix_record_content() as a public pure function (no rctx dependency) so it can be unit tested directly. 2. Unit tests (patch_whl_tests.bzl) — 5 new rules_testing tests for fix_record_content: - Adds missing files to RECORD - Returns None when nothing is missing - Preserves quoting style of existing entries - Skips excluded files (INSTALLER, RECORD*, REQUESTED) - Skips .whl files 3. Integration test (patch_whl_patch_test.py) — End-to-end test that: - Creates a wheel from testdata/pkg/ via whl_from_dir_repo - Applies a patch via whl_library(whl_patches=...) - Verifies pkg.PATCHED is True (was False) and pkg.DATA is unchanged The integration test follows the established pattern: test data in tests/pypi/patch_whl/testdata/, registered in internal_dev_deps.bzl, wired into MODULE.bazel, and built as a py_test. All 205 pypi tests pass. With this we can potentially more easily separate whl and sdist path ways.
There was a problem hiding this comment.
Code Review
This pull request replaces the Python-dependent wheel patching mechanism with a native Starlark implementation that extracts, patches, fixes the RECORD file natively, and repacks the wheel using system utilities. Feedback on the changes highlights several issues: the CSV parsing logic for RECORD files does not handle filenames with commas or quotes correctly; both the Unix and Windows repacking commands will recursively include the output .whl file itself unless explicitly excluded; and the directory traversal loop uses an excessively large range of 10 million, which should be reduced to prevent potential memory and performance issues in Starlark.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
repack_whl.py is no longer needed after refactoring patch_whl.bzl to not use Python.
Use a temporary .zip file when repacking wheels on Windows to avoid Compress-Archive errors when writing directly to .whl files, and exclude the temp zip and final wheel from the archived files.
Move-Item uses -Destination, not -DestinationPath. This corrects a previous betrayal that caused parameter binding failures on Windows.
Instead of passing the PowerShell script as a command string, write it to a temporary repack_whl.ps1 file and execute it using -File. This improves reliability and avoids escaping issues.
Uh oh!
There was an error while loading. Please reload this page.
Instead of creating a temporary PowerShell script dynamically, use a static repack_whl.ps1 source file in the repository. This is cleaner, more reliable, and avoids repository_ctx.write errors.
Uh oh!
There was an error while loading. Please reload this page.
Refactored patch_whl.bzl — Extracted fix_record_content() as a public
pure function (no rctx dependency) so it can be unit tested directly.
Unit tests (patch_whl_tests.bzl) — 5 new rules_testing tests for fix_record_content:
Integration test (patch_whl_patch_test.py) — End-to-end test that:
The integration test follows the established pattern: test data in
tests/pypi/patch_whl/testdata/, registered in internal_dev_deps.bzl,
wired into MODULE.bazel, and built as a py_test. All 205 pypi tests
pass.
With this we can potentially more easily separate whl and sdist path
ways.