Skip to content

fix(node): gate pyo3 behind a python feature so the addon has no libpython - #31

Merged
wintonzheng merged 1 commit into
mainfrom
shu/sky-12273-pyo3-feature-gate-v2
Jul 14, 2026
Merged

fix(node): gate pyo3 behind a python feature so the addon has no libpython#31
wintonzheng merged 1 commit into
mainfrom
shu/sky-12273-pyo3-feature-gate-v2

Conversation

@wintonzheng

Copy link
Copy Markdown
Contributor

What

Gate pyo3 behind a python cargo feature (on by default) so the napi node addon builds without any Python linkage. Re-applies #27, which was closed when main history was rewritten.

  • Cargo.toml: pyo3 optional; [features] default = ["python"], python = ["dep:pyo3"]
  • node/Cargo.toml: consumes the core with default-features = false
  • src/lib.rs: #[cfg(feature = "python")] on the pyo3 bindings; run_detached gets a pure fallback (its non-attached branch); create_page_raw/create_page_async/attach_existing_page return Arc<PageInner> so the FFI section never references PyPage

Why

Every napi prebuild links libpython today: the Linux x64 addon carries DT_NEEDED libpython3.12.so.1.0 and the macOS addon links the runner's Python.framework path, so published prebuilds would only load on machines with the runner's exact Python. The aarch64 zig cross-link fails outright (unable to find dynamic system library python3.8), which is what blocks the npm release. CI smoke passes only because runners have Python installed.

Testing

  • cargo check --locked with default features and --no-default-features (0 errors; 0 warnings on default)
  • cargo test --locked 23/23
  • npm run build + npm run smoke: addon drives a browser; otool -L shows only libiconv/libSystem, zero libpython/Python.framework strings
  • maturin build --release: cp38-abi3 wheel builds; fresh-venv install + sync API launches chromium and reads a title

Checklist

  • I kept this change focused.
  • I added or updated tests for behavior changes. (behavior-preserving refactor; 23 existing tests + both build-path smokes cover it)
  • I did not include private credentials, tokens, or internal-only artifacts.

…ython

Re-applies the change from #27, which was closed when main history was
rewritten. The napi addon linked libpython on every platform because
rustwright-core depends on pyo3 unconditionally: prebuilds only load on
machines with the runner's exact Python, and the aarch64 zig cross-link
fails outright (no libpython in the zig sysroot), blocking the npm release.

- Cargo.toml: pyo3 optional; [features] default = ["python"], python = ["dep:pyo3"]
- node/Cargo.toml: depend on the core with default-features = false
- src/lib.rs: cfg-gate the pyo3 bindings; run_detached gets a pure
  fallback; create_page_raw/create_page_async/attach_existing_page return
  Arc<PageInner> so the FFI path never touches PyPage

Verified: cargo check both feature configs (0 errors, 0 default warnings),
cargo test 23/23, napi addon links only libiconv/libSystem (zero
libpython/Python.framework strings) and smoke-drives a browser; maturin
cp38-abi3 wheel builds and the python sync API launches chromium.

Refs: SKY-12273

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K7b2PMrqsT9BJGaF6DFJA2
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Summary

Clean, well-scoped fix for a real blocking issue (aarch64 zig cross-link failure, libpython DT_NEEDED in prebuilds). The Arc<PageInner> refactor to decouple shared async helpers from PyPage is the right approach. No critical issues.

🔴 Critical Issues (0)

None.

🟡 Suggestions (2)
  • --no-default-features has no CI gate. test.yml only runs cargo check --locked (default features). The node build path will only be exercised by release-npm.yml on tag push. A simple step added to test.yml would catch any future #[cfg(feature = "python")] annotation that gets missed:

    - name: Cargo check (no-default-features / node path)
      run: cargo check --locked --no-default-features

    Without this, regressions to the non-Python build silently survive every PR until a release attempt.

  • #[cfg(not(feature = "python"))] run_detached may be dead code. The non-Python fallback just calls operation() inline. The RustwrightBrowser/RustwrightPage public Rust API goes through block_on_raw directly, so run_detached may have no callers in the --no-default-features build. If so this produces a dead_code warning — which is consistent with the checklist noting "0 warnings on default" but being silent about --no-default-features. If that warning fires, either add #[allow(dead_code)] or remove the fallback entirely and let run_detached stay Python-only (since no non-Python caller exists). The absence of a dead-code warning also implies there is a non-Python caller, in which case calling operation() inline (vs. spawning a thread) is a behavioral change worth a comment.

📝 Minor / Style (2)
  • run_detached fallback deserves a one-line comment clarifying the intent, e.g. // No GIL to release in the non-Python build; call directly. Otherwise the divergence from the Python version (which spawns a thread specifically to avoid GIL deadlocks) looks like a bug to future readers.

  • Checklist wording is ambiguous. (0 errors; 0 warnings on default) can be read as applying only to the default-features invocation. Expanding it to (0 errors, 0 warnings on both feature sets) — or explicitly noting any expected warnings on --no-default-features — would make the PR self-documenting.

@wintonzheng
wintonzheng merged commit 7f0f694 into main Jul 14, 2026
5 of 6 checks passed
@wintonzheng
wintonzheng deleted the shu/sky-12273-pyo3-feature-gate-v2 branch July 14, 2026 00:50
@github-actions

Copy link
Copy Markdown

Synced to rustwright-cloud: https://github.com/Skyvern-AI/rustwright-cloud/pull/20

wintonzheng added a commit that referenced this pull request Jul 14, 2026
## What

Bump the version to 0.1.0-alpha.3 across all six version files
(lockfiles regenerated via the toolchain).

## Why

Release prerequisite for the first npm publish now that #31 removed the
libpython linkage. PyPI already has 0.1.0a2 and the old tags were
deleted in the history rewrite, so the release fixes forward to alpha.3;
the tag will publish npm (first release, `next` dist-tag) and PyPI
(alpha.3 with the pyo3 feature gating) together. Nothing publishes on
merge; publishing is tag-gated and environment-approved.

## Testing

- [x] cargo check --locked (all 8 version fields agree at 0.1.0-alpha.3)
- [x] Full local battery ran on the parent commit (#31): cargo test
23/23, node addon builds with zero python linkage + smoke, maturin wheel
+ python API smoke

## Checklist

- [x] I kept this change focused.
- [x] I added or updated tests for behavior changes. (n/a: version-only)
- [x] I did not include private credentials, tokens, or internal-only
artifacts.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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