fix(perry-ffi): unbreak cargo test -p perry-ffi (undefined js_register_ffi_handle_exists_probe)#5088
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds conditional compilation logic to ChangesFFI Symbol Stub for Test Linking
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@CHANGELOG.md`:
- Around line 1-30: Remove the changelog entry introduced in this PR: delete the
"## v0.5.1164 — fix(perry-ffi): unbreak `cargo test -p perry-ffi` (undefined
`js_register_ffi_handle_exists_probe`)" section from CHANGELOG.md (the entire
paragraph shown in the diff) so the PR does not modify release notes; leave the
rest of the file untouched or restore it to the state prior to this PR.
🪄 Autofix (Beta)
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: 005afe20-4855-4d19-8cce-badd66a8c1a6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-ffi/src/handle.rs
Comment on lines
+1
to
+30
| ## v0.5.1164 — fix(perry-ffi): unbreak `cargo test -p perry-ffi` (undefined `js_register_ffi_handle_exists_probe`) | ||
|
|
||
| The `cargo-test` CI gate was red on `main`: #5083 added a | ||
| `js_register_ffi_handle_exists_probe(...)` call inside `register_handle` | ||
| (`crates/perry-ffi/src/handle.rs`) to wire up the runtime's handle-vs-timer | ||
| disambiguation probe. That symbol is defined in `perry-runtime`, but | ||
| `perry-ffi`'s `runtime-link` feature (which pulls `perry-runtime` into the | ||
| Cargo graph) is **off** by default, and CI runs `cargo test -p perry-ffi` | ||
| per-package in isolation — no `--workspace` feature unification. So the | ||
| handle-registry tests (a bare `#[cfg(test)]` module that calls | ||
| `register_handle`) retained a reference to a symbol that wasn't linked, and the | ||
| test binary failed at `cc` with `undefined reference to | ||
| js_register_ffi_handle_exists_probe`. | ||
|
|
||
| All other `perry-ffi` test modules avoid this by gating on | ||
| `#[cfg(all(test, feature = "runtime-link"))]`, but gating the handle tests the | ||
| same way would drop them from CI entirely (the per-package loop never enables | ||
| the feature). Instead, this adds a **test-only no-op stub** for the symbol, | ||
| gated `#[cfg(all(test, not(feature = "runtime-link")))]`, so the registry tests | ||
| keep running and the binary links. The real `extern` declaration is gated out | ||
| of that exact configuration to avoid an `E0428` name clash. The stub can never | ||
| collide with `perry-runtime`'s real definition: it exists only in `perry-ffi`'s | ||
| own `test` binary with `runtime-link` off — not in `runtime-link`-on builds | ||
| (real symbol used) nor in the lib builds that npm wrappers link against | ||
| `libperry_runtime.a`. | ||
|
|
||
| Verified: `cargo test -p perry-ffi` now links and passes (13 tests); `cargo test | ||
| -p perry-ffi --features runtime-link` still uses the real perry-runtime symbol | ||
| and passes (35 tests), no collision. | ||
|
|
There was a problem hiding this comment.
Remove this changelog entry from the PR.
This release note belongs in the maintainer merge step, not in an external-contributor PR. As per coding guidelines, CHANGELOG.md is updated when changes land on main, and the recorded learning says contributors should not modify it here. As per coding guidelines, CHANGELOG.md entries are required for every change that lands on main; according to the repo learning, external contributor PRs should not modify CHANGELOG.md, and the maintainer adds release notes at merge time.
🤖 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 `@CHANGELOG.md` around lines 1 - 30, Remove the changelog entry introduced in
this PR: delete the "## v0.5.1164 — fix(perry-ffi): unbreak `cargo test -p
perry-ffi` (undefined `js_register_ffi_handle_exists_probe`)" section from
CHANGELOG.md (the entire paragraph shown in the diff) so the PR does not modify
release notes; leave the rest of the file untouched or restore it to the state
prior to this PR.
Sources: Coding guidelines, Learnings
…ter_ffi_handle_exists_probe) #5083 added a `js_register_ffi_handle_exists_probe(...)` call inside `register_handle` (handle.rs) for the runtime's handle-vs-timer disambiguation probe. That symbol is defined in perry-runtime, but perry-ffi's `runtime-link` feature is off by default and CI runs `cargo test -p perry-ffi` per-package in isolation (no --workspace feature unification). The handle-registry tests (bare `#[cfg(test)]`) call `register_handle`, so the test binary retained a reference to an unlinked symbol and failed at `cc` with `undefined reference`. Gating the handle tests on `runtime-link` (the pattern other perry-ffi test modules use) would drop them from CI entirely (the per-package loop never enables the feature). Instead add a test-only no-op stub gated `#[cfg(all(test, not(feature = "runtime-link")))]` so the registry tests keep running and the binary links; the real extern declaration is gated out of that exact config to avoid an E0428 name clash. The stub can never collide with perry-runtime's real definition (it exists only in perry-ffi's own test binary with runtime-link off). Verified: `cargo test -p perry-ffi` links + passes (13); `cargo test -p perry-ffi --features runtime-link` uses the real symbol + passes (35).
proggeramlug
force-pushed
the
fix/perry-ffi-test-link-handle-probe
branch
from
defdf41 to
3a1c5ef
Compare
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
1 participant
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.
Problem
The
cargo-testCI gate is red onmain(e.g. #5083's own run, and every PR branched off it — including #5086). The failure is a link error, not a test failure — everytest resultline shows0 failed, then the job dies atcc:Root cause
#5083 added a
js_register_ffi_handle_exists_probe(...)call insideregister_handle(crates/perry-ffi/src/handle.rs) to wire up the runtime's handle-vs-timer disambiguation probe. That symbol is defined inperry-runtime, whichperry-ffionly pulls in behind itsruntime-linkfeature (off by default).perry-ffi's handle-registry tests live in a bare#[cfg(test)]module (unlike every otherperry-ffitest module, which gates on#[cfg(all(test, feature = "runtime-link"))]). They callregister_handle, so once #5083 maderegister_handlereference the runtime symbol, the default test binary retained an undefined reference. It only surfaces in isolation because CI runscargo test -p $packageper-package in a loop (see.github/workflows/test.yml) — there's nocargo test --workspace, so feature unification from theperry-ext-*dev-deps never pullsruntime-linkin.Fix
Gating the handle tests on
runtime-linklike the other modules would drop them from CI entirely (the per-package loop never enables the feature). Instead, add a test-only no-op stub for the symbol, gated#[cfg(all(test, not(feature = "runtime-link")))], so the registry tests keep running and the binary links. The realexterndeclaration is gated out of that exact configuration to avoid anE0428name clash.The stub can never collide with
perry-runtime's real definition — it exists only inperry-ffi's owntestbinary withruntime-linkoff:runtime-linkon → stub excluded, realperry-runtimesymbol used.testlib builds (npm wrappers) → stub excluded; final binary linkslibperry_runtime.a.Verification
cargo test -p perry-ffi(default, the failing config) → links + passes, 13 tests.cargo test -p perry-ffi --features runtime-link→ uses the real perry-runtime symbol, passes, 35 tests, noE0428, no duplicate-symbol.Touches only
crates/perry-ffi/src/handle.rs(+ version/changelog).This unblocks the
cargo-testgate for the whole repo. Recommended to merge before #5086 (the loop-guard perf PR), which currently inherits this same redcargo-test; after this lands I'll rebase #5086 and its CI will go green.Summary by CodeRabbit