SBCL librarian runtime - #58
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moves off the 2023 sbcl-librarian pin, which also unblocks modern SBCL. libquil is now the generated C bindings only; the Lisp image lives in a core loaded by the libsbcl_librarian runtime, whose constructor initializes Lisp when it is loaded. The explicit init(core) call is gone, as is libquil's hand-rolled error handling. libquil keeps shipping a core rather than the FASL bundles that CREATE-FASL-LIBRARY-CMAKE-PROJECT produces. FASL bundles are re-loaded on every startup, re-running load-time code, and cl-quil resolves stdgates.quil through ASDF:SYSTEM-RELATIVE-PATHNAME at load time -- so a FASL build only runs where quilc's source tree sits at the path recorded when it was built. A core evaluates that once, at build time. build-image.lisp emits three things from one image: libquil's bindings, the runtime's bindings, and the core exporting both sets of callables. The Makefile builds the runtime too, so plain 'make' still yields a usable artifact. Errors move to the runtime's get_error_message/lisp_err_t. libquil redefines default-error-map so ordinary bad input reports as LISP_ERR_FAILURE rather than an internal bug, handling T rather than CL:ERROR because cl-quil signals conditions that are not subtypes of ERROR. REARCHITECTURE.md records the decisions. Needs three sbcl-librarian fixes, on its fix-secondary-system-bundles branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- build.yml builds SBCL $SBCL_VERSION (2.6.7) with --with-sb-linkable-runtime,
replacing the sbcl-2.2.4 build that failed in make-host-1 ('undefined variable:
SB-VM::END') because a current host SBCL cannot cross-compile a 2022 target.
sbcl-librarian's repo and ref are workflow inputs, since libquil needs three
fixes that are not upstream yet.
- The artifact is now the bindings library, its header and the whole runtime
directory: libsbcl_librarian, libquil.core, libsbcl.so and the runtime headers.
install.sh and the release archives follow, with libquil.core installed beside
libsbcl_librarian because the runtime finds its core relative to itself.
- The examples drop their init(core) calls and -pagezero_size, link the runtime,
and use get_error_message/lisp_err_t.
- The redundant quickload steps before make are gone; build-image.lisp does it.
Verified by simulating package -> zip -> install and running the libquil-sys
suite against the installed layout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
magicl looks for Homebrew's reference LAPACK first and otherwise falls back to a bare liblapack.dylib, which on macOS resolves to Accelerate's. Neither works: the reference build returns incorrect eigenvectors on arm64, failing with 'Could not find diagonalizer for matrix ... after 16 attempts', and Accelerate's LAPACK predates 3.3 so routines quilc calls are missing, failing with 'The alien function zuncsd_ is undefined'. OpenBLAS is correct and complete. Loading it first means its symbols are the ones that resolve, and because SBCL records loaded shared objects in the core and reloads them at startup, the choice is baked into the artifact instead of depending on what the loader happens to find. Verified against a stock magicl checkout with Homebrew's lapack installed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cutting a prerelease from a branch produced a tag pointing at the wrong code.
knope 0.10/0.11 create the GitHub release against the default branch, so the
version bump landed on the branch while the tag landed on main -- and the publish
job, which checks out whatever that tag resolves to, then tried to republish main's
already-released version:
error: crate libquil-sys@0.4.2 already exists on crates.io index
Adopt the setup rigetti-pyo3 arrived at (36a1850, 9b5235c, 3505971):
- knope 0.23, which tags the ref being released and takes --prerelease-label, so
the separate prerelease workflow in knope.toml is no longer needed;
- the ref decides what gets cut: main releases, anything else prereleases;
- check out ${{ github.ref }} so a release acts on the dispatched branch;
- pass the token to the Release step through GITHUB_TOKEN, which is how it
authenticates;
- dry-run the release on pull requests, so a broken config is visible before it is
dispatched.
The publish job now checks out the release tag explicitly, so it always publishes
exactly what was tagged rather than whatever the target commitish points at.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… root Two problems with installing a release: magicl loads BLAS and LAPACK at runtime under their unversioned names, libblas.so and liblapack.so, so a missing one does not surface at install or link time: it surfaces much later, in the middle of compiling a program. Check for them before anything is downloaded, and name the ones that are missing. The unversioned names are also why a runtime-only package is not enough -- Debian's libblas3 provides libblas.so.3 and no unversioned symlink -- so say that in the requirements, which is the part users get wrong. Drop libz from the requirements while there: nothing in the release artifacts refers to it. Container images commonly run as root with no sudo installed, where every sudo call here fails with "sudo: command not found" even though nothing needs elevating. That is how CI installs libquil, so the installer was unusable there. Resolve sudo once: empty when already root, sudo when available, and a clear error when neither. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # libquil depends on three sbcl-librarian fixes (secondary-system FASL bundles, | ||
| # library prefix, configurable core name). They live on a branch of Rigetti's | ||
| # fork; move this back to quil-lang/sbcl-librarian once they are upstream. | ||
| SBCL_LIBRARIAN_REPO: "https://github.com/rigetti/sbcl-librarian.git" | ||
| SBCL_LIBRARIAN_REF: "fix-secondary-system-bundles" |
There was a problem hiding this comment.
A specific version, rather than a git dependency, should be used once this MR is merged: quil-lang/sbcl-librarian#91
There was a problem hiding this comment.
Do we have an issue to track these? (Version tag and switching back to upstream)
There was a problem hiding this comment.
I was initially thinking we might be able to merge these in order so that the sbcl-librarian change would be released before we merge this, but since that's not getting any traction, I'll open an issue.
| # SBCL is built from source because a linkable runtime (libsbcl.so) is | ||
| # required and neither make.sh nor any package manager produces one. |
There was a problem hiding this comment.
"Any package manager" may be overbroad; I believe I only actually checked Homebrew.
| # libffi-dev pulls in whichever libffi runtime the image ships; naming | ||
| # it explicitly breaks whenever the runner image moves (libffi7 does not | ||
| # exist on ubuntu 24.04, which ubuntu-latest now resolves to). | ||
| run: sudo apt update && sudo apt install -y libblas-dev libffi-dev liblapack-dev libz-dev gfortran |
There was a problem hiding this comment.
comment can probably be deleted following PR review
There was a problem hiding this comment.
Are libffi 7 and 8 both supported? If not, I think I'd rather the breakage here over weird errors elsewhere.
There was a problem hiding this comment.
I honestly have no idea why Debian attaches "7" and "8" to the package names. libffi itself has been on version 3+ since 2008. I can't find any indication that there would be any breakage from this update.
| cd $GITHUB_WORKSPACE/libquil | ||
| ls | ||
| sbcl --noinform --non-interactive --eval '(ql:quickload :sbcl-librarian)' | ||
| sbcl --dynamic-space-size 8192 --noinform --non-interactive --eval '(ql:quickload :libquil)' |
There was a problem hiding this comment.
I am honestly not sure what these were doing.
There was a problem hiding this comment.
quickload comes from Quicklisp, which appears to be a package manager
There was a problem hiding this comment.
Yeah, but I don't know what the (ql:quickload :<symbol>) actually does; I assume it's just validating that the library can be imported.
There was a problem hiding this comment.
...and I don't know why we wouldn't want to validate that, so I'm going to try restoring them.
| # Deliberately not installing lapack. magicl prefers Homebrew's reference | ||
| # LAPACK over everything else, and on arm64 it computes incorrect | ||
| # eigenvectors -- compilation then fails with "Could not find diagonalizer | ||
| # for matrix ... after 16 attempts". With it absent, magicl falls through to | ||
| # a bare liblapack.dylib, which these symlinks point at OpenBLAS. |
There was a problem hiding this comment.
I don't know how to verify this claim, but it sounds like it should potentially be reported upstream as a bug.
| # Each artifact is downloaded into its own directory: the two macOS | ||
| # artifacts contain identically named files, so a shared directory would | ||
| # have one overwrite the other. |
There was a problem hiding this comment.
I think "two macOS artifacts" here may refer to amd-64, which we're not building; possibly this comment can just be deleted.
| # 0.23 is needed for `--prerelease-label` and because older versions tag | ||
| # the default branch rather than the ref being released, which produces a | ||
| # tag pointing at the wrong code when releasing from a branch. |
There was a problem hiding this comment.
| # 0.23 is needed for `--prerelease-label` and because older versions tag | |
| # the default branch rather than the ref being released, which produces a | |
| # tag pointing at the wrong code when releasing from a branch. |
| # the Lisp image (and supplies get_error_message). No -pagezero_size: the image is | ||
| # mapped by the runtime library, not by this executable. |
There was a problem hiding this comment.
Again, not sure there needs to be a comment here for what has been deleted, namely -pagezero_size
There was a problem hiding this comment.
The earliest PR for aarch64 support stated that we'd need to conditionally enable -pagezero_size, but I think (...and hope) Claude is correct that sbcl-librarian is now managing the page size.
| (setf *last-error* (format nil "~a" condition)) | ||
| (setf sbcl-librarian::*error-message* | ||
| (format nil "~a" condition)) |
There was a problem hiding this comment.
I don't really understand what the original code was doing, but this at least doesn't seem obviously wrong to me.
| ;; As in COMPILE-PROTOQUIL, the LAPACK library for macOS will sometimes hit a | ||
| ;; division-by-zero. Mask those interrupts so they can be handled in Lisp. | ||
| (magicl:with-blapack | ||
| (cl-quil:compiler-hook parsed-program chip-specification))) |
There was a problem hiding this comment.
Again, this seems like something that ought to be reported upstream, if correct.
| ;;; PROCESS-PROTOQUIL and STRIP-FINAL-HALT-RESPECTING-REWIRINGS live in quilc's | ||
| ;;; *application*, which we do not depend on, so they are vendored here rather | ||
| ;;; than requiring a quilc fork that exposes them from the library. | ||
| ;;; TODO(https://github.com/quil-lang/quilc/pull/933): remove these once the logic is upstreamed |
There was a problem hiding this comment.
I don't know SBCL and have not reviewed the changes in this file.
| ;; libquil depends on systems that come from Quicklisp (cffi, bordeaux-threads, | ||
| ;; ...). ASDF alone will not fetch those, so prefer Quicklisp when it is | ||
| ;; available and fall back to plain ASDF for setups that vendor the | ||
| ;; dependencies themselves. | ||
| (let ((quicklisp-init (merge-pathnames "quicklisp/setup.lisp" (user-homedir-pathname)))) | ||
| (when (and (null (find-package '#:quicklisp)) | ||
| (probe-file quicklisp-init)) | ||
| (load quicklisp-init))) | ||
|
|
There was a problem hiding this comment.
This looks like a fair amount of "magic" that I am not able to follow. I would appreciate feedback from someone who understands the asdf build-system and the sbcl runtime.
There was a problem hiding this comment.
As mentioned, it's a build-system: https://www.sbcl.org/asdf/
| ;;; Error reporting comes from SBCL-LIBRARIAN's built-in ERRORS api, which | ||
| ;;; provides the `lisp_err_t' type, `get_error_message', and `enable_backtrace'. | ||
| ;;; DEFINE-API always uses SBCL-LIBRARIAN's DEFAULT-ERROR-MAP, which records the | ||
| ;;; condition into that message. | ||
| ;;; | ||
| ;;; We redefine that map for libquil's own APIs. The stock one classifies any | ||
| ;;; plain CL:ERROR as an internal bug (LISP_ERR_BUG), attaching a backtrace and an | ||
| ;;; "Internal lisp bug:" prefix. Almost everything libquil signals is a user error | ||
| ;;; -- malformed Quil, an unknown memory region, an unsupported instruction -- so | ||
| ;;; the stock mapping would report ordinary bad input as a libquil bug. Mapping | ||
| ;;; CL:ERROR to LISP_ERR_FAILURE keeps the message clean and matches how libquil | ||
| ;;; behaved before it adopted the built-in error handling. | ||
| ;;; | ||
| ;;; WRAP-ERROR-HANDLING is consulted when a callable is compiled, so this affects | ||
| ;;; only the callables compiled after it -- libquil's own. The APIs already | ||
| ;;; compiled into libsbcl_librarian keep the stock behaviour. |
There was a problem hiding this comment.
This all seems reasonable to me. I am not able to verify that the sbcl-librarian calls themselves do what the comment says, though.
| err "Unsupported CPU architecture for macOS: ${ARCH}. Only Apple Silicon (arm64) is supported." \ | ||
| "Intel macOS builds are no longer published. You can build libquil from source; see" \ | ||
| "https://github.com/rigetti/libquil#building-from-source" | ||
| ;; |
There was a problem hiding this comment.
Building for Intel Macs is much more onerous now that they have been discontinued, and it doesn't seem worthwhile to me.
| library_is_available() { | ||
| local soname="lib${1}.${LIBQUIL_LIB_SUFFIX}" | ||
|
|
||
| # The loader's own cache is authoritative where it exists. |
There was a problem hiding this comment.
This comment looks like it belongs on the ldconfig branch...?
| # `brew install openblas` from being reported as missing, at the cost of not | ||
| # catching the case where magicl ends up unable to load a keg-only install. | ||
| local dir | ||
| for dir in /usr/local/lib /usr/lib /opt/homebrew/lib /opt/homebrew/opt/openblas/lib |
There was a problem hiding this comment.
Per the comment above, it sounds like there's no canonical way to search "all the paths where dynamic libs might be expected to exist", but if there is, we should do that instead.
There was a problem hiding this comment.
man ld on macos says /usr/lib then /usr/local/lib, then other paths specified manually.
This SO answer says the DYLD_LIBRARY_PATH envvar is also checked. man dyld lists all envvars that get used.
otool -L <bin> lists all of the libraries a binary is linked against and where they are found, similar to ldd on Linux.
find / -name "${soname}" would be the exhaustive option.
locate <soname> works, but I got this message when I initially tried to run it:
WARNING: The locate database (/var/db/locate.database) does not exist.
To create the database, run the following command:
sudo launchctl load -w /System/Library/LaunchDaemons/com.apple.locate.plist
Please be aware that the database can take some time to generate; once
the database has been created, this message will no longer appear.
Since we require pkg-config for zstd support, we could maybe use that here too. Assuming blas and lapack (and openblas?) (the only libraries this function appears to be called with) all provide pkg-config files.
| local soname="lib${1}.${LIBQUIL_LIB_SUFFIX}" | ||
|
|
||
| # The loader's own cache is authoritative where it exists. | ||
| if [[ -z "${IS_LINUX-}" ]] |
There was a problem hiding this comment.
Not sure why this isn't using the same OS ... Darwin check above, which seems more explicit.
| # Installing into /usr/local needs root. Container images commonly run as root without | ||
| # sudo installed, where calling it would fail even though nothing needs elevating. |
There was a problem hiding this comment.
If preferred, we can remove simply sudo "${0}"; exit $? if the current user isn't root, so that the commands below can be written without ${SUDO}.
There was a problem hiding this comment.
Yeah I'd be fine with
if [[ "$(id -u)" -ne 0 ]]; then
echo "This script must be run as root!"
exit 1
fi
There was a problem hiding this comment.
Again, a fair bit of "magic" here, but the changes generally look reasonable to me.
| `magicl` loads `BLAS` and `LAPACK` at runtime under their unversioned names, | ||
| `libblas.so` and `liblapack.so` (`.dylib` on macOS). Distributions ship those names in | ||
| their development packages, so installing only a runtime package such as Debian's | ||
| `libblas3` — which provides `libblas.so.3` and no unversioned symlink — is not enough. |
There was a problem hiding this comment.
If I'm reading this correctly, it should just say that the -dev packages need to be installed, not just the runtime packages.
| > Note: do not install Homebrew's `lapack` on Apple Silicon. `magicl` prefers it | ||
| > over every other backend, and it computes incorrect eigenvectors there, which | ||
| > surfaces as `Could not find diagonalizer for matrix ... after 16 attempts` | ||
| > during compilation. |
There was a problem hiding this comment.
See other comment about wanting to validate this claim & report upstream
| Building requires an SBCL with a *linkable runtime* (`libsbcl.so`). Neither | ||
| `make.sh` nor any package manager produces one — Homebrew's `sbcl` bottle, for | ||
| instance, does not — so SBCL has to be built from source: |
There was a problem hiding this comment.
see other comment about "any package manager" being perhaps overbroad
| ### D4: Build the runtime manually rather than via its CMake project | ||
|
|
||
| Upstream's `lib/CMakeLists.txt` invokes the generator with `sbcl --script`, which | ||
| skips `~/.sbclrc` and therefore Quicklisp — so `generate-bindings.lisp`'s | ||
| `(asdf:load-system :swank)` fails with `Component :SWANK not found`. | ||
|
|
||
| Rather than patch upstream's CMake, `src/build-image.lisp` defines the runtime's | ||
| aggregate library itself and emits `runtime/sbcl_librarian.c` alongside libquil's | ||
| own bindings. One image therefore produces everything: libquil's bindings, the | ||
| runtime's bindings, and the core that backs both. That also avoids the swank | ||
| dependency and the second, discarded core that upstream's generator would build. |
There was a problem hiding this comment.
I'm not sure if "upstream" here is referring to SBCL or sbcl-librarian. If the latter, perhaps we should patch the CMake setup.
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
| - [x] CI, `install.sh` and release archives updated for the new artifact set | ||
| - [x] Packaging verified by simulating package -> zip -> install -> build and | ||
| running the libquil-sys suite against the installed layout (22/22) | ||
|
|
There was a problem hiding this comment.
If we keep this file, obviously the file should be truncated before this section
| # libquil depends on three sbcl-librarian fixes (secondary-system FASL bundles, | ||
| # library prefix, configurable core name). They live on a branch of Rigetti's | ||
| # fork; move this back to quil-lang/sbcl-librarian once they are upstream. | ||
| SBCL_LIBRARIAN_REPO: "https://github.com/rigetti/sbcl-librarian.git" | ||
| SBCL_LIBRARIAN_REF: "fix-secondary-system-bundles" |
There was a problem hiding this comment.
Do we have an issue to track these? (Version tag and switching back to upstream)
| # for current cl-quil: the long-standing 2022-04-01 pin predates clos-encounters | ||
| # and fails with SYSTEM-NOT-FOUND. |
There was a problem hiding this comment.
Is the note about the 2022 version necessary?
There was a problem hiding this comment.
Nope, one of Claude's "useful for reviewers but not posterity" comments; will remove
| ~/quicklisp | ||
| ~/.sbclrc | ||
| key: ${{ runner.os }}-build-${{ hashFiles('**/versions') }} | ||
| key: ${{ env.ImageOS }}-${{ runner.arch }}-build-${{ hashFiles('**/versions') }} |
There was a problem hiding this comment.
It's provided by the GHA runner docker-image, but is unfortunately not yet documented (despite being available for a few years now): github/docs#40108
| # libffi-dev pulls in whichever libffi runtime the image ships; naming | ||
| # it explicitly breaks whenever the runner image moves (libffi7 does not | ||
| # exist on ubuntu 24.04, which ubuntu-latest now resolves to). | ||
| run: sudo apt update && sudo apt install -y libblas-dev libffi-dev liblapack-dev libz-dev gfortran |
There was a problem hiding this comment.
Are libffi 7 and 8 both supported? If not, I think I'd rather the breakage here over weird errors elsewhere.
| # one being built -- building the old sbcl-2.2.4 with a current host fails in | ||
| # make-host-1 with "undefined variable: SB-VM::END". |
There was a problem hiding this comment.
Not sure the note about what the old version does is that relevant
| local dir | ||
| for dir in /usr/local/lib /usr/lib /usr/lib64 /lib /lib64 | ||
| do | ||
| [[ -e "${dir}/${soname}" ]] && return 0 | ||
| done | ||
| return 1 |
There was a problem hiding this comment.
This duplicates the logic in the "not linux" section but with fewer paths. Since this is bash, we could make an array of paths to search, and append homebrew to it if brew is detected.
| for lib in blas lapack | ||
| do | ||
| library_is_available "${lib}" || LIBQUIL_MISSING+=("lib${lib}.${LIBQUIL_LIB_SUFFIX}") | ||
| done |
There was a problem hiding this comment.
I'm not sure why the variable is called LIBQUIL_MISSING, but this loops over blas and lapack.
| # Installing into /usr/local needs root. Container images commonly run as root without | ||
| # sudo installed, where calling it would fail even though nothing needs elevating. |
There was a problem hiding this comment.
Yeah I'd be fine with
if [[ "$(id -u)" -ne 0 ]]; then
echo "This script must be run as root!"
exit 1
fi| sudo xattr -r -d com.apple.quarantine /usr/local/lib/libquil.dylib | ||
| sudo xattr -r -d com.apple.quarantine /usr/local/lib/libquil.core | ||
| sudo xattr -r -d com.apple.quarantine /usr/local/lib/libsbcl.so | ||
| ${SUDO} xattr -r -d com.apple.quarantine "${LIBQUIL_LIB_PREFIX}/libquil.dylib" |
There was a problem hiding this comment.
Might echo a message saying we're doing this
| # The runtime is told to load libquil.core rather than the stock | ||
| # sbcl_librarian.core, so that libquil's image is what comes up. |
There was a problem hiding this comment.
I thought another comment said we disable sbcl_librarian.core from being generated?
2 participants
Updates
sbcl-librarianand adopt its new conventions.The motivation here is to get
libquilworking on Apple Silicon, but the approach required rearchitecting to account for upstream changes inquilc, specifically updates to thesbcl-librarianFFI layer.The change was generated by Claude Code, and the design decisions are documented in the REARCHITECTURE file.