Repository navigation
Validate wheel ABI, linkage, and runtime compatibility - #473
harsh21234i wants to merge 24 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Pre-merge checks:
|
[Medium impact] The PR appears safe to merge; the remaining previous finding is a non-blocking gap in the matrix test. Findings
SummaryThe PR adds platform, exported-symbol, and linkage checks for native wheels.
The earlier non-ELF export, shared-library interpreter, macOS version comparison, option-check, and delay-load fixture threads were manually resolved without explanatory replies. The current code addresses those findings. Reviews (11) · Last reviewed commit: "Merge upstream main and resolve release ..." · Reviewed by Greptile |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
scripts/verify_wheel.py (2)
970-995: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider catching
struct.errorinmain.The new parsers call
struct.unpack_fromon attacker- or build-controlled offsets. Any residual out-of-range read raisesstruct.error, which theexceptclause at line 996 does not cover. The verifier then exits with a traceback and a nonzero code from an unhandled exception, not the intendedwheel verification failedmessage.♻️ Proposed change
- except (AssertionError, KeyError, zipfile.BadZipFile) as e: + except (AssertionError, KeyError, struct.error, zipfile.BadZipFile) as e:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_wheel.py` around lines 970 - 995, Update the exception handling in main around verify_wheel to also catch struct.error raised by parser offset reads, producing the existing “wheel verification failed” handling instead of an uncaught traceback. Preserve the current handling for the exceptions already covered.
227-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the last symbol entry against a partial read.
symbol_lengthis not required to be a multiple ofentry_size. If the table is ragged, the final iteration callsstruct.unpack_frompast the declared table end and can raisestruct.error.maincatches onlyAssertionError,KeyError, andzipfile.BadZipFile, so the verifier exits with a traceback instead of the intended failure message.♻️ Proposed bound
- for offset in range(symbol_offset, symbol_offset + symbol_length, entry_size): + for offset in range(symbol_offset, symbol_offset + symbol_length - symbol_size + 1, entry_size):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_wheel.py` around lines 227 - 243, Update the symbol-entry loop around the symbol parsing logic to process only complete entries, ensuring each iteration’s offset plus entry_size does not exceed symbol_offset + symbol_length. Skip or stop before parsing a ragged final entry so struct.unpack_from cannot read beyond the declared symbol table.tests/test_verify_wheel.py (2)
108-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd positive export coverage for the three formats.
Every fixture here is a bare header.
_elf_exported_symbolsreturns early at line 171 because the data is 20 bytes,_macho_exported_symbolsreturns early becausencommandsis 0, and_pe_exported_symbolsreturns early because the data directory is out of range. The tests therefore never execute the section-table, symbol-table, or export-directory paths. Thesh_link/sh_entsizedefect flagged inscripts/verify_wheel.pylines 204-222 is invisible to this suite.Add a fixture that carries a real
SHT_DYNSYMsection plus string table and assert that_require_exported_symbolsacceptsxy_abi_version. Add equivalent Mach-OLC_SYMTABand PE export-directory fixtures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_verify_wheel.py` around lines 108 - 146, Add positive export-symbol coverage for ELF, Mach-O, and PE in the tests around test_native_binary_header_matches_wheel_platform. Build fixtures containing a valid ELF SHT_DYNSYM with its linked string table, a Mach-O LC_SYMTAB with the exported symbol, and a PE export directory exposing xy_abi_version, then call _require_exported_symbols and assert acceptance. Ensure the ELF fixture exercises section-table symbol parsing, including valid sh_link and sh_entsize values, rather than relying on the existing header-only binaries.
174-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises dependency-family coverage that the body does not assert.
test_elf_linkage_validates_glibc_floor_and_dependency_familyasserts the glibc floor twice but never asserts a rejection for a dependency outside_ELF_ALLOWED_GLIBC, and never exercises themusllinux_branch. Add a case with an unexpectedDT_NEEDEDname and a case with a musl interpreter, or rename the test to match its scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_verify_wheel.py` around lines 174 - 184, Expand test_elf_linkage_validates_glibc_floor_and_dependency_family to assert rejection of an unexpected DT_NEEDED dependency and to exercise the musllinux_ interpreter branch, using the existing _elf_linkage_fixture and _require_elf_linkage helpers. If dependency-family and musl coverage are not added, rename the test to describe only glibc-floor validation..github/workflows/release.yml (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the interpreter path explicitly instead of chaining with
||.The
||fallback runs the second interpreter whenever the first command exits nonzero, including when the assertions fail rather than when the interpreter is missing. On Linux and macOS a genuine assertion failure is then reported as a missing./smoke/Scripts/python.exe, which hides the real cause. The 300-character probe is also duplicated verbatim, so the two copies can drift.♻️ Proposed restructure
- name: Verify the wheel installs and loads the native core # Every published wheel is built for the runner architecture and must # pass an import plus representative native-kernel call. shell: bash run: | uv venv smoke uv pip install -p smoke dist/*.whl numpy anywidget "reflex>=0.9.6" - ./smoke/bin/python -c "import importlib.metadata as m, numpy as np, reflex_xy, xy.kernels as k; assert k.BACKEND=='native', k.BACKEND; assert reflex_xy.__version__ == m.version('xy'); codes, unique = k.factorize_fixed(np.asarray(['a', 'b', 'a'], dtype='S1')); assert codes.tolist() == [0, 1, 0] and unique.tolist() == [0, 1]; print('native', k.__file__)" \ - || ./smoke/Scripts/python.exe -c "import importlib.metadata as m, numpy as np, reflex_xy, xy.kernels as k; assert k.BACKEND=='native', k.BACKEND; assert reflex_xy.__version__ == m.version('xy'); codes, unique = k.factorize_fixed(np.asarray(['a', 'b', 'a'], dtype='S1')); assert codes.tolist() == [0, 1, 0] and unique.tolist() == [0, 1]; print('native', k.__file__)" + py=./smoke/bin/python + [ -x "$py" ] || py=./smoke/Scripts/python.exe + cat > smoke_probe.py <<'PY' + import importlib.metadata as m + import numpy as np + import reflex_xy + import xy.kernels as k + assert k.BACKEND == "native", k.BACKEND + assert reflex_xy.__version__ == m.version("xy") + codes, unique = k.factorize_fixed(np.asarray(["a", "b", "a"], dtype="S1")) + assert codes.tolist() == [0, 1, 0] + assert unique.tolist() == [0, 1] + print("native", k.__file__) + PY + "$py" smoke_probe.pyIf you move the probe into a checked-in script such as
scripts/wheel_smoke.py, update the required substrings inscripts/verify_ci_workflow.pylines 1274-1278 and the fixture string intests/test_verify_ci_workflow.pylines 1307-1310.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 104 - 108, Update the smoke-test step to select the platform-appropriate interpreter explicitly rather than chaining duplicate probes with ||, so assertion failures remain visible and only the valid executable is invoked. Extract the shared validation into a checked-in script such as scripts/wheel_smoke.py, and update the corresponding required-substring checks in verify_ci_workflow.py and its test fixture to match the new workflow content.scripts/verify_ci_workflow.py (1)
1282-1286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
native: falseguard now watches a matrix key that no step reads.
.github/workflows/release.ymlremoved theif: matrix.nativecondition from the smoke step, somatrix.nativeis unused metadata. This check passes as long as nobody writes the exact textnative: false, and it does not verify that the smoke step actually runs for each entry. A future entry added without thenativekey at all bypasses the guard.Prefer one of two options. Remove the
nativekey from the matrix and this guard, since the smoke step is now unconditional. Or keep the key and assert that the smoke step carries noif:condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_ci_workflow.py` around lines 1282 - 1286, Update the wheels workflow validation around wheels_job and the release matrix so it matches the unconditional smoke step: remove the obsolete native matrix key and the native: false guard, or retain that metadata while validating that the smoke step has no if: condition. Ensure entries missing native cannot bypass the intended smoke-step coverage check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify_wheel.py`:
- Around line 970-995: Update scripts/verify_wheel.py at lines 970-995 to catch
struct.error alongside the existing exceptions in main, preserving the standard
verification-failure message and exit code 1. At lines 227-243, bound the ELF
symbol loop to process only complete Elf*_Sym entries when symbol_length is not
a multiple of entry_size. At lines 584-589, validate that len(data) is at least
0x40 before reading e_lfanew at offset 0x3C.
- Around line 137-149: Update the Mach-O endianness selection in the detection
branch around `_macho_exported_symbols` so both big-endian magics,
`\xfe\xed\xfa\xce` and `\xfe\xed\xfa\xcf`, select `">"`, matching
`_require_macho_linkage`; preserve little-endian handling for the corresponding
reversed magics.
- Around line 929-946: Update the wheel verification flow around the
native-library checks to reject require_linkage when expect_platform is None
with an explicit assertion or validation error. Open the ZipFile once, read
native_libs[0] once, and reuse the extracted bytes for _require_native_target,
_require_exported_symbols, and the platform-specific linkage helper while
preserving the existing platform dispatch.
- Around line 428-435: Remove the ELF-only format guard from
_require_exported_symbols and validate required against info.exported_symbols
for ELF, Mach-O, and PE, using the existing _macho_exported_symbols and
_pe_exported_symbols paths. Extend the related tests to cover successful
required-symbol matches for all three formats.
- Around line 204-222: Store section_link and section_entry_size as separate
tuple fields in the sections collection, update its annotation and all
unpacking/indexing in the dynamic-symbol handling flow accordingly, and use
sh_link only for string_index and sh_entsize for entry_size. Add a fixture
containing a valid ELF section header and dynamic symbol table so
_require_exported_symbols validates a genuine binary without the false
invalid-table assertion.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 104-108: Update the smoke-test step to select the
platform-appropriate interpreter explicitly rather than chaining duplicate
probes with ||, so assertion failures remain visible and only the valid
executable is invoked. Extract the shared validation into a checked-in script
such as scripts/wheel_smoke.py, and update the corresponding required-substring
checks in verify_ci_workflow.py and its test fixture to match the new workflow
content.
In `@scripts/verify_ci_workflow.py`:
- Around line 1282-1286: Update the wheels workflow validation around wheels_job
and the release matrix so it matches the unconditional smoke step: remove the
obsolete native matrix key and the native: false guard, or retain that metadata
while validating that the smoke step has no if: condition. Ensure entries
missing native cannot bypass the intended smoke-step coverage check.
In `@scripts/verify_wheel.py`:
- Around line 970-995: Update the exception handling in main around verify_wheel
to also catch struct.error raised by parser offset reads, producing the existing
“wheel verification failed” handling instead of an uncaught traceback. Preserve
the current handling for the exceptions already covered.
- Around line 227-243: Update the symbol-entry loop around the symbol parsing
logic to process only complete entries, ensuring each iteration’s offset plus
entry_size does not exceed symbol_offset + symbol_length. Skip or stop before
parsing a ragged final entry so struct.unpack_from cannot read beyond the
declared symbol table.
In `@tests/test_verify_wheel.py`:
- Around line 108-146: Add positive export-symbol coverage for ELF, Mach-O, and
PE in the tests around test_native_binary_header_matches_wheel_platform. Build
fixtures containing a valid ELF SHT_DYNSYM with its linked string table, a
Mach-O LC_SYMTAB with the exported symbol, and a PE export directory exposing
xy_abi_version, then call _require_exported_symbols and assert acceptance.
Ensure the ELF fixture exercises section-table symbol parsing, including valid
sh_link and sh_entsize values, rather than relying on the existing header-only
binaries.
- Around line 174-184: Expand
test_elf_linkage_validates_glibc_floor_and_dependency_family to assert rejection
of an unexpected DT_NEEDED dependency and to exercise the musllinux_ interpreter
branch, using the existing _elf_linkage_fixture and _require_elf_linkage
helpers. If dependency-family and musl coverage are not added, rename the test
to describe only glibc-floor validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5010e04b-b00c-4ee4-af95-07c3ac3cdbb7
📒 Files selected for processing (5)
.github/workflows/release.ymlscripts/verify_ci_workflow.pyscripts/verify_wheel.pytests/test_verify_ci_workflow.pytests/test_verify_wheel.py
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/verify_ci_workflow.py`:
- Around line 1290-1297: Update the release wheel validation around
_matrix_include_entries and reject an empty matrix_entries list before
evaluating native flags. Preserve the existing failure for any entry whose
native value is not true, ensuring workflows using unsupported matrix forms
cannot bypass runtime verification.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18a4319a-e011-47ef-b8ac-caee81cd0dc2
📒 Files selected for processing (3)
scripts/verify_ci_workflow.pyscripts/verify_wheel.pytests/test_verify_wheel.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_verify_wheel.py
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/wheel_smoke.py`:
- Around line 12-16: Replace the bare assertions in the smoke-test checks with
explicit conditionals that raise an appropriate exception when the backend,
version, codes, or unique values do not match the expected results. Update the
checks around kernels.BACKEND, reflex_xy.__version__, and factorize_fixed so
validation remains active under optimized Python execution.
In `@spec/process/production-readiness.md`:
- Line 390: Update the release checklist and design-dossier platform matrix to
match release.yml: retain only Linux x86-64, macOS arm64, Windows x64, and the
separate PyEmscripten wheel, and remove aarch64, armv7, musllinux, macOS x86-64,
Windows x86, and Windows arm64 entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55d7d25c-508c-4b91-aeca-04acf32fb20f
📒 Files selected for processing (6)
.github/workflows/release.ymlscripts/verify_ci_workflow.pyscripts/verify_wheel.pyscripts/wheel_smoke.pyspec/process/production-readiness.mdtests/test_verify_wheel.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/release.yml
- scripts/verify_ci_workflow.py
- scripts/verify_wheel.py
- tests/test_verify_wheel.py
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/verify_ci_workflow.py">
<violation number="1" location="scripts/verify_ci_workflow.py:580">
P1: The release validator can accept a workflow that does not execute `scripts/wheel_smoke.py`: the new check only looks for its text inside active run lines, so an `echo` or other non-invoking command passes. Requiring the smoke command as an exact executable line, preferably through a hard-gated exact-command step or shell control-flow validation, would preserve the native runtime publication gate.</violation>
<violation number="2" location="scripts/verify_ci_workflow.py:1299">
P3: An invalid wheel smoke gate is reported twice because the same `_require_step_run_contains` check is duplicated; removing the second invocation keeps one actionable diagnostic instead of duplicate errors.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
FarhanAliRaza
left a comment
There was a problem hiding this comment.
I built the PR head and ran the new gate against the wheel. The header and ABI parsing works. The linkage policy rejects the wheel the release job actually builds, and the branch no longer applies to main.
Required changes (details inline):
- glibc allowlist fails the CI wheel. A
cargo-zigbuild --target x86_64-unknown-linux-gnu.2.17build, packed exactly as the release job packs it, fails--require-linkagewithunsupported glibc dependencies: ['libdl.so.2', 'libpthread.so.0']. Use the PEP 599 manylinux2014 library set. - Windows allowlist probably misses
bcryptprimitives.dll. Rust std imports it for the default-hasherHashMapthat the core uses. Check the import table of a realwin_amd64build and allowlist what it needs. - Merge
main.release.ymlis nowbuild_release_artifacts.yml, andvalidate_release_workflowmoved out ofverify_ci_workflow.py. There are four conflicting files. - Dropping 8 of 11 wheel targets needs explicit maintainer sign-off, or runtime legs on
ubuntu-24.04-arm,windows-11-arm, Alpine containers or QEMU. The dossier §F1 edits currently turn a CI gap into product scope. - Ruff fails (RUF043, and three files need reformatting). Also remove the duplicated
_require_step_run_containsblock.
What works:
pytest tests/test_verify_wheel.py tests/test_verify_ci_workflow.py→ 157 passed.verify_ci_workflow.pypasses on the PR head.- The glibc floor check correctly rejects a wheel built against host glibc 2.39 but tagged
manylinux_2_17(requires glibc 2.35). xy_abi_versionis found in the ELF dynsym.scripts/wheel_smoke.pypasses in a clean venv with the zig-built wheel installed.- A 200k-point categorical scatter exported from that installed wheel renders correctly in headless Chromium, with no console errors.
No news fragment is needed, because the diff touches no files under python/, src/ or js/.
Generated by Claude Code
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| smoke_modes = set(re.findall(r"\bsmoke: ([A-Za-z0-9-]+)", "\n".join(wheel_entries))) | ||
| assert smoke_modes == {"host", "manylinux-armv7", "alpine", "alpine-armv7"} |
There was a problem hiding this comment.
Missing smoke modes go unnoticed
The new smoke_modes set checks which modes appear anywhere, but no longer checks that every wheel entry has one. Removing smoke: host from one target still passes because other targets retain it. That target then skips every smoke step and still uploads its wheel.
Keep the per-entry assertion alongside the set check so future matrix edits cannot silently remove a wheel’s install test.
| smoke_modes = set(re.findall(r"\bsmoke: ([A-Za-z0-9-]+)", "\n".join(wheel_entries))) | |
| assert smoke_modes == {"host", "manylinux-armv7", "alpine", "alpine-armv7"} | |
| assert all(re.search(r"\bsmoke: [A-Za-z0-9-]+", line) for line in wheel_entries), wheel_entries | |
| smoke_modes = set(re.findall(r"\bsmoke: ([A-Za-z0-9-]+)", "\n".join(wheel_entries))) | |
| assert smoke_modes == {"host", "manylinux-armv7", "alpine", "alpine-armv7"} |
Knowledge Base Used: Build, packaging, and release validation
Preserve PR behavior while integrating the shared CI build and selection changes.
Merging this PR will not alter performance1 warning, 1 note
Comparing Footnotes
|
Fixes #448
Summary
xy_abi_versionsymbol and validate Linux, macOS, and Windows linkage policy.bcryptprimitives.dllandntdll.dllimports.expect-artifactscontract aligned, and document the supported prebuilt wheel targets.Local validation
python -m pytest tests/test_verify_wheel.py tests/test_release_process.py tests/test_kernels.py -q: 193 passed, 1 skipped.python scripts/verify_ci_workflow.py: passed.git diff --check: passed.cargo test: 137 passed.scripts/abi_smoke.py: 140 checks passed against the built Windows DLL.xy-0.0.0-py3-none-win_amd64.whl; the verifier passed platform, ABI export, and linkage checks against the wheel and real DLL.uvx reflex-release@0.1.0a4 sync --check, Ruff, andgit diff --check: passed.The native install/load matrix cannot run on this Windows development machine; the hosted release workflow must supply the cross-platform runtime proof. The docs-site pytest could not be collected locally because the docs environment dependency
reflex_baseis not installed.Platform coverage
The release matrix retains all eleven previously published native wheel targets. Each wheel gets an install and native-kernel smoke on a matching native runner, with ARMv7 targets exercised in QEMU containers and musllinux wheels installed in Alpine. The artifact contract and installation/release documentation list the same complete target set.
Summary by CodeRabbit
Release Improvements
Reliability
Documentation