Skip to content

fix(perry-ffi): unbreak cargo test -p perry-ffi link on Linux (cargo-test gate)#5089

Closed
proggeramlug wants to merge 1 commit into
mainfrom
fix/perry-ffi-test-link
Closed

fix(perry-ffi): unbreak cargo test -p perry-ffi link on Linux (cargo-test gate)#5089
proggeramlug wants to merge 1 commit into
mainfrom
fix/perry-ffi-test-link

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Problem

The cargo-test CI gate has been red since v0.5.1164 / #5083 (the http2 server.close() timer-id collision fix). Every PR opened against main since then fails cargo-test.

The failure is a link error, not a test assertion:

error: linking with `cc` failed: exit status: 1
  = note: /usr/bin/ld: ... in function `perry_ffi::handle::ensure_handle_exists_probe_registered::{{closure}}':
          undefined reference to `js_register_ffi_handle_exists_probe'
error: could not compile `perry-ffi` (lib test) due to 1 previous error

Root cause

#5083 added a perry-runtime hook, js_register_ffi_handle_exists_probe, declared as an extern "C" in crates/perry-ffi/src/handle.rs and called from ensure_handle_exists_probe_registered, which register_handle invokes lazily on first handle creation.

  • In the real perry binary the symbol resolves — perry-runtime is always linked in.
  • But the perry-ffi lib-test binary does not link perry-runtime, so the symbol is undefined there. perry-ffi's unit tests (round_trip_simple_value, handles_are_unique, …) all call register_handle, so the probe call is on a test-reachable path and survives --gc-sections. Linux ld treats the undefined reference as a hard error and aborts the gate.
  • macOS's linker resolves it lazily, so cargo test -p perry-ffi passed locally and the regression slipped through.

The crate's other runtime hooks (perry_ffi_gc_register_*) stay undefined harmlessly: no test-reachable code calls them, so the linker drops their references.

Fix

Split the probe out of the shared extern "C" block:

  • #[cfg(not(test))] extern declaration — production, unchanged (byte-identical codegen).
  • #[cfg(test)] #[no_mangle] no-op definition — gives the lib-test binary a local symbol to link against.

The cfg(test) stub only applies when perry-ffi is the crate under test; when perry-ffi is built as a dependency for another crate it uses the extern declaration, so there's no duplicate-symbol risk against the real perry-runtime definition.

Validation

  • cargo test -p perry-ffi13 passed (was: link error).
  • cargo build -p perry-ffi (non-test) clean, no warnings.
  • cargo fmt clean.

Version bumped to 0.5.1165 with a CHANGELOG entry per the workflow.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed test link failures on Linux that prevented successful test execution.
  • Chores

    • Bumped version to 0.5.1165.

…break cargo-test link on Linux

PR #5083 added the perry-runtime hook js_register_ffi_handle_exists_probe,
declared as an extern "C" in handle.rs and called from register_handle via
ensure_handle_exists_probe_registered. The perry-ffi lib-test binary does not
link perry-runtime, so the symbol is undefined; because the unit tests call
register_handle, the reference survives --gc-sections and Linux ld fails the
link (the cargo-test CI gate). macOS resolves it lazily, hiding it locally.

Split the probe out of the shared extern block: keep the extern declaration
under #[cfg(not(test))] (production, unchanged) and add a #[cfg(test)]
#[no_mangle] no-op definition so the lib-test binary has a local symbol.
Production codegen is byte-identical. cargo test -p perry-ffi now passes.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This release pins a Linux linker failure in perry-ffi by adding a test-time no-op stub for the missing js_register_ffi_handle_exists_probe FFI symbol. The fix uses conditional compilation to avoid breaking production builds, then version identifiers and release notes are updated to v0.5.1165 across the workspace.

Changes

FFI Link Fix and Release v0.5.1165

