Skip to content

Resurrect CI: modernize workflows and fix bit-rot (no dependency changes) - #28

Merged
mlin merged 21 commits into
mlin:mainfrom
Wid-L-Hacker:conrad/ci-resurrection
Oct 20, 2026
Merged

mlin merged 21 commits into
mlin:mainfrom
Wid-L-Hacker:conrad/ci-resurrection

Conversation

@Wid-L-Hacker

Copy link
Copy Markdown
Contributor

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.

…BC/Rust/Python bit-rot fixes (pre-phase-2 state, = conrad/ci-modernization @ 4c4f5de)

@mlin mlin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

Comment thread .github/workflows/build.yml
Comment thread bindings/python/genomicsqlite/__init__.py Outdated
Comment thread Dockerfile Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread bindings/rust/build.rs
Comment thread bindings/rust/build.rs Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
mlin and others added 5 commits October 18, 2026 21:54
…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 mlin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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:

  1. The new JDBC smoke test searches the project's deployment repository for runtime dependencies that are not deployed there, so its Java invocation fails.
  2. 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.
  3. 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.

Comment thread .github/workflows/build.yml Outdated
Comment thread bindings/python/genomicsqlite/__init__.py Outdated
Comment thread test/pkg_smoke.sh
Comment thread bindings/python/release.sh
Wid-L-Hacker and others added 5 commits October 18, 2026 23:16
- 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

@mlin mlin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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_init entry 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.sh is 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.

@mlin

mlin commented Oct 20, 2026

Copy link
Copy Markdown
Owner

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 6fab91f.

Changes relative to this PR: compare.

Validation: all six CI jobs passed at 631d24d: lint, Linux Debug, Linux Release, portable Linux, macOS ARM, and final artifacts/JDBC.

1. Make loading independent of the library filename

The 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 sqlite3_extension_init, and only guesses a name from the filename if that standard symbol is absent.

The branch adds a small forwarding entry point in src/genomicsqlite.cc:

extern "C" int sqlite3_extension_init(sqlite3 *db, char **pzErrMsg,
                                      const sqlite3_api_routines *pApi) {
    return sqlite3_genomicsqlite_init(db, pzErrMsg, pApi);
}

The existing sqlite3_genomicsqlite_init remains available for callers that already specify it. Python returns to _MEMCONN.load_extension(_DLL), and the shell uses .load "path" with no explicit initializer. This also makes direct loading of renamed libraries work for consumers outside our bindings.

I reproduced the original problem: copying the old library to libgenomicsqlite-linux-x86_64.so makes ordinary loading fail with undefined symbol: sqlite3_genomicsqlitelinuxx_init. Explicit initialization succeeds; the new standard entry point also succeeds through ordinary loading.

The new test/extension_loading.py, registered in CTest, checks four filenames (canonical, Linux-qualified, macOS-qualified, and an arbitrary name containing a space), both default and legacy named initialization, and both Python and the shell: 16 combinations. Each load runs in a fresh process; the Python cases also verify automatic registration after closing the loading connection. The test rejects the original library for the expected reason. All cases pass locally with SQLite 3.41.2, 3.46.1, and 3.52.0, and on macOS ARM in CI with SQLite 3.54.1.

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 explanation

The earlier failed ARM job used Python 3.12.10 and failed at _MEMCONN.enable_load_extension(True) with AttributeError. Its pytest output identifies /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.

The important distinction is in GitHub's macOS Python builder: its source-build configuration enables SQLite extension loading, but Build() uses python.org installer packages for ARM (and for Python >=3.11). Thus restoring setup-python with ARM Python 3.12 does not reproduce the older Intel Python 3.8 source-build setup. The source configuration alone was misleading until I followed which build path actually runs.

GenomicSQLite already required enable_load_extension before this PR. Homebrew is a justified way to supply an enabled interpreter. Python 3.12 specifically is a CI baseline, not a new library requirement. An intermediate resurrection run actually ran Homebrew Python 3.15.0 and passed CTest despite its attempted 3.12 PATH selection.

The branch retains explicit Homebrew Python/venv selection, shortens the comment to the verified distinction, checks enable_load_extension(True) itself rather than just method presence, and logs the executable/Python/SQLite versions. It uses GITHUB_PATH to add the SQLite shell instead of reconstructing the whole PATH again. The successful macOS run confirms the venv's Python 3.12.15, all six CTest suites, Rust checks, and the installed-package API/CLI smoke.

3. The hard-coded Rust version fallback can be removed by supplying the missing tag

I checked the remote tag lists: this fork currently has no tags, while upstream has v0.9.4. The relevant jobs already use actions/checkout with fetch-depth: 0, which fetches all history and tags—but it cannot fetch tags that do not exist in the fork.

I reproduced this with an isolated repository containing the PR's full history and no tags: git describe --tags fails. After fetching just upstream's v0.9.4, the same history describes as v0.9.4-13-g6fab91f, which is the intended Cargo version after stripping v.

Assuming origin names the fork, importing that release tag is sufficient:

git fetch https://github.com/mlin/GenomicSQLite.git tag v0.9.4
git push origin refs/tags/v0.9.4

Fetching 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 git describe --tags --dirty in both Cargo wrappers and dropping the duplicated 0.9.4-$(git rev-parse --short HEAD) fallback and its maintenance comments. The Docker Rust stage receives the checkout's .git, so it receives the tag as well. The separate Docker safe.directory fix is still relevant to ownership and should remain.

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.

Wid-L-Hacker and others added 5 commits October 19, 2026 16:41
- 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 mlin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

Comment thread Dockerfile Outdated
@mlin

mlin commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

Remaining requests for this PR, followed by a suggestion for a separate binding-selection follow-up.

macOS deployment and CPU baseline

Please try targeting macOS 11.0 for the shipped extension and explicitly specify -mcpu=apple-m1 in its C/C++ build flags. M1 is already Clang's default macOS ARM CPU target; spelling it out records the intended instruction-set baseline. The current artifact from run 39370493285 has ordinary ARM64 CPU subtype, but its LC_BUILD_VERSION declares minos 15.0 (sdk 15.5). Its dynamic dependencies are just system libcurl, libc++, and libSystem.

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 CMAKE_OSX_DEPLOYMENT_TARGET=11.0 (set before configuring CMake), plus the explicit M1 flags. Check the static Homebrew libzstd.a: if its objects target a newer OS or introduce newer runtime dependencies, build Zstandard from source for the same deployment target and CPU baseline and point the existing LIBZSTD_A cache variable at that archive. The final dylib's load commands and symbol requirements need checking; changing the declared minimum alone would not establish compatibility. An older-OS runtime smoke test would provide additional evidence; please distinguish the target version from the oldest version actually tested and report any blockers rather than expanding this into an overhaul of the whole CI toolchain.

Small mechanical cleanups in this PR

  1. Remove the Rust/JDBC extraction-basename workaround. The binaries built by this PR now export sqlite3_extension_init, so the bundled copy can retain its architecture-qualified resource filename. In Rust, pass env!("GENOMICSQLITE_BUNDLED_LIB") as the extraction basename instead of branching on .dylib versus .so. In JDBC, use basename directly instead of constructing plainBasename. Remove the accompanying comments claiming that qualified filenames fail. Keep resource extraction and its lifetime management; only the renaming is obsolete. This relies on packaging the current native binaries, as the release pipeline does; supporting bundles deliberately populated with older native binaries would be a separate compatibility requirement. Keep the installed-package smoke tests exercising the resulting names.
  2. Combine Rust's two identical None implementations of bundle_libgenomicsqlite() under #[cfg(not(all(feature = "bundle_libgenomicsqlite", bundled_lib)))]. This expresses the complement of the implementation that actually embeds a library.
  3. Trim and correct the explanatory comments. In particular, remove the Dockerfile claim that the Cargo wrapper tolerates a tag-less checkout: the restored git describe is deliberately fail-fast. Retain the safe.directory ownership explanation. Shorten the long selector and portable-runtime comments to the contracts a maintainer needs: select by compilation target, and scope the portable SQLite environment to the processes being tested. The history of superseded workarounds can stay in this PR discussion.

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 bindings

Let'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 mlin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 LIBGENOMICSQLITE unset 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.
@mlin
mlin merged commit 1826ef9 into mlin:main Oct 20, 2026
6 checks passed
@mlin
mlin deleted the conrad/ci-resurrection branch October 20, 2026 07:23
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