feat(plugin): enable Windows plugin support#5409
Merged
proggeramlug merged 4 commits intoJun 19, 2026
Merged
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:
📝 WalkthroughWalkthroughCross-platform plugin loading is implemented by adding ChangesWindows Plugin Lifecycle Support
Sequence Diagram(s)sequenceDiagram
participant Shell as test_plugin_lifecycle.sh
participant PerryCLI as perry compile
participant HostBin as host (compiled binary)
participant Runtime as perry-runtime
participant DyLib as plugin.dylib/.so/.dll
Shell->>PerryCLI: compile plugin.ts --dylib
PerryCLI-->>Shell: plugin.{dylib,so,dll}
Shell->>PerryCLI: compile host.ts
PerryCLI-->>Shell: host{.exe}
Shell->>HostBin: run host <plugin-path>
HostBin->>Runtime: perry_plugin_load(path)
Runtime->>DyLib: open_library → LoadLibraryW / dlopen
Runtime->>DyLib: lookup_symbol("plugin_activate") → GetProcAddress / dlsym
DyLib-->>Runtime: plugin_activate fn pointer
Runtime-->>HostBin: plugin_id
HostBin->>Runtime: emit "transform" hook {name:"perry"}
Runtime->>DyLib: transform hook → uppercases name
DyLib-->>HostBin: {name:"PERRY"}
HostBin->>Runtime: invoke "greet" tool {who:"windows"}
Runtime->>DyLib: greet tool
DyLib-->>HostBin: "hello, windows"
HostBin->>Runtime: perry_plugin_unload(plugin_id)
Runtime->>DyLib: lookup_symbol("plugin_deactivate") → optional
Runtime->>DyLib: close_library → FreeLibrary / dlclose
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 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: 3
🧹 Nitpick comments (1)
crates/perry-runtime/src/plugin.rs (1)
143-148: 💤 Low valueConsider using
FormatMessageWfor human-readable Windows error messages.The current implementation returns only a numeric error code. Windows provides
FormatMessageWwithFORMAT_MESSAGE_FROM_SYSTEMto convert error codes into descriptive strings, which would improve debuggability.This is a nice-to-have enhancement — the numeric codes are searchable and sufficient for debugging.
🤖 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 `@crates/perry-runtime/src/plugin.rs` around lines 143 - 148, The error handling code in plugin.rs is currently returning only numeric Windows error codes. To improve debuggability, use the Windows API function FormatMessageW with the FORMAT_MESSAGE_FROM_SYSTEM flag to convert the numeric error code into a human-readable error message string. Locate the code section (around lines 143-148) where the error code is being returned or logged, and replace the numeric code with the formatted error message obtained from FormatMessageW. This will make Windows errors much more descriptive and easier to debug without requiring users to manually look up error codes.
🤖 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 `@crates/perry-ui-windows/src/ffi/widget_layout_extras.rs`:
- Around line 1-2: The file-level comment at the top of widget_layout_extras.rs
references "plugin stubs" and mentions functions that have been removed from the
file. Update the file-level documentation comment to remove references to the
removed functions (perry_plugin_load and perry_plugin_unload) and instead
accurately describe what the file currently contains, such as its actual FFI
exports and widget layout functionality.
In `@tests/test_plugin_lifecycle.sh`:
- Line 19: The set -e option at the beginning of the script causes immediate
termination when any command fails, preventing the explicit error handling logic
at the EXIT variable assignment and diagnostic block from executing. Remove or
modify the set -e directive to allow the script to continue execution so that
the error handling code at lines 144-150 can properly capture the exit status
with EXIT=$? and run the diagnostic output even when commands fail, making
failures more actionable with proper error reporting.
- Line 144: The plugin reference in the test uses `plugin.$PLUGIN_EXT` without
an explicit path prefix, which allows the operating system's plugin loader to
resolve it via system search paths instead of deterministically from the temp
directory, causing potential test flakiness. Modify any references to the plugin
(where `plugin.$PLUGIN_EXT` is used) to include an explicit relative path prefix
by changing it to `./plugin.$PLUGIN_EXT` to ensure consistent deterministic
resolution from the temporary directory.
---
Nitpick comments:
In `@crates/perry-runtime/src/plugin.rs`:
- Around line 143-148: The error handling code in plugin.rs is currently
returning only numeric Windows error codes. To improve debuggability, use the
Windows API function FormatMessageW with the FORMAT_MESSAGE_FROM_SYSTEM flag to
convert the numeric error code into a human-readable error message string.
Locate the code section (around lines 143-148) where the error code is being
returned or logged, and replace the numeric code with the formatted error
message obtained from FormatMessageW. This will make Windows errors much more
descriptive and easier to debug without requiring users to manually look up
error codes.
🪄 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: 02a4473d-5334-43d2-b133-f463b49387d4
📒 Files selected for processing (6)
crates/perry-runtime/Cargo.tomlcrates/perry-runtime/src/plugin.rscrates/perry-ui-windows/src/ffi/widget_layout_extras.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/link/mod.rstests/test_plugin_lifecycle.sh
nglmercer
force-pushed
the
feature/windows-plugin-support
branch
2 times, most recently
from
3da25f5 to
da56bcb
Compare
Compile-side: - crates/perry/src/commands/compile.rs: add Windows arm to the `--output-type dylib` link path. On Linux this is `cc -shared`; on macOS `cc -dynamiclib -flat_namespace -undefined dynamic_lookup`; on Windows we now invoke `link /NOLOGO /DLL /OUT:<path>`. The DLL leaves perry_* / js_* external references unresolved, the same way the macOS dylib does — they bind to the host process at LoadLibrary time. - crates/perry/src/commands/compile/link/mod.rs: when `ctx.needs_plugins && is_windows`, write a per-build `perry_plugin_host_<pid>.def` listing the runtime + plugin-manager exports and pass `/DEF:<path>` to link.exe. This is the MSVC equivalent of `-rdynamic` / `-Wl,-u,_<sym>`. The shared PLUGIN_HOST_SYMBOLS const is now the single source of truth for both the macOS `-u` force-keep list and the Windows .def file; Linux still uses `-rdynamic`. The Windows /OPT:REF + /OPT:ICF defaults are overridden to /OPT:NOREF for plugin hosts so dead strip does not discard exports the .def file is preserving. Runtime: - crates/perry-runtime/src/plugin.rs: factor open_library / lookup_symbol / close_library / last_load_error into cfg-gated helpers, then drop the `#[cfg(unix)]` gates on perry_plugin_load and perry_plugin_unload. Unix keeps `libc::dlopen` / `dlsym` / `dlclose`; Windows now uses `LoadLibraryW` / `GetProcAddress` / `FreeLibrary` (via windows-sys 0.61). The `LibHandle` newtype stays platform-agnostic (`*mut c_void`) so the rest of the registry/dispatch surface is unchanged. ABI-check, activate, deactivate, hook/tool/service/ route/event registration all work the same on Windows. - crates/perry-runtime/Cargo.toml: enable `Win32_System_LibraryLoader` on the existing windows-sys block. (FreeLibrary is actually exported from `Win32::Foundation`, not the LibraryLoader module, so the import path lives in code.) UI: - crates/perry-ui-windows/src/ffi/widget_layout_extras.rs: drop the no-op `perry_plugin_load` (returned 0) and `perry_plugin_unload` stubs. The real implementations in perry-runtime now link through for both console and UI plugin hosts. Test: - tests/test_plugin_lifecycle.sh: end-to-end bash test that compiles a `plugin.ts` (with `registerHook` + `registerTool` + `setMetadata` + `deactivate`) and a `host.ts` (with `loadPlugin` / `emitHook` / `invokeTool` / `unloadPlugin`), runs the host against the plugin, and asserts the hook transformed the input and the tool returned the expected greeting. Detects the host platform and uses the right extensions (`.dll`/`.exe` on Windows, `.so` on Linux, `.dylib` on macOS); skips on unsupported hosts. Not changed: types/perry/plugin/index.d.ts (no surface change), docs/src/plugins/* (existing macOS/Linux docs still apply), and no version bump or CHANGELOG entry per CLAUDE.md (this is a contributor PR — the maintainer folds in the patch bump and changelog at merge time).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/compile.rs (1)
4931-4939:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake dylib defaults target-aware for Windows.
default_output_path()still chooses.sofrom the host cfg before the Windows dylib branch runs, so--target windows --output-type dylibwithout-oemits a PE DLL named*.soinstead of*.dll.🐛 Proposed fix
if is_dylib { - #[cfg(target_os = "macos")] - { - PathBuf::from(format!("{}.dylib", stem)) - } - #[cfg(not(target_os = "macos"))] - { - PathBuf::from(format!("{}.so", stem)) - } + if matches!(target, Some("windows") | Some("windows-winui")) + || (target.is_none() && cfg!(target_os = "windows")) + { + PathBuf::from(format!("{}.dll", stem)) + } else if matches!(target, Some("macos")) + || (target.is_none() && cfg!(target_os = "macos")) + { + PathBuf::from(format!("{}.dylib", stem)) + } else { + PathBuf::from(format!("{}.so", stem)) + }Also applies to: 5297-5298
🤖 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 `@crates/perry/src/commands/compile.rs` around lines 4931 - 4939, The dylib output path selection in the `is_dylib` block within `default_output_path()` is missing Windows-specific handling. Currently it defaults to `.so` for all non-macOS targets, but Windows should use `.dll` extension instead. Add a Windows-specific configuration check using `#[cfg(target_os = "windows")]` that returns a PathBuf with `.dll` extension for the stem, placing it as a separate branch alongside the existing macOS check and the fallback non-macOS check to ensure Windows targets correctly generate `*.dll` files instead of `*.so` files when using dylib output type.
🤖 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 `@crates/perry-codegen/src/codegen/entry.rs`:
- Around line 985-1056: The plugin ABI shim code block (the if is_dylib section
containing perry_plugin_abi_version, plugin_activate, and plugin_deactivate
function definitions) is currently only present in the non-entry branch, which
causes single-file dylib plugins to skip this entire section since they have
is_entry=true. Move the entire if is_dylib block so that the ABI shim emissions
execute whenever is_dylib is true, regardless of the is_entry value, ensuring
both single-file and multi-file dylib plugins properly export the required
plugin entry points to the runtime.
In `@crates/perry/src/commands/compile.rs`:
- Around line 5299-5302: The current code checks if any native module has a
"deactivate" exported function by scanning all modules in
ctx.native_modules.values(), but the plugin_deactivate ABI shim is only emitted
from the entry module contract. To fix this, restrict the check to only the
entry module instead of iterating through all native modules. Replace the
.values().any() call with a check that only examines the entry module's exported
functions for "deactivate". Apply the same fix to the other occurrence mentioned
at lines 5348-5350, ensuring both instances only gate on the entry module.
- Around line 5342-5352: The code is ignoring write failures to the `.def` file
by using `let _ = writeln!(...)` and still passing the `/DEF:<path>` argument to
the linker even if the file creation or writes fail, which masks the real error.
Capture the results of the writeln calls instead of discarding them with `let _
= `, check if any write operation fails, and only add the `/DEF:<path>` argument
to the linker (c.arg call) if the file is successfully created and all write
operations complete successfully. If any write fails, propagate or return the
error so the real failure is visible instead of a generic linker error.
In `@crates/perry/src/commands/compile/link/build_and_run.rs`:
- Around line 427-442: The code silently ignores failures when creating the
`.def` file and unconditionally passes the path to `/DEF:`, which causes unclear
linker errors if the temp directory is not writable. Additionally, the
`def_path` is never cleaned up after linking completes, unlike other temp files
such as `response_file_to_clean` and `embedded_info_plist_path` which are
handled around line 1780. First, replace the `if let Ok(mut def_file) =
fs::File::create(&def_path)` block with proper error handling that either
ensures the file is created successfully or logs a clear error and handles the
failure gracefully. Second, add `def_path` to the cleanup logic near line
1780-1785 where other temporary files are tracked and removed after the linking
process completes, ensuring the temp `.def` file is properly deleted when no
longer needed.
- Around line 443-465: Replace the compile-time #[cfg(target_os = "...")]
attribute-based conditional blocks (the #[cfg(target_os = "macos")] and
#[cfg(target_os = "linux")] blocks) with a runtime if/else check that determines
the target OS at runtime. Use the is_linux variable that is already available in
the context to make this decision, or derive an is_macos check from the same
source. This ensures that when cross-compiling from one OS to another (for
example, from macOS to Linux), the correct linker flags are applied based on the
target platform rather than the host platform where compilation is happening.
---
Outside diff comments:
In `@crates/perry/src/commands/compile.rs`:
- Around line 4931-4939: The dylib output path selection in the `is_dylib` block
within `default_output_path()` is missing Windows-specific handling. Currently
it defaults to `.so` for all non-macOS targets, but Windows should use `.dll`
extension instead. Add a Windows-specific configuration check using
`#[cfg(target_os = "windows")]` that returns a PathBuf with `.dll` extension for
the stem, placing it as a separate branch alongside the existing macOS check and
the fallback non-macOS check to ensure Windows targets correctly generate
`*.dll` files instead of `*.so` files when using dylib output type.
🪄 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: fdcbbb21-79ce-49ba-ba18-667d4efe059a
📒 Files selected for processing (4)
crates/perry-codegen/src/codegen/entry.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/link/build_and_run.rscrates/perry/src/commands/compile/link/mod.rs
💤 Files with no reviewable changes (1)
- crates/perry/src/commands/compile/link/mod.rs
Comment on lines
+985
to
+1056
| let has_plugin_activate = hir.exported_functions.iter().any(|(name, _)| name == "activate"); | ||
| let has_plugin_deactivate = hir | ||
| .exported_functions | ||
| .iter() | ||
| .any(|(name, _)| name == "deactivate"); | ||
| drop(ctx); | ||
| llmod.ic_counter = ic_end; | ||
| llmod.buffer_alias_counter += buffer_alias_used; | ||
| llmod.native_rep_records.extend(native_rep_records); | ||
| for (name, ret, params) in pending { | ||
| llmod.declare_function(&name, ret, ¶ms); | ||
| } | ||
|
|
||
| // Plugin ABI shim — only emitted when the entry module is being | ||
| // built as a dylib (perry compile --output-type dylib). The host's | ||
| // `loadPlugin` calls `GetProcAddress(handle, "plugin_activate")` | ||
| // (Windows) / `dlsym(handle, "plugin_activate")` (macOS/Linux) to | ||
| // find the entry, so every dylib must export that name (and | ||
| // `plugin_deactivate` if the user supplied one). The shim unwraps | ||
| // the NaN-boxed `api` handle, calls the user's `activate(api)` | ||
| // with the raw pointer, and returns 1 on success / 0 if the | ||
| // module doesn't export `activate` (host treats that as load | ||
| // failure). `perry_plugin_abi_version` is the version the runtime | ||
| // checks against the host's expected ABI before calling activate | ||
| // — bump when the shim contract changes. | ||
| if is_dylib { | ||
| use crate::codegen::helpers::scoped_fn_name; | ||
| use crate::nanbox::{POINTER_MASK_I64, POINTER_TAG_I64}; | ||
|
|
||
| { | ||
| let abi_fn = llmod.define_function("perry_plugin_abi_version", I64, vec![]); | ||
| let _ = abi_fn.create_block("entry"); | ||
| let blk = abi_fn.block_mut(0).unwrap(); | ||
| blk.ret(I64, "2"); | ||
| } | ||
|
|
||
| if has_plugin_activate { | ||
| let user_activate = scoped_fn_name(module_prefix, "activate"); | ||
| llmod.declare_function(&user_activate, DOUBLE, &[DOUBLE]); | ||
| let fn_def = llmod.define_function( | ||
| "plugin_activate", | ||
| I64, | ||
| vec![(I64, "api_handle".to_string())], | ||
| ); | ||
| let _ = fn_def.create_block("entry"); | ||
| let blk = fn_def.block_mut(0).unwrap(); | ||
| let lower48 = blk.and(I64, "api_handle", POINTER_MASK_I64); | ||
| let tagged = blk.or(I64, &lower48, POINTER_TAG_I64); | ||
| let boxed = blk.bitcast_i64_to_double(&tagged); | ||
| let _ = blk.call(DOUBLE, &user_activate, &[(DOUBLE, &boxed)]); | ||
| blk.ret(I64, "1"); | ||
| } else { | ||
| let fn_def = llmod.define_function( | ||
| "plugin_activate", | ||
| I64, | ||
| vec![(I64, "_api_handle".to_string())], | ||
| ); | ||
| let _ = fn_def.create_block("entry"); | ||
| let blk = fn_def.block_mut(0).unwrap(); | ||
| blk.ret(I64, "0"); | ||
| } | ||
|
|
||
| if has_plugin_deactivate { | ||
| let user_deactivate = scoped_fn_name(module_prefix, "deactivate"); | ||
| llmod.declare_function(&user_deactivate, DOUBLE, &[]); | ||
| let fn_def = llmod.define_function("plugin_deactivate", VOID, vec![]); | ||
| let _ = fn_def.create_block("entry"); | ||
| let blk = fn_def.block_mut(0).unwrap(); | ||
| blk.call_void(&user_deactivate, &[]); | ||
| blk.ret_void(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether single-file dylib plugins get plugin_activate emitted.
# Check if there's entry-branch ABI shim emission elsewhere in the codebase.
echo "=== Checking for plugin ABI shim emission in entry branch ==="
rg -n "plugin_activate|perry_plugin_abi_version" crates/perry-codegen/src/codegen/entry.rs
echo ""
echo "=== Checking if plugins use a wrapper module pattern ==="
rg -n -A5 "output.*dylib|dylib.*output" crates/perry/src/commands/compile.rs | head -50
echo ""
echo "=== Checking compile entry module handling for dylib ==="
rg -n "is_entry.*dylib|dylib.*is_entry" crates/perry-codegen/Repository: PerryTS/perry
Length of output: 2826
🏁 Script executed:
# Check the is_entry conditional and its boundaries
sed -n '150,180p' crates/perry-codegen/src/codegen/entry.rs
echo "---"
# Check around line 666 where the non-entry branch allegedly starts
sed -n '660,680p' crates/perry-codegen/src/codegen/entry.rs
echo "---"
# Check the context around the ABI shim (lines 985-1056)
sed -n '980,1010p' crates/perry-codegen/src/codegen/entry.rsRepository: PerryTS/perry
Length of output: 4633
🏁 Script executed:
# Search for plugin structure and compilation patterns
echo "=== Looking for plugin wrapper or setup ==="
rg -n "wrapper|plugin.*module|module.*plugin" crates/perry/src/commands/compile.rs | head -30
echo ""
echo "=== Checking if there are plugin-related tests ==="
fd -t f "test" crates/perry-codegen/ | xargs grep -l "plugin\|dylib" | head -5
echo ""
echo "=== Look for how is_entry is determined for plugins ==="
rg -n "is_entry.*=" crates/perry/src/commands/compile.rs | grep -A2 -B2 "dylib\|plugin"Repository: PerryTS/perry
Length of output: 2213
🏁 Script executed:
# Check the context around line 5315 in compile.rs where plugin_deactivate is mentioned
sed -n '5305,5330p' crates/perry/src/commands/compile.rs
echo ""
echo "=== Check how is_entry is determined for dylib ==="
rg -n "is_entry\s*=" crates/perry/src/commands/compile.rs | head -20
echo ""
echo "=== Look for any plugin documentation or comments ==="
rg -n -B3 "plugin" crates/perry/src/commands/compile.rs | grep -A3 "dylib" | head -40Repository: PerryTS/perry
Length of output: 2746
🏁 Script executed:
# Check if the entry branch has ANY ABI shim emission we might have missed
sed -n '152,665p' crates/perry-codegen/src/codegen/entry.rs | grep -n "plugin_activate\|perry_plugin_abi_version"
echo ""
echo "=== Verify: is there a wrapper pattern for dylib plugins? ==="
rg -n "dylib.*plugin|plugin.*dylib" crates/perry/src/commands/compile.rs -B2 -A2 | head -50Repository: PerryTS/perry
Length of output: 356
🏁 Script executed:
# Check if there are other ABI shim emissions in the codebase
rg "plugin_activate|perry_plugin_abi_version" crates/perry-codegen/ --type rs
echo ""
echo "=== Verify the exact boundary of is_entry and else branches ==="
sed -n '150,160p' crates/perry-codegen/src/codegen/entry.rs
sed -n '660,670p' crates/perry-codegen/src/codegen/entry.rs
sed -n '1055,1065p' crates/perry-codegen/src/codegen/entry.rsRepository: PerryTS/perry
Length of output: 1484
🏁 Script executed:
# Search for all plugin ABI shim emissions across the entire codebase
echo "=== All occurrences of plugin_activate or perry_plugin_abi_version ==="
rg "plugin_activate|perry_plugin_abi_version" crates/perry-codegen/src/
echo ""
echo "=== Confirm the end of else branch (should be around line 1056) ==="
sed -n '1050,1065p' crates/perry-codegen/src/codegen/entry.rsRepository: PerryTS/perry
Length of output: 1666
Plugin ABI shim code is in the non-entry branch—single-file dylib plugins will not export plugin_activate.
The plugin ABI shim (perry_plugin_abi_version, plugin_activate, plugin_deactivate) is defined only in the non-entry branch of compile_module_entry (lines 985-1056). However, the comment at lines 998-999 and compile.rs:5314 both state the ABI shim should be emitted "for the dylib's entry module."
For a single-file plugin compiled with perry compile plugin.ts --output-type dylib:
is_entry=truefor plugin.ts- The entry branch (lines 152-665) executes
- The plugin ABI shim code (lines 985-1056) in the non-entry branch is skipped
- Result:
plugin_activateis never exported, and the runtime will reject the plugin
All plugin ABI shim emissions occur only at lines 985-1056; no equivalent code exists in the entry branch. The shim logic must also run when is_entry && is_dylib.
🤖 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 `@crates/perry-codegen/src/codegen/entry.rs` around lines 985 - 1056, The
plugin ABI shim code block (the if is_dylib section containing
perry_plugin_abi_version, plugin_activate, and plugin_deactivate function
definitions) is currently only present in the non-entry branch, which causes
single-file dylib plugins to skip this entire section since they have
is_entry=true. Move the entire if is_dylib block so that the ABI shim emissions
execute whenever is_dylib is true, regardless of the is_entry value, ensuring
both single-file and multi-file dylib plugins properly export the required
plugin entry points to the runtime.
Comment on lines
+5299
to
+5302
| let has_plugin_deactivate = ctx | ||
| .native_modules | ||
| .values() | ||
| .any(|m| m.exported_functions.iter().any(|(n, _)| n == "deactivate")); |
There was a problem hiding this comment.
Gate plugin_deactivate on the entry module only.
This scans every native module, but the ABI shim is emitted from the entry module contract. A non-entry export named deactivate can add plugin_deactivate to the .def file without a matching ABI wrapper.
🐛 Proposed fix
- let has_plugin_deactivate = ctx
- .native_modules
- .values()
- .any(|m| m.exported_functions.iter().any(|(n, _)| n == "deactivate"));
+ let has_plugin_deactivate = ctx
+ .native_modules
+ .get(&entry_path)
+ .is_some_and(|m| {
+ m.exported_functions
+ .iter()
+ .any(|(name, _)| name == "deactivate")
+ });Also applies to: 5348-5350
🤖 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 `@crates/perry/src/commands/compile.rs` around lines 5299 - 5302, The current
code checks if any native module has a "deactivate" exported function by
scanning all modules in ctx.native_modules.values(), but the plugin_deactivate
ABI shim is only emitted from the entry module contract. To fix this, restrict
the check to only the entry module instead of iterating through all native
modules. Replace the .values().any() call with a check that only examines the
entry module's exported functions for "deactivate". Apply the same fix to the
other occurrence mentioned at lines 5348-5350, ensuring both instances only gate
on the entry module.
Comment on lines
+5342
to
+5352
| if let Ok(mut def_file) = std::fs::File::create(&def_path) { | ||
| use std::io::Write; | ||
| let _ = writeln!(def_file, "LIBRARY {}", stem); | ||
| let _ = writeln!(def_file, "EXPORTS"); | ||
| let _ = writeln!(def_file, " plugin_activate"); | ||
| let _ = writeln!(def_file, " perry_plugin_abi_version"); | ||
| if has_plugin_deactivate { | ||
| let _ = writeln!(def_file, " plugin_deactivate"); | ||
| } | ||
| } | ||
| c.arg(format!("/DEF:{}", def_path.display())); |
There was a problem hiding this comment.
Propagate .def file creation and write failures.
The linker always receives /DEF:<path> even if the file was never created or was only partially written, turning the real failure into a generic link error.
🛠️ Proposed fix
- if let Ok(mut def_file) = std::fs::File::create(&def_path) {
+ {
use std::io::Write;
- let _ = writeln!(def_file, "LIBRARY {}", stem);
- let _ = writeln!(def_file, "EXPORTS");
- let _ = writeln!(def_file, " plugin_activate");
- let _ = writeln!(def_file, " perry_plugin_abi_version");
+ let mut def_file = std::fs::File::create(&def_path).map_err(|e| {
+ anyhow!(
+ "failed to create Windows plugin .def file {}: {}",
+ def_path.display(),
+ e
+ )
+ })?;
+ writeln!(def_file, "LIBRARY {}", stem)?;
+ writeln!(def_file, "EXPORTS")?;
+ writeln!(def_file, " plugin_activate")?;
+ writeln!(def_file, " perry_plugin_abi_version")?;
if has_plugin_deactivate {
- let _ = writeln!(def_file, " plugin_deactivate");
+ writeln!(def_file, " plugin_deactivate")?;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Ok(mut def_file) = std::fs::File::create(&def_path) { | |
| use std::io::Write; | |
| let _ = writeln!(def_file, "LIBRARY {}", stem); | |
| let _ = writeln!(def_file, "EXPORTS"); | |
| let _ = writeln!(def_file, " plugin_activate"); | |
| let _ = writeln!(def_file, " perry_plugin_abi_version"); | |
| if has_plugin_deactivate { | |
| let _ = writeln!(def_file, " plugin_deactivate"); | |
| } | |
| } | |
| c.arg(format!("/DEF:{}", def_path.display())); | |
| { | |
| use std::io::Write; | |
| let mut def_file = std::fs::File::create(&def_path).map_err(|e| { | |
| anyhow!( | |
| "failed to create Windows plugin .def file {}: {}", | |
| def_path.display(), | |
| e | |
| ) | |
| })?; | |
| writeln!(def_file, "LIBRARY {}", stem)?; | |
| writeln!(def_file, "EXPORTS")?; | |
| writeln!(def_file, " plugin_activate")?; | |
| writeln!(def_file, " perry_plugin_abi_version")?; | |
| if has_plugin_deactivate { | |
| writeln!(def_file, " plugin_deactivate")?; | |
| } | |
| } | |
| c.arg(format!("/DEF:{}", def_path.display())); |
🤖 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 `@crates/perry/src/commands/compile.rs` around lines 5342 - 5352, The code is
ignoring write failures to the `.def` file by using `let _ = writeln!(...)` and
still passing the `/DEF:<path>` argument to the linker even if the file creation
or writes fail, which masks the real error. Capture the results of the writeln
calls instead of discarding them with `let _ = `, check if any write operation
fails, and only add the `/DEF:<path>` argument to the linker (c.arg call) if the
file is successfully created and all write operations complete successfully. If
any write fails, propagate or return the error so the real failure is visible
instead of a generic linker error.
Comment on lines
+427
to
+442
| let def_path = std::env::temp_dir().join(format!( | ||
| "perry_plugin_host_{}.def", | ||
| std::process::id() | ||
| )); | ||
| if let Ok(mut def_file) = fs::File::create(&def_path) { | ||
| let _ = writeln!( | ||
| def_file, | ||
| "NAME {}", | ||
| exe_path.file_stem().and_then(|s| s.to_str()).unwrap_or("perry_host") | ||
| ); | ||
| let _ = writeln!(def_file, "EXPORTS"); | ||
| for sym in PLUGIN_HOST_SYMBOLS { | ||
| let _ = writeln!(def_file, " {}", sym); | ||
| } | ||
| } | ||
| cmd.arg(format!("/DEF:{}", def_path.display())); |
There was a problem hiding this comment.
Silent failure when .def file creation fails; temp file not cleaned up.
-
If
fs::File::createfails (e.g., temp directory not writable), the code silently proceeds and passes a non-existent path to/DEF:, causing a confusing linker error instead of a clear diagnostic. -
The
.deffile is created but never removed after linking completes. Other temp files (response_file_to_clean,embedded_info_plist_path) are tracked and cleaned up at lines 1780-1785, butdef_pathis not.
🛠️ Suggested fix
+ let mut def_file_to_clean: Option<PathBuf> = None;
let def_path = std::env::temp_dir().join(format!(
"perry_plugin_host_{}.def",
std::process::id()
));
- if let Ok(mut def_file) = fs::File::create(&def_path) {
+ let mut def_file = fs::File::create(&def_path).map_err(|e| {
+ anyhow!("Failed to create plugin host .def file at {}: {}", def_path.display(), e)
+ })?;
- let _ = writeln!(
- def_file,
- "NAME {}",
- exe_path.file_stem().and_then(|s| s.to_str()).unwrap_or("perry_host")
- );
- let _ = writeln!(def_file, "EXPORTS");
- for sym in PLUGIN_HOST_SYMBOLS {
- let _ = writeln!(def_file, " {}", sym);
- }
- }
+ writeln!(
+ def_file,
+ "NAME {}",
+ exe_path.file_stem().and_then(|s| s.to_str()).unwrap_or("perry_host")
+ )?;
+ writeln!(def_file, "EXPORTS")?;
+ for sym in PLUGIN_HOST_SYMBOLS {
+ writeln!(def_file, " {}", sym)?;
+ }
+ def_file_to_clean = Some(def_path.clone());
cmd.arg(format!("/DEF:{}", def_path.display()));Then add cleanup near line 1780:
+ if let Some(path) = def_file_to_clean {
+ let _ = fs::remove_file(path);
+ }
if let Some(path) = response_file_to_clean {
let _ = fs::remove_file(path);
}🤖 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 `@crates/perry/src/commands/compile/link/build_and_run.rs` around lines 427 -
442, The code silently ignores failures when creating the `.def` file and
unconditionally passes the path to `/DEF:`, which causes unclear linker errors
if the temp directory is not writable. Additionally, the `def_path` is never
cleaned up after linking completes, unlike other temp files such as
`response_file_to_clean` and `embedded_info_plist_path` which are handled around
line 1780. First, replace the `if let Ok(mut def_file) =
fs::File::create(&def_path)` block with proper error handling that either
ensures the file is created successfully or logs a clear error and handles the
failure gracefully. Second, add `def_path` to the cleanup logic near line
1780-1785 where other temporary files are tracked and removed after the linking
process completes, ensuring the temp `.def` file is properly deleted when no
longer needed.
Comment on lines
+443
to
465
| } else { | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| // Force-keep all functions from plugin-related native libraries | ||
| for native_lib in &ctx.native_libraries { | ||
| if native_lib.module.contains("plugin") { | ||
| for func in &native_lib.functions { | ||
| cmd.arg(format!("-Wl,-u,_{}", func.name)); | ||
| } | ||
| } | ||
| } | ||
| // Force-keep Perry runtime symbols that plugin dylibs reference. | ||
| // These are collected from the Perry runtime's public API. | ||
| // Using -u tells the linker "treat as referenced" so dead_strip keeps them. | ||
| for sym in PLUGIN_HOST_SYMBOLS { | ||
| cmd.arg(format!("-Wl,-u,_{}", sym)); | ||
| } | ||
| } | ||
| // Force-keep Perry runtime symbols that plugin dylibs reference. | ||
| // These are collected from the Perry runtime's public API. | ||
| // Using -u tells the linker "treat as referenced" so dead_strip keeps them. | ||
| let runtime_syms = [ | ||
| "js_array_alloc", | ||
| "js_array_from_f64", | ||
| "js_array_push_f64", | ||
| "js_bigint_is_zero", | ||
| "js_closure_alloc", | ||
| "js_console_log_spread", | ||
| "js_dynamic_object_get_property", | ||
| "js_dynamic_string_equals", | ||
| "js_gc_register_global_root", | ||
| "js_is_truthy", | ||
| "js_jsvalue_compare", | ||
| "js_jsvalue_equals", | ||
| "js_nanbox_get_pointer", | ||
| "js_nanbox_pointer", | ||
| "js_nanbox_string", | ||
| "js_native_call_method", | ||
| "js_object_alloc_class_with_keys", | ||
| "js_object_alloc_with_shape", | ||
| "js_register_class_method", | ||
| "js_string_char_code_at", | ||
| "js_string_from_bytes", | ||
| "js_string_length", | ||
| "perry_debug_trace_init", | ||
| "perry_debug_trace_init_done", | ||
| "perry_init_guard_check_and_set", | ||
| ]; | ||
| for sym in &runtime_syms { | ||
| cmd.arg(format!("-Wl,-u,_{}", sym)); | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| cmd.arg("-rdynamic"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cross-compilation will use incorrect symbol export strategy.
The #[cfg(target_os = "...")] attributes are compile-time checks based on the host OS, but this code path should vary based on the target OS (which is_linux already computes at runtime). When cross-compiling from macOS to Linux, only the macOS block compiles in, causing macOS-style -Wl,-u,_<sym> flags to be passed instead of -rdynamic.
Use runtime checks like is_linux and is_macos (or derive one from existing flags) to ensure the correct linker flags are emitted for the target platform.
🔧 Suggested fix
} else {
- #[cfg(target_os = "macos")]
- {
+ // Check target platform at runtime, not host platform at compile time.
+ let is_macos_target = matches!(target, Some("macos"))
+ || (target.is_none() && cfg!(target_os = "macos"));
+ if is_macos_target {
// Force-keep all functions from plugin-related native libraries
for native_lib in &ctx.native_libraries {
if native_lib.module.contains("plugin") {
for func in &native_lib.functions {
cmd.arg(format!("-Wl,-u,_{}", func.name));
}
}
}
// Force-keep Perry runtime symbols that plugin dylibs reference.
- // These are collected from the Perry runtime's public API.
- // Using -u tells the linker "treat as referenced" so dead_strip keeps them.
for sym in PLUGIN_HOST_SYMBOLS {
cmd.arg(format!("-Wl,-u,_{}", sym));
}
- }
- #[cfg(target_os = "linux")]
- {
+ } else if is_linux {
cmd.arg("-rdynamic");
}
}🤖 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 `@crates/perry/src/commands/compile/link/build_and_run.rs` around lines 443 -
465, Replace the compile-time #[cfg(target_os = "...")] attribute-based
conditional blocks (the #[cfg(target_os = "macos")] and #[cfg(target_os =
"linux")] blocks) with a runtime if/else check that determines the target OS at
runtime. Use the is_linux variable that is already available in the context to
make this decision, or derive an is_macos check from the same source. This
ensures that when cross-compiling from one OS to another (for example, from
macOS to Linux), the correct linker flags are applied based on the target
platform rather than the host platform where compilation is happening.
…lict in compile/link/mod.rs Brings main's PerryTS#5400 (Windows response-file link) + PerryTS#5406/PerryTS#5408/PerryTS#5410/PerryTS#5412 codegen split + PerryTS#5402 require Tier 2 + PerryTS#5414 unknown builtin-namespace fix into the Windows plugin support branch. The conflict in compile/link/mod.rs is structural: main extracted the orchestrator into link/build_and_run.rs while the branch kept it inline. Resolved by taking main's refactor and re-applying the Windows plugin support on top: - compile/link/mod.rs: add PLUGIN_HOST_SYMBOLS const (the runtime + plugin export list — single source of truth for the macOS -u force-keep and the Windows .def file) plus use std::io::Write for the .def writer. - compile/link/build_and_run.rs: convert the if ctx.needs_plugins && !is_windows block to if ctx.needs_plugins with platform branches. The Windows branch writes a per-build perry_plugin_host_<pid>.def using the NAME directive (not LIBRARY — that one tells link.exe the output is a DLL and breaks the host .exe) and passes /DEF:<path> to the linker. PLUGIN_HOST_SYMBOLS replaces the inline untime_syms array on macOS too. - compile.rs: dylib link path on Windows now uses lld-link with /FORCE:UNRESOLVED (the .def file lists plugin_activate / perry_plugin_abi_version / plugin_deactivate so lld-link's empty default export table doesn't break GetProcAddress in the host) and writes a per-build .def file with the same three symbols. Lifts the pre-existing 'use link.exe' out for the LLVM-friendly linker. - codegen/entry.rs: re-apply the dylib plugin ABI shim block (emits perry_plugin_abi_version, plugin_activate, and the optional plugin_deactivate) on top of main's refactor of compile_module_entry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
Closes the Windows plugin support gap. Until this PR,
perry/pluginworked on macOS / Linux only: the runtime loader usedlibc::dlopen/dlsym/dlclose(Unix-only,#[cfg(unix)]-gated) and the--output-type dyliblink path had no Windows arm. On Windows,loadPlugin(path)returned 0 from a no-op stub inperry-ui-windows, andperry foo.ts --target windows --output-type dylibhad no link line.This PR adds the missing pieces so the same TS surface (
perry/plugin) and the sameperry_module_init/perry_plugin_*ABI work end-to-end on Windows. The host.exeexports the runtime + plugin-manager symbols; the plugin.dllresolves them atLoadLibrarytime, identical in shape to how the macOS-flat_namespace -undefined dynamic_lookupdylib resolves against its host.Changes
Runtime (
crates/perry-runtime/src/plugin.rs):open_library/lookup_symbol/close_library/last_load_errorinto cfg-gated helpers.#[cfg(unix)]gates onperry_plugin_load/perry_plugin_unload. Unix keepslibc::dlopen/dlsym/dlclose; Windows usesLoadLibraryW/GetProcAddress/FreeLibrary(viawindows-sys 0.61).LibHandlenewtype stays*mut c_voidso the rest of the registry / dispatch surface is unchanged. ABI check, activate, deactivate, hook / tool / service / route / event registration all work the same.crates/perry-runtime/Cargo.toml: enableWin32_System_LibraryLoader(andFreeLibraryis inWin32::Foundation, notLibraryLoader— the import path lives in code).Compile (
crates/perry/src/commands/compile.rs):--output-type dyliblink path. Linux wascc -shared; macOS iscc -dynamiclib -flat_namespace -undefined dynamic_lookup; Windows is nowlink /NOLOGO /DLL /OUT:<path>. The DLL leavesperry_*/js_*external references unresolved — they bind to the host process atLoadLibrarytime.Link (
crates/perry/src/commands/compile/link/mod.rs):ctx.needs_plugins && is_windows, write a per-buildperry_plugin_host_<pid>.deflisting the runtime + plugin-manager exports and pass/DEF:<path>tolink.exe. This is the MSVC equivalent of-rdynamic/-Wl,-u,_<sym>. The newPLUGIN_HOST_SYMBOLSconst is the single source of truth for both the macOS-uforce-keep list and the Windows.deffile (Linux still uses-rdynamic)./OPT:REF->/OPT:NOREFfor plugin hosts so dead strip does not discard exports the.deffile is preserving.UI (
crates/perry-ui-windows/src/ffi/widget_layout_extras.rs):perry_plugin_load(returned 0) andperry_plugin_unloadstubs. The real implementations inperry-runtimenow link through for both console and UI plugin hosts.Test (
tests/test_plugin_lifecycle.sh):plugin.ts(registerHook+registerTool+setMetadata+deactivate) and ahost.ts(loadPlugin/emitHook/invokeTool/unloadPlugin), runs the host against the plugin, and asserts the hook transformed the input and the tool returned the expected greeting. Detects host platform and uses the right extensions (.dll/.exeon Windows,.soon Linux,.dylibon macOS); skips on unsupported hosts.Notes for maintainers
CHANGELOG.mdentry, noCLAUDE.mdchange in this PR per the contributor-PR process documented inCLAUDE.md. The maintainer folds those in at merge time.PERRY_PLUGIN_TABLE/PERRY_PLUGIN_INSTANCE_TABLEalready route everyperry/pluginTS surface toperry_plugin_*runtime symbols — that wiring was in place and is now exercised on Windows too.docs/src/plugins/*pages are platform-agnostic and the // platforms: macos, linux, windows annotations on the snippet files become reality rather than aspirational.Verified
cargo checkonperry-runtime,perry-ui-windows,perry— clean.cargo build --release -p perry-runtime -p perry-stdlib -p perry— succeeds.cargo test --release -p perry --bins— 583 pass; 4 pre-existing failures (printf is not recognizedon Windows + POSIX-path test) unrelated to this PR.cargo test --release -p perry-runtime— 132+ pass; 1 pre-existingSTATUS_STACK_BUFFER_OVERRUNinnative_abi::testsunrelated.Summary by CodeRabbit