fix(compile): reject Node native addons in compilePackages#4986
Conversation
39228c9 to
48ae00e
Compare
48ae00e to
4233b26
Compare
proggeramlug
left a comment
There was a problem hiding this comment.
Audited this against #4961 — the approach is sound: the guard sits at the right hook point (module collection, before the CJS wrap and SWC parse, so both ESM and CJS-shaped packages fail before codegen/link), the perry.nativeLibrary exemption reuses the existing helper consistently, and the test matrix covers the issue's required cases. Four findings inline: two in the detection logic (gypfile substring match, devDependencies signal), one perf (per-module rescan without memoization), one robustness note on package-root resolution. Nits, take or leave: find_node_addon_file flags a directory named *.node (extension check precedes the is_dir branch — an is_file() guard would fix it), and marker_path_for_display is a one-line wrapper called three times that could be inlined.
|
CI update: the remaining failure on this PR is I reproduced the same test failure locally after merging current I also checked recent PR CI. Several current PRs have |
|
Follow-up GC bisection detail: I can make the failing stress test pass/fail with the conservative stack scan knob. Local setup was the current PR branch after merging current Commands/results: $env:PERRY_RUNTIME_DIR = D:\tmp\perry-native-addon-pr\target\perry-auto-8981e03852ab7578\release
& D:\tmp\perry-gc-diagnostic-target\debug\deps\gc_write_barrier_stress-90aa6112441215e7.exe tenured_mutation_stress --nocaptureDefault/manual With conservative stack scanning disabled, the same test passes: $env:PERRY_CONSERVATIVE_STACK_SCAN = 0
& D:\tmp\perry-gc-diagnostic-target\debug\deps\gc_write_barrier_stress-90aa6112441215e7.exe tenured_mutation_stress --nocapture
# okWith conservative stack scanning explicitly enabled, it fails again with the same verifier class: $env:PERRY_CONSERVATIVE_STACK_SCAN = 1
& D:\tmp\perry-gc-diagnostic-target\debug\deps\gc_write_barrier_stress-90aa6112441215e7.exe tenured_mutation_stress --nocapture
# old-young-edge-verifier missing_edges=4882The sibling My read: this points at the recent manual- |
…ive-addons # Conflicts: # crates/perry/src/commands/compile/collect_modules/tests.rs
|
Latest CI after rebasing onto current
Those are the same failures in the upstream Tests run for |
Summary
Reject packages that ship Node native addons when they are opted into
perry.compilePackages, matching the existing porting-doc guidance that these packages are not portable through the JS/TS compile path.Fixes #4961.
Changes
binding.gyp,prebuilds/, packagegypfile, bounded-depth*.nodefiles, and common native-addon loader dependencies (node-gyp-build/bindings).perry.nativeLibrary/ perry-ffi instead of late runtime/link failures.perry.nativeLibrary.compilePackagesis for JS/TS and packages likenode-pty,sharp,better-sqlite3, andsqlite3need a native-library replacement path.Related issue
Fixes #4961
Test plan
cargo fmt --allcargo test -p perry compile_package_ -- --nocapturecargo test -p perry perry_native_library_package_is_not_rejected_by_node_addon_guard -- --nocapturegit diff --checkThe focused test runs passed on Windows. Existing workspace warnings were emitted but not introduced by this PR.
Screenshots / output
Representative new diagnostic shape:
Checklist
feat:/fix:/docs:/chore:prefix convention used in the log