Repository navigation
Resurrect CI: modernize workflows and fix bit-rot (no dependency changes) - #28
Conversation
…BC/Rust/Python bit-rot fixes (pre-phase-2 state, = conrad/ci-modernization @ 4c4f5de)
mlin
left a comment
There was a problem hiding this comment.
Reviewed commit fbff4200e5b46707ec12ddd3ea6308c640730770. The portable build reproduces successfully, and the successful six-job CI run is useful evidence. I recommend changes before merging because the macOS artifact substitution is incompatible with the existing Python/JDBC package selectors, and some green checks bypass the behavior exercised by installed packages.
The inline comments distinguish correctness issues from design/maintenance recommendations. In particular, please align the macOS artifact names and selectors, fix CLI initialization, restore the Cargo scripts' executable permissions, execute the minimum-runtime Docker stages, account for libc ABI in Rust selection, and select the intended Homebrew interpreter explicitly.
I also recommend making missing Rust bundles a build-time failure, isolating the optional older SQLite runtime instead of installing it into the runner's global loader cache, and retaining the ELF compatibility assertions behind a small named checker. The old libstdc++ ABI flag and CMake policy compatibility shim have concrete compatibility reasons to stay.
Validation performed:
- Native Debug build, Python/C smoke tests, and 16 existing Python cases passed.
- Four CLI regression cases fail on the unchanged PR and pass with a local anonymous-file alternative on SQLite 3.46.1 and 3.52.0; interactive and spaced-path checks also passed.
- A smaller Rust selector passed both loading modes (2 unit tests and 1 doctest each), including the web test, and packaged-crate verification.
- The unchanged portable Docker build passed; its shell/extension also loads in fresh AlmaLinux 8.
The alternatives are local experiments. Full loader/JDBC/macOS suites were not rerun locally; package-selector probes and the upstream logs establish the specific macOS findings, and the proposed macOS changes still need runner validation.
…tors - P1: Python/JDBC selectors match OS+CPU exactly and stage the bundled library under its plain basename at load time; installed-package smoke test (test/pkg_smoke.sh) exercises the selector with LIBGENOMICSQLITE unset - P2a: CLI startup via anonymous inherited fd (-cmd '.read /dev/fd/N') instead of a -init tempfile; composes with a user-supplied -init - P2b: name the Docker test stages and build each target explicitly in CI - P2c: cargo wrappers reverted to main's bytes (fetch-depth: 0 checkouts have the tags); direct invocations restored at all call sites - P2d+R1: Rust bundling selects one OS/CPU/ABI path, validates it, and fails the build when a requested bundle is missing or ABI-incompatible - P2e: macOS job uses an explicit Homebrew python@3.12 venv and asserts version + enable_load_extension - R2: portable-library tests scoped to build/ instead of a global ldconfig plant; R3: ELF version-floor assertions extracted to test/check_elf_version_floor.py (build.yml is staged at the branch root in this commit; it moves to .github/workflows/ by rename, which also triggers CI.)
…ged at the branch root (rename restores it and triggers CI)
mlin
left a comment
There was a problem hiding this comment.
Re-reviewed e1ec04aee6952746bd76ea999d92176cbef67a03, focusing on the final tree.
Most of the earlier concerns are addressed in the implementation: architecture-qualified resources now match the Python/JDBC selectors; CLI startup uses an anonymous inherited file; the Cargo wrappers are executable; the Docker runtime targets are explicitly selected; Rust rejects a glibc bundle for musl and fails for a missing bundle on supported targets; Homebrew Python is selected explicitly; portable SQLite lookup is scoped; and ELF compatibility checking is factored into a helper.
Three remaining issues are detailed inline:
- The new JDBC smoke test searches the project's deployment repository for runtime dependencies that are not deployed there, so its Java invocation fails.
- Python's new native-library staging leaks directories/files when the installed CLI execs SQLite. An explicit extension entry point removes the need for staging.
- The package smoke test loses a relative shell directory after changing cwd and silently runs a different SQLite executable.
There is also a small executable-mode regression in the Python release script.
Validation:
- 16 existing Python cases plus four CLI regression cases passed with the updated bindings.
- The installed Python package/API/CLI smoke test passes, but separate cleanup probes reproduce the staging leak.
- Both Rust loading modes passed their two unit tests (including HTTP) and one doctest; target-selection probes confirm the missing-bundle and musl guards.
- Both JDBC unit tests pass. The workflow's packaged-JAR smoke invocation fails; the same program passes with the corrected runtime classpath and no library override.
- The ELF helper accepts the portable libraries and rejects binaries requiring newer GLIBC/GLIBCXX versions.
- A fresh AlmaLinux 8 C API smoke test passed using the previously reproduced portable core; native sources are unchanged.
The full updated Docker rebuild was not completed because dependency/package fetching stalled. macOS execution is still unverified locally, and the current upstream CI run awaits approval.
- cargo wrappers: restore the tag-less git describe fallback (the fork has no tags for checkout to fetch) - Dockerfile rust stage: safe.directory for the build-context .git - python: load the bundled library from its qualified name with the entry point named explicitly; delete the temp-dir staging (the CLI's exec leaked one staging dir per call) - pkg_smoke: canonicalize the shell dir before cd; assert the CLI leaves no staging dirs - artifacts: extract the JDBC smoke test to test/jdbc_smoke_test.sh; resolve the runtime classpath with dependency:build-classpath
…sh workflow files directly)
mlin
left a comment
There was a problem hiding this comment.
Re-reviewed 6fab91f209a4a2d41eb93acb9da7b64fbd32c14f. All four outstanding findings from the previous review are addressed, and I found no further code blockers.
Verified:
- The extracted JDBC smoke helper uses Maven's resolved runtime classpath and passes end to end. I exercised the exact helper with a packaged JAR containing both real platform resources; the JDBC implementation itself is unchanged since the previous review.
- Python now loads the architecture-qualified library through the explicit
sqlite3_genomicsqlite_initentry point. Six packaged CLI invocations across SQLite 3.46 and 3.52 left no staging directories. - The package smoke helper passes and actually invokes the requested shell when given a relative directory, verified with an instrumented executable. Its cleanup regression check also passes.
release.shis executable in the committed tree.- 16 existing Python cases plus four CLI regression cases pass; the CLI cases also pass separately on SQLite 3.52. The restored Cargo fallback produces a valid crate version in a tag-less checkout, and the ELF checker accepts the portable binaries.
I also checked the Linux job log from fork run 39294337754: both explicitly selected AlmaLinux runtime stages actually execute, including the Rust unit tests/doctest. That run passed lint, Debug, Release, and the portable Linux job; macOS was cancelled and artifacts were skipped. The current head differs from that restored-workflow commit only by the CI-trigger comment.
Approving the code changes and resolving the four remaining threads. A successful macOS build and final artifacts/JDBC job are still needed before merging; I have not executed macOS locally.
|
I followed up on the extension-loading and macOS Python questions with an alternative implementation and actual CI runs. Branch: mlin:codex/sqlite-loading, based on the current PR head Changes relative to this PR: compare. Validation: all six CI jobs passed at 1. Make loading independent of the library filenameThe explicit initializer in this PR works, but there is a simpler shared-library interface that lets us remove the special handling from the binding and CLI. SQLite's documented protocol first looks for The branch adds a small forwarding entry point in extern "C" int sqlite3_extension_init(sqlite3 *db, char **pzErrMsg,
const sqlite3_api_routines *pApi) {
return sqlite3_genomicsqlite_init(db, pzErrMsg, pApi);
}The existing I reproduced the original problem: copying the old library to The new The branch also removes the obsolete global temp-directory scan from the package smoke test and gives that test an EXIT trap to clean its own scratch directory. The installed-package API and CLI checks remain. 2. The macOS Python failure now has a concrete explanationThe earlier failed ARM job used Python 3.12.10 and failed at The important distinction is in GitHub's macOS Python builder: its source-build configuration enables SQLite extension loading, but GenomicSQLite already required The branch retains explicit Homebrew Python/venv selection, shortens the comment to the verified distinction, checks 3. The hard-coded Rust version fallback can be removed by supplying the missing tagI checked the remote tag lists: this fork currently has no tags, while upstream has I reproduced this with an isolated repository containing the PR's full history and no tags: Assuming git fetch https://github.com/mlin/GenomicSQLite.git tag v0.9.4
git push origin refs/tags/v0.9.4Fetching locally alone will not fix Actions; the tag must be pushed to the fork so the checkout action can obtain it. With that in place, I recommend restoring the original fail-fast This tag cleanup is a recommendation for the PR; I have not changed the fork's tags or included it in the experiment branch. The standard entry-point change and the tested Homebrew setup above are the changes I recommend taking from that branch. |
- src/genomicsqlite.cc: add the standard sqlite3_extension_init entry point forwarding to sqlite3_genomicsqlite_init, so loading no longer depends on the library filename; the named entry point stays for callers that already specify it - python: plain load_extension(_DLL) and .load with no explicit initializer (architecture-qualified _DLL selection unchanged) - test/extension_loading.py: new CTest-registered loading matrix (4 filenames x default/legacy init x Python/shell) - test/pkg_smoke.sh: drop the obsolete staging-dir scan; EXIT trap cleans the script's own scratch dir - macOS job: tested Homebrew python@3.12 venv setup (shorter comment; enable_load_extension(True) check with version logging; GITHUB_PATH adds the SQLite shell instead of reconstructing PATH) - docs: note the interpreter loadable-extensions requirement (build.yml is staged at the branch root in this commit; it moves to .github/workflows/ by rename, which also triggers CI.)
…nch root (rename restores it and triggers CI)
mlin
left a comment
There was a problem hiding this comment.
Re-reviewed 2e5ede4f5a732cd591db1296ced4905022b12f13. No new code blockers; approving. There is one non-blocking stale Docker comment noted inline.
The adopted standard entry point, Python loading calls, loading regression, installed-package smoke helper, macOS workflow, and documentation match the tested codex/sqlite-loading experiment byte for byte. The old named initializer remains available. The fork now has the upstream release tags, and both Cargo wrappers are restored byte for byte to the original fail-fast implementation, with executable modes preserved. The current head describes as v0.9.4-18-g2e5ede4.
Validation:
- Fork CI at this exact head passed all six jobs: lint, Linux Debug, Linux Release, portable Linux, macOS ARM, and artifacts/JDBC. Every reported step succeeded, including both explicitly selected minimum-runtime Docker stages and the installed-package checks.
- A fresh snapshot of this head passed 16 existing Python tests plus four CLI regression cases, all 16 default/named initializer loading combinations, and the installed-package API/CLI smoke with SQLite 3.52. These local runs reused the native library built from the byte-identical experiment sources; I did not rebuild that unchanged native implementation locally.
- All four relevant scripts have mode 100755 in the committed tree. The 13 earlier inline threads remain resolved.
The main-repository run is separately marked action_required pending workflow approval; the successful fork run above supplies the runtime evidence for this review.
|
Remaining requests for this PR, followed by a suggestion for a separate binding-selection follow-up. macOS deployment and CPU baselinePlease try targeting macOS 11.0 for the shipped extension and explicitly specify The scope is the release extension and the native code linked into it. CI, Homebrew Python, Maven, and the other test/build tools can stay on macOS 15. Start with Small mechanical cleanups in this PR
Please retain the missing-bundle errors, ABI guards, compatibility checks, and installed-package smoke tests. These cleanups shouldn't weaken their behavior or introduce a new selection framework here. Suggested follow-up: consistent native-library selection across bindingsLet's handle the broader binding-selection design separately. Define one documented policy for supported OS/architecture/libc combinations, architecture aliases, the filenames actually shipped, overrides, and unsupported-target/missing-library behavior. Each binding can implement that policy with appropriate language facilities, with shared conformance cases to keep the results aligned. Rust must continue using the compilation target, rather than the machine running its build script. For Java, investigate reusing SQLite JDBC's platform utility, checking compatibility with our supported dependency versions. For Python, evaluate platform wheels so the package installer can do more of the selection, including libc and minimum-OS compatibility. Those options need investigation rather than additional machinery in this PR. The follow-up should also include updating the language bindings guide in the docs. This follow-up needn't hold up this resurrection PR. |
…ion; extraction-basename and comment cleanups - macOS job: configure the extension build with CMAKE_OSX_DEPLOYMENT_TARGET=11.0 and -mcpu=apple-m1; check Homebrew libzstd.a against that target (source build + LIBZSTD_A fallback if its objects require a newer macOS); assert the shipped dylib declares minos 11.0 in the symbols & digests step - Rust/JDBC: extract the bundled library under its architecture-qualified name -- the built binaries export sqlite3_extension_init, so the plain-basename renaming workaround and its comments are obsolete - Rust: merge the two identical None bundle_libgenomicsqlite() impls under the single complement cfg - Trim the selector and portable-runtime comments to their contracts; remove the Dockerfile claim that the cargo wrapper tolerates a tag-less checkout (the restored git describe is deliberately fail-fast); the safe.directory ownership note stays (build.yml is staged at the branch root in this commit; it moves to .github/workflows/ by rename, which also triggers CI.)
…ged at the branch root (rename restores it and triggers CI)
…p explicit -mcpu=apple-m1 Mike checked Homebrew's libzstd.a carries the same baseline, so the 11.0 deployment target, the libzstd deployment-target step with its LIBZSTD_A source-build fallback, and the minos assertion are removed again. The macOS job is now the pre-refinement version plus the explicit M1 flags. Everything else from the refinement round stands. (build.yml remains staged at the branch root; rename restores it and triggers CI.)
mlin
left a comment
There was a problem hiding this comment.
Re-reviewed bff979acf4223d03f78d242d24185f5bf51a75d8. No code blockers; approving the current implementation.
The mechanical cleanups are in place: Rust and JDBC extract the bundled library under its architecture-qualified resource name, Rust's two identical non-bundled implementations are combined under the complementary cfg, and the selector/portable-runtime comments are shorter. The stale Docker claim about tag-less checkouts is removed, so I resolved that final old inline thread; all 14 earlier threads are now resolved. The missing-bundle checks and ABI guards are preserved.
The macOS 11 request in my earlier comment is withdrawn following the subsequent compatibility discussion. The agreed scope is Apple Silicon prebuilts with an M1 instruction baseline and macOS 15 minimum. This revision keeps the explicit -mcpu=apple-m1 flags and removes the attempted macOS 11 targeting/dependency rebuild. I downloaded the artifact from this exact revision: its Mach-O build metadata declares macOS 15.0, SDK 15.5, and ordinary ARM64 CPU subtype. The build log also confirms the M1 compiler flag.
Validation:
- Fork CI at this exact head passed all six jobs, including portable Linux, macOS ARM, and artifacts/JDBC. The macOS bundled Rust unit tests/doctest and installed Python package check pass; the final packaged JDBC smoke also passes.
- A fresh local snapshot passed the bundled Rust smoke test and doctest, plus both unit tests (including HTTP) and the doctest without default features. The JDBC unit tests, Spotless check, and packaged-JAR smoke pass, with
LIBGENOMICSQLITEunset for the packaged smoke. These local binding tests reused the already-built native library; the native sources are unchanged from the previous reviewed revision.
One small, non-blocking documentation correction remains: README.md:25 still says the supplied binaries are for “Linux and macOS on x86-64.” Please update that sentence to Linux x86-64 and macOS Apple Silicon (macOS 15+). The broader binding-selection design and language bindings guide update can remain a separate follow-up as agreed.
…acOS 15+) Correct the stale line 25 platform claim from PR mlin#28 approval review 5581776926.
CI had bit-rotted across the board: retired actions/runners, CentOS 7 EOL breaking the portable build, and Maven/JDBC, Rust, and macOS failures.
This PR modernizes the workflows (incl. macOS ARM), moves the portable build to AlmaLinux 8 (glibc 2.28 floor), and applies the minimal code/test/build fixes.
CI status: fully green - Actions run 39197670905, all six jobs incl. release artifacts.
No dependency changes and no SQLite version change (pin stays 3.41.2); modernization follows in separate PRs.