Layer / File(s) Summary
FFI handle probe symbol test stub
crates/perry-ffi/src/handle.rs
The js_register_ffi_handle_exists_probe extern "C" symbol is conditionally compiled: declared as an external symbol under cfg(not(test)) to link against perry-runtime in production, and defined as a #[no_mangle] no-op stub under cfg(test) to resolve undefined-symbol link failures in unit tests.
Version bump and release documentation
Cargo.toml, CLAUDE.md, CHANGELOG.md
Workspace version is incremented from 0.5.1164 to 0.5.1165 in Cargo.toml and documentation metadata in CLAUDE.md. CHANGELOG.md documents the fix, explaining the conditional extern/cfg(test) handling and that production codegen is unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A linker's cry echoes through the test,
One symbol missing makes the build distressed—
So with cfg(test) and #[no_mangle] grace,
A stub finds its way to the right place!
Now perry-ffi tests dance free,
Version five-one-one-six-five, it shall be! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description violates the repository's template requirements by editing CHANGELOG.md, CLAUDE.md, and Cargo.toml version fields, which the template explicitly states the maintainer handles at merge time. Remove version bumps from Cargo.toml, CLAUDE.md, and CHANGELOG.md entries; the maintainer will add these at merge time. Keep only the fix to crates/perry-ffi/src/handle.rs and the summary/changes/test plan sections.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(perry-ffi): unbreak cargo test -p perry-ffi link on Linux (cargo-test gate)' directly and clearly summarizes the main change—fixing a linker error in perry-ffi tests on Linux.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/perry-ffi-test-link

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-26: Remove the contributed CHANGELOG entry titled "v0.5.1165 —
fix(perry-ffi): unbreak the `cargo test -p perry-ffi` link on Linux" from the PR
by reverting the edits to CHANGELOG.md that add that paragraph; leave the file
as it was in the base branch so maintainers can add release notes at merge time.
🪄 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: e36e1bc3-0867-4c5e-860f-8f909625baab

📥 Commits

Reviewing files that changed from the base of the PR and between 9b6bbe0 and a0b93cd.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • CHANGELOG.md
  • CLAUDE.md
  • Cargo.toml
  • crates/perry-ffi/src/handle.rs

Comment thread CHANGELOG.md
Comment on lines +1 to +26
## v0.5.1165 — fix(perry-ffi): unbreak the `cargo test -p perry-ffi` link on Linux

The `cargo-test` CI gate started failing after v0.5.1164 (#5083, the http2
`server.close()` timer-id collision fix). That PR added a perry-runtime hook,
`js_register_ffi_handle_exists_probe`, declared as an `extern "C"` in
`crates/perry-ffi/src/handle.rs` and called from
`ensure_handle_exists_probe_registered`, which `register_handle` invokes
lazily on first handle creation.

In the real `perry` binary the symbol resolves because perry-runtime is always
linked in. But the **`perry-ffi` lib-test binary does not link perry-runtime**,
so the symbol is undefined there. `perry-ffi`'s unit tests (`round_trip_simple_value`,
`handles_are_unique`, …) all call `register_handle`, so the probe call sits on
a test-reachable path and survives `--gc-sections`. On Linux `ld` an undefined
reference is a hard link error (`undefined reference to
'js_register_ffi_handle_exists_probe'`), aborting the gate. macOS's linker
resolves it lazily, so it passed locally. The crate's other runtime hooks
(`perry_ffi_gc_register_*`) stay undefined harmlessly: no test-reachable code
calls them, so the linker drops their references.

Fix: split the probe out of the shared `extern "C"` block into a
`#[cfg(not(test))]` extern declaration (production, unchanged) plus a
`#[cfg(test)] #[no_mangle]` no-op definition that gives the lib-test binary a
local symbol to link against. Production codegen is byte-identical; only the
test binary gains the stub. `cargo test -p perry-ffi` now links and passes
(13 tests).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Please drop this changelog entry from the contributor PR.

This repo’s release policy says external contributor PRs should not edit CHANGELOG.md; maintainers add the release note at merge time.
Based on learnings, external contributor PRs should not touch CHANGELOG.md; maintainers bump version and write the changelog 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 - 26, Remove the contributed CHANGELOG entry
titled "v0.5.1165 — fix(perry-ffi): unbreak the `cargo test -p perry-ffi` link
on Linux" from the PR by reverting the edits to CHANGELOG.md that add that
paragraph; leave the file as it was in the base branch so maintainers can add
release notes at merge time.

Source: Learnings

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate of #5088, which was opened first and fixes the same cargo-test link failure (#5083's undefined js_register_ffi_handle_exists_probe in the perry-ffi lib-test binary).

#5088's gating is also more correct: it stubs the symbol under #[cfg(all(test, not(feature = "runtime-link")))], so it avoids a duplicate-symbol clash with the real perry-runtime definition under cargo test -p perry-ffi --features runtime-link — which my plain #[cfg(test)] gating would have hit. Deferring to #5088.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant