Fix linux crash issue - #13
Conversation
7045a5f to
2e1e79a
Compare
5e658fb to
b200912
Compare
dIvYaNshhh
left a comment
There was a problem hiding this comment.
Some really useful stuff in here — _load_errors plus get_load_error_message() turns an opaque "failed to load native library" into a per-path list of what was tried and why each failed, which is the diagnostic that's been missing. get_last_error_message() picking up the details field is a straight win too; the C side was populating something we were throwing away. And the "preserve local debug checkout" logic is thoughtful — checking both status --short and recorded-vs-current SHA is the right pair of conditions.
Also glad to see this reverts #12's --remote submodule change back to recorded-SHA-first.
That said, I had a hard time reviewing it, because the title says "Fix linux crash issue" and the description is empty, but the diff is four separate workstreams: the Ubuntu 20.04 Docker cross-build (the actual fix, I think?), a build.sh rewrite with per-target output dirs, error-message plumbing across agent.py/library.py, and a ~40-symbol expansion of the public auth exports. Could those be split, or at minimum could the description explain what the crash was and which part addresses it? The auth re-export change in particular is a public API change that has nothing to do with Linux.
Blocking-ish items, all inline:
- The Debian
pipfix only landed in 2 of the 7 example wrappers — five still break on the platform this PR targets. And theVENV_PYTHONfallback can pip-install into the user's system Python. - We're bundling EOL OpenSSL 1.1.1 into shipped Linux artifacts.
patchelffailures are silently swallowed, in the script that exists to fix Linux loading.- The new error message tells users to set
GOPHER_DEBUG=1, which isn't a variable this codebase reads. run()now raises where it used to return a string — breaking, no CHANGELOG entry.
One carried-over concern: this keeps #12's cwd-first library search order and grows it to three cwd entries, one of which (native/current) is now a symlink that build.sh creates. So we're following a symlink from the current working directory ahead of the installed package. Same argument as on #12 — please gate that behind a dev opt-in.
Smaller things: native/ output is written three times per build (per-target dir, current symlink, and a flat copy into native/lib), and the linux-on-macOS skip-rebuild path trusts the stamp with no symbol check where the macOS path runs nm.
2e1e79a to
bb91598
Compare
b200912 to
a2a7472
Compare
bb91598 to
a46cfc5
Compare
a2a7472 to
dc0426e
Compare
dIvYaNshhh
left a comment
There was a problem hiding this comment.
Re-reviewed at dc0426e6. All eight comments addressed — resolving every thread, nothing outstanding from me.
- OpenSSL —
libssl.so*|libcrypto.so*added to the bundling skip list, so we're no longer vendoring an EOL TLS stack into published wheels. This was my main concern on this PR. patchelf—2>/dev/null || trueis gone; it hard-fails now, which is what you want in the script that exists to fix Linux loading.- Dockerfile — pinned to
ubuntu:20.04@sha256:8feb4d8c…, and via anARGso it stays overridable. Better than what I suggested. GOPHER_DEBUG→DEBUG=1— the hint now names a variable the code actually reads.- Versioned library resolution — the lexicographic
sorted()[0]problem and the macOSlibfoo.0.1.30.dylibnaming are both handled. - Auth error naming — no longer two same-named-but-unrelated exception types at the top level.
run()null-response — behaviour change documented.- Linux native dependency verification added, which closes the macOS/Linux asymmetry I flagged.
The one I want to call out specially is 6e97e5a3 / c4feb45b: the venv setup is now shared across all seven wrappers rather than the two this PR originally patched. That was the structural item I raised back on #10 and I'd half expected it to get deferred indefinitely — good to see it actually done, and it means this PR's Debian fix now applies everywhere instead of to 2/7 of the examples.
Suite at the tip of the stack: 151 passed, 116 skipped, 3 failed — all three failures reproduce identically on origin/main (they want ./build.sh), so nothing regressed.
Only leftover is the empty PR description. This one in particular would benefit: the title says "Fix linux crash issue" but the diff spans a Docker cross-build, a build.sh rewrite, error-message plumbing, and a public auth export expansion. A short note on what the crash was and which part fixes it would help whoever picks this up next.
dc0426e to
a2f61a7
Compare
de69016 to
d43201e
Compare
Summary:\n- include native error details in Python FFI error messages\n- use an actionable create-agent fallback when native returns null without last_error\n- add tests for native error detail propagation and fallback create errors
Summary:\n- make GopherAgent.run raise AgentError when native returns no response\n- preserve native last_error text for null run results\n- add focused tests for fallback and native run error messages
Summary:\n- allow GOPHER_MCP_PYTHON_LIBRARY_PATH and GOPHER_ORCH_LIBRARY_PATH to point at a library file or directory\n- collect native library load errors for clearer AgentError messages\n- add loader tests for file, directory, versioned library, and diagnostic handling
Summary:\n- search native/<platform>/lib and native/current/lib before compatibility native/lib\n- keep local build outputs ahead of installed platform packages\n- update main and auth loader tests for platform-specific native output paths
Summary:\n- re-export OAuth, session, auto-refresh, metadata, URL, and validation auth helpers from ffi.auth\n- expose common auth helpers at gopher_mcp_python.auth and the package root\n- add import-contract tests for root and auth package exports
Summary:\n- add target parsing and resolved native output directories for macOS and Linux builds\n- preserve local gopher-orch and gopher-mcp submodule checkouts instead of forcing remote updates\n- install native outputs under native/<target> while maintaining native/current and native/lib compatibility paths
Add a Docker-based linux-x64 build path on macOS, preserve local submodule checkouts during builds, and bundle Linux shared-library dependencies for portable artifacts. Update API example runners to install the published PyPI SDK/native packages through the venv Python so Debian environments do not accidentally invoke a system pip.
Document that Python 3.8+ is the base requirement, while venv and pip are needed for PyPI installation, examples, and development workflows. Add Debian/Ubuntu package guidance and use python3 -m pip in the source install command.
Summary:\n- replace stale GOPHER_DEBUG guidance with DEBUG=1\n- mention Python native library load diagnostics in the fallback message\n- update error-message coverage to lock the documented env var
Summary:\n- add an Unreleased changelog entry for the GopherAgent.run behavior change\n- note that null native responses now raise AgentError instead of returning a sentinel string
Summary:\n- skip libssl and libcrypto when copying Linux native package dependencies\n- keep TLS libraries system-provided so distro security updates apply\n- add regression coverage for the Linux dependency skip policy
Summary:\n- make Linux patchelf rpath updates fail the Docker build on error\n- add linux-x64 publish workflow checks for ORIGIN rpath and missing dependencies\n- fail publishing if OpenSSL libraries are bundled into the native package\n- document Linux OpenSSL as a system-provided runtime dependency\n- expand packaging policy tests for rpath and dependency verification
Summary: - prefer exact unversioned native library names before scanning versioned files - sort Linux versioned .so candidates by parsed numeric version - support macOS versioned dylib names with the version before .dylib - add regression tests for unversioned preference and versioned fallback selection
Summary: - export the high-level auth exception as top-level GopherAuthError - expose the FFI auth enum as GopherAuthFfiError instead of GopherAuthBaseError - lazily resolve auth exports to avoid eager auth FFI imports from package import - update auth export tests for the corrected public contract
Summary: - pin the Ubuntu 20.04 Linux x64 builder image by digest - pass the pinned base image into the Dockerfile from build.sh - use the same pinned image for Linux artifact verification - add packaging tests to prevent direct mutable ubuntu:20.04 usage
a2f61a7 to
6a69838
Compare
Summary
This PR fixes the Linux native-package crash by making the Python SDK package and
loader agree on how native libraries are built, named, bundled, and resolved on
Linux.
The crash path was Linux wheel loading: the package could miss the right
versioned native library or ship dependencies that were not portable across the
target runtime. The fix is split across the build and runtime sides:
build.shfor Linux native builds and add an Ubuntu 20.04 cross-buildpath for the Linux x64 wheel.
ubuntu:20.04@sha256:8feb4d8c..., whilekeeping it overridable through an ARG.
libssl.so*andlibcrypto.so*into published Linux wheels.patchelffailures hard failures so broken runtime paths are caughtduring packaging.
of relying on lexicographic ordering.
Other Fixes In This Stack
DEBUG=1, which is theenvironment variable the code actually reads.
run()returns null.collision.
dependency setup applies consistently.
Validation
151 passed, 116 skipped, 3 failedorigin/mainand require./build.sh, so this branch did not introduce them.