Repository navigation
[ISSUE #7563] Harden wasm fixture verification. - #7581
Conversation
|
@wy471x This focused fixture-verification follow-up is ready for review before #7569. I verified that #7569 contains this exact prerequisite commit and that the shared verifier, verifier regressions, workflow and WasmLoader file are identical between the two branches. Combined validation with the current upstream master passes all nine Python verifier regressions and all 102 WASM Java tests against the committed fixtures, with zero failures/errors/skips and no Checkstyle violations. The export-lookup policy remains the merged strict policy; the production loader difference here is documentation only. Please review this focused prerequisite first. The subsequent #7569 synchronization is prepared locally; its publication and follow-up review remain gated on this PR landing, preserving the documented merge order. |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Review: APPROVE
Good hardening of the WASM fixture verification pipeline (issue #7563), with offline coverage added.
What's good
verify-wasm-fixtures.pynow requires thewasm-gatewayfamily and iterates everywasm-*directory (failing fast when one is not a directory), so newly added families likewasm-concurrent(from #7582) are automatically verified byte-for-byte without modifying committed files.verify_rustdrops the--offlineflag, so Cargo may fetch dependencies in CI (the verifier still restores original resource bytes after success, ordinary failure, orKeyboardInterrupt).- A new
test-wasm-fixtures.pyprovides offline regression tests for the verification logic itself (mockingsubprocess), covering missing/extra/empty families, directory-type checks, and resource restoration on partial build / maven / keyboard-interrupt failures.wasm-fixtures.ymlwires this into bothpull_requestandpushtriggers. WasmLoader.javaonly gains a clarifying Javadoc onfindWasmExtern;README.mddocuments the new behavior.
Non-blocking note
- None blocking. The WasmLoader Javadoc-only change here overlaps with #7582's body rewrite of the same method — see the cross-PR note on #7582 for merge-order guidance.
Clean and well-tested — approving.
|
@yangjj-iso This PR currently has merge conflicts with Root cause: the conflict is in Please rebase this PR onto the latest |
Signed-off-by: redyyang <[email protected]>
67db1a8 to
2c864b5
Compare
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Summary
Hardens wasm fixture verification (#7563). The verifier now requires the base module's wasm-gateway and the API module's wasm-concurrent families, validates every wasm-* directory byte-for-byte (rejecting non-directories and empty families), and a new offline test script covers the failure/cleanup paths.
What the change does
verify-wasm-fixtures.py: family-existence checks forwasm-gateway/wasm-concurrent, then iteratesglob("wasm-*")in both roots with explicitRuntimeErrors for non-directories / empty output.test-wasm-fixtures.py(new): offline regression tests forverify_wat()andverify_rust()(missing/empty/non-dir families, resource restoration on success, partial build failure, Maven failure,KeyboardInterrupt).wasm-fixtures.yml: runs the new test script and triggers on its changes.WasmLoader.java: javadoc-only change.
Verification
- CI green (
pr_build,Analyze (java),rat_check,check-license-header,scope,sql-matrix). - The Rust build drops
--offline; the README documents that cargo may fetch dependencies and that no pre-populated offline cache is required.
Notes (non-blocking)
- Removing
--offlinemeans thewasm-fixturesjob now needs network access to the cargo registry; documented and acceptable, but a potential source of CI flakiness worth watching.
Conclusion
APPROVE.
WASM fixture verification can silently accept missing fixture families and omit newly added WAT directories. Require the base module's
wasm-gatewayand the API module'swasm-concurrent, discover everywasm-*directory in both modules, and reject missing, malformed, empty or byte-mismatched families.Rebased onto upstream
masteratad89a887aafter the instance-pool rewrite in #7582. Preserve the merged pool implementation and concurrent fixture coverage; theWasmLoaderchange remains Javadoc only. Remove Cargo's forced offline mode and document the clean-checkout requirement and cleanup limits after forced process termination.This is the focused prerequisite for #7569; merge this fixture-verification follow-up first. The bridge PR will need its own synchronization with the current instance-pool implementation afterward.
Validation on the rebased head:
All 14 Python verifier regression checks passed, including five checks for the API/concurrent fixture family and additional API fixture directories.
All 55 WASM Java tests passed on Java 21 against committed fixtures, with zero failures, errors or skips; the affected 33-module reactor passed Checkstyle.
Apache RAT passed across the affected reactor;
git diff --checkpassed.The pinned WABT/Rust rebuild workflow has been triggered for this head. Full local WAT/Rust rebuilds and a full-repository Maven install were not repeated for this update.
Read the contribution guidelines.
Add regression tests.
Full-repository Maven install; affected-reactor validation is recorded above.