Skip to content

Fix linux crash issue - #13

Merged
bettercallsaulj merged 15 commits into
mainfrom
fix_linux_crash
Jul 31, 2026
Merged

Fix linux crash issue#13
bettercallsaulj merged 15 commits into
mainfrom
fix_linux_crash

Conversation

@bettercallsaulj

@bettercallsaulj bettercallsaulj commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

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:

  • Rework build.sh for Linux native builds and add an Ubuntu 20.04 cross-build
    path for the Linux x64 wheel.
  • Pin the Linux builder image to ubuntu:20.04@sha256:8feb4d8c..., while
    keeping it overridable through an ARG.
  • Stop bundling libssl.so* and libcrypto.so* into published Linux wheels.
  • Make patchelf failures hard failures so broken runtime paths are caught
    during packaging.
  • Resolve versioned native library names correctly on Linux and macOS instead
    of relying on lexicographic ordering.
  • Add CI/package checks for Linux native dependencies to prevent regressions.

Other Fixes In This Stack

  • Improve native loader diagnostics and point users at DEBUG=1, which is the
    environment variable the code actually reads.
  • Raise and document clearer agent errors when native run() returns null.
  • Expand public auth exports and remove the confusing top-level auth error name
    collision.
  • Share API example venv setup across all seven wrappers so the Linux/Debian
    dependency setup applies consistently.

Validation

  • 151 passed, 116 skipped, 3 failed
  • The three failures reproduce identically on origin/main and require
    ./build.sh, so this branch did not introduce them.

@dIvYaNshhh dIvYaNshhh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 pip fix only landed in 2 of the 7 example wrappers — five still break on the platform this PR targets. And the VENV_PYTHON fallback can pip-install into the user's system Python.
  • We're bundling EOL OpenSSL 1.1.1 into shipped Linux artifacts.
  • patchelf failures 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.

Comment thread gopher_mcp_python/agent.py Outdated
Comment thread gopher_mcp_python/agent.py
Comment thread examples/api/create_by_api_key_run.sh Outdated
Comment thread scripts/docker/build-linux-x64-ubuntu20.sh
Comment thread scripts/docker/build-linux-x64-ubuntu20.sh Outdated
Comment thread gopher_mcp_python/ffi/library.py Outdated
Comment thread gopher_mcp_python/__init__.py Outdated
Comment thread scripts/docker/Dockerfile.linux-x64-ubuntu20 Outdated

@dIvYaNshhh dIvYaNshhh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at dc0426e6. All eight comments addressed — resolving every thread, nothing outstanding from me.

  • OpenSSLlibssl.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.
  • patchelf2>/dev/null || true is 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 an ARG so it stays overridable. Better than what I suggested.
  • GOPHER_DEBUGDEBUG=1 — the hint now names a variable the code actually reads.
  • Versioned library resolution — the lexicographic sorted()[0] problem and the macOS libfoo.0.1.30.dylib naming 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.

RahulHere added 15 commits July 31, 2026 17:27
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
@bettercallsaulj
bettercallsaulj changed the base branch from feature_token_protect to main July 31, 2026 09:28
@bettercallsaulj
bettercallsaulj merged commit b035fed into main Jul 31, 2026
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.

2 participants