kernel: distribute darwin/arm64 lib as nested per-platform module (go get, no build step) - #440
kernel: distribute darwin/arm64 lib as nested per-platform module (go get, no build step)#440msrathore-db wants to merge 3 commits into
Conversation
Commit the prebuilt darwin/arm64 kernel static archive in a nested per-platform Go module (internal/backend/kernel/kernellib/darwin_arm64) plus the C header, so a kernel opt-in build works from 'go get' with no 'make kernel-lib' step. The nested module is imported for its cgo link side-effect only under darwin && arm64 && databricks_kernel, so a pure-Go Thrift build or a non-darwin build never compiles or downloads it. Co-authored-by: Isaac
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium
Distribution-model change (nested per-platform kernel module) looks sound for the in-tree build: build constraints on cgo_darwin.go and link.go match, the default Thrift build stays CGO-free, and .gitignore/header handling is consistent. One Medium concern: the require v0.0.0 + local replace won't resolve for external go get consumers once a release is cut, since replace isn't transitive and module-graph resolution ignores build tags — the release must be gated on publishing/tagging the nested module.
| // only the archive for the platform it targets (and nothing at all for a | ||
| // pure-Go Thrift build). The replace pins them to the in-tree directories; when | ||
| // published, the require versions are what a `go get` consumer resolves. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.0 |
There was a problem hiding this comment.
🟡 Medium — The nested module is wired with require .../kernellib/darwin_arm64 v0.0.0 + a local replace. This works for in-tree builds, but note two things that undercut the PR's "works straight from go get" goal for external consumers:
-
replaceis not transitive. A downstream project that doesgo get github.com/databricks/databricks-sql-goignores this repo'sreplacedirective entirely (replace is honored only in the main module). It sees only the barerequire .../darwin_arm64 v0.0.0. -
Module-graph resolution is build-tag-independent. MVS must load the
go.modof everyrequired module to build the graph, even for a pure-Thrift (CGO_ENABLED=0, no tag) build that never compiles a file from the nested module. Becausev0.0.0is not a published/tagged version of the nested module, that resolution would fail for all consumers — not just kernel builds — with an "unknown revision" error, once a release of this repo is cut carrying thisgo.mod.
The PR description acknowledges this ("when published, the require versions are what a go get consumer resolves"), so this is a known follow-up rather than a defect in the in-tree workflow. Flagging so the release that publishes this is gated on: tagging the nested module at a real version and updating the require to match. Until then, external go get of a tagged release would break even for Thrift-only users.
Add sync-kernel-libs.yml: a manually-triggered workflow that downloads the per-platform libdatabricks_sql_kernel.a archives built by the kernel repo's build-c-abi-libs workflow for a chosen kernel release tag, verifies each checksum, places them into the nested internal/backend/kernel/kernellib/<platform> modules, moves KERNEL_REV to the tag's commit, and opens a sync PR. This is the consumer half of the committed-per-platform kernel distribution: it keeps the committed archives (and header + KERNEL_REV) in lockstep with a kernel release without a manual copy, while leaving the actual bump as a reviewed PR. Co-authored-by: Isaac
There was a problem hiding this comment.
Verdict: 1 High · 1 Low
Solid, well-documented distribution change; the nested-module wiring is correct for in-repo builds. Main concern (High): the unconditional require ... v0.0.0 + local replace in go.mod resolves in-repo but not for external go get consumers (replace is ignored downstream), which would break the default build until the nested module is actually published/tagged — contradicting the README's "works straight from go get" claim. One Low note on the sync workflow swallowing go mod verify failures. Nit: the verify step's comment says it "type-checks the darwin build" but the command builds linux/amd64.
| // only the archive for the platform it targets (and nothing at all for a | ||
| // pure-Go Thrift build). The replace pins them to the in-tree directories; when | ||
| // published, the require versions are what a `go get` consumer resolves. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.0 |
There was a problem hiding this comment.
🟠 High — The nested module is pulled in with an unconditional require ... v0.0.0 plus a replace => ./internal/backend/kernel/kernellib/darwin_arm64. This resolves correctly for in-repo builds (make test, go build ./..., CI), because replace short-circuits the placeholder v0.0.0.
But replace directives are ignored in any module other than the main module (Go modules reference). So for an external consumer that does go get github.com/databricks/databricks-sql-go, the replace does not apply and Go must resolve .../kernellib/darwin_arm64@v0.0.0 from the proxy/VCS. Since the top-level require is unconditional, this module's go.mod is loaded for every consumer build graph — including a pure-Thrift, CGO_ENABLED=0 build that never compiles a kernellib file. Unless the tag internal/backend/kernel/kernellib/darwin_arm64/v0.0.0 actually exists in the published repo, go get/go build fails at module resolution (unknown revision v0.0.0) for all consumers, not just kernel users.
This directly contradicts the README section added in this PR ("works straight from go get with no build step") and the PR's headline claim. The PR text acknowledges "when published, the require versions are what a go get consumer resolves" — so the gap is known, but as it stands the merged+tagged state would break the default build for downstream consumers. Please confirm the publish/tagging plan makes v0.0.0 (or the eventual pinned version) resolvable before the README advertises frictionless go get, or gate the require so a Thrift build doesn't force resolution.
| branch="sync/kernel-${KERNEL_TAG}" | ||
| git config user.name "databricks-ci" | ||
| git config user.email "ci@databricks.com" | ||
| git checkout -b "$branch" |
There was a problem hiding this comment.
🔵 Low — go mod verify || true swallows any failure, so a genuinely corrupt/tampered module cache would not fail this "verify module graph resolves" step — only the subsequent go build gates the job. The comment frames this step as catching a broken go.mod/missing module; the || true defeats the go mod verify half of that intent. Consider dropping || true (or logging explicitly why a failure is tolerated) so the checksum verification of the freshly-committed archive can actually fail the sync PR.
Remove sync-kernel-libs.yml. The kernel repo's build-c-abi-libs workflow now PUSHES the compiled per-platform archives into this repo and opens the sync PR (see databricks-sql-kernel#244). Push keeps the cross-repo credential in the private kernel repo (a token with PR-write on this public driver only), instead of putting a kernel-source-read token in this public repo — which the pull model would have required. Co-authored-by: Isaac
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-documented build/distribution change; the build constraints on cgo_darwin.go and the nested link.go match, and the Thrift-build exclusion reasoning is sound. One medium concern: the require v0.0.0 + local replace only makes the kernel backend build in-tree — replace is ignored downstream and v0.0.0 isn't resolvable, so the "works from go get" claim isn't met for external consumers until the nested module is published at a real version.
| // only the archive for the platform it targets (and nothing at all for a | ||
| // pure-Go Thrift build). The replace pins them to the in-tree directories; when | ||
| // published, the require versions are what a `go get` consumer resolves. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.0 |
There was a problem hiding this comment.
🟡 Medium — The nested-module wiring here works only for in-tree builds — which is all the PR's verification actually exercised (go build ./... run from within this repo, where the replace applies). It does not deliver the headline "works straight from go get, no build step" for external consumers:
replacedirectives are ignored for dependencies. When someone addsdatabricks-sql-goas a dependency, only their main module'sreplacedirectives take effect; thereplaceon line 62 of this repo'sgo.modis dropped. So a downstreamgo build -tags databricks_kernelfor darwin/arm64 will try to resolve.../kernellib/darwin_arm64 v0.0.0from the module proxy, not from./internal/....v0.0.0is not a resolvable version. There is nointernal/backend/kernel/kernellib/darwin_arm64/v0.0.0tag, so that resolution fails for a consumer. go-duckdb's model (cited in the PR) requires the nested modules to be published as real tagged versions that the parentrequires directly — not av0.0.0placeholder held together by an in-treereplace.
Net effect: the go get path the PR is built around is currently only proven for builds run inside this checkout. Recommend either (a) tempering the README/PR claim to "builds from a repo checkout" until the nested modules are published and required at real versions, or (b) documenting the publish+version-bump step as a hard prerequisite before the kernel backend is advertised as go-get-installable. The Thrift-build verification is unaffected (the import is build-tag-excluded), so this only concerns the kernel-tag darwin/arm64 consumer path.
1 participant
What
Distribute the darwin/arm64 kernel static library as a nested per-platform Go module committed in-repo, so the SEA/kernel backend works straight from
go getwith nomake kernel-libstep — and documentgit clone --filter=blob:noneso direct clones stay light despite the committed binary.This is the go-duckdb distribution model (
duckdb-go-bindingscommits prebuilt per-platform.as in nested modules). Our per-platform archive (~62 MB, 1 file) is 2–8× lighter than theirs (~104–137 MB, ~18 files/platform).Changes
internal/backend/kernel/kernellib/darwin_arm64/— its owngo.mod, the committedlibdatabricks_sql_kernel.a, andlink.gocarrying the#cgo LDFLAGS(guarded tocgo && databricks_kernel && darwin && arm64).cgo_darwin.goin the kernel package is now a blank-import shim that pulls the nested module in for its link side-effect (same build constraint), instead of holding the LDFLAGS +${SRCDIR}/lib/...path itself.go.modgainsrequire+replacefor the nested module.internal/backend/kernel/include/databricks_kernel.h(small, platform-independent, needed at compile time) and un-ignored it in.gitignore. Thelib/scratch dir stays ignored (still used bymake kernel-libfor linux/windows and source builds).--filter=blob:none(+--sparse) and a note thatgo getconsumers pull no git history and no kernel binary for a Thrift build.Why nested (not flat) module
Go fetches a module's zip only when the build compiles a file from it. Because the nested module's only Go file is build-tag-constrained to
darwin && arm64 && databricks_kernel, a Thrift build or a non-darwin build never downloads the darwin archive — an end user pulls only their own platform, not all of them. (go list -depsfor the default build shows 0kernellibpackages.)Verification (darwin/arm64, live on pecotesting)
CGO_ENABLED=0, no tag):go build ./...OK; nested module absent from the build graph (0kernellibdeps); pulls no kernel binary.CGO_ENABLED=1 -tags databricks_kernel):go build ./...OK;go list -depsconfirms the nested module IS linked in.TestKernelE2E*— 23 PASS, 1 SKIP (M2M), 0 FAIL through the nested-module link path (scalar/decimal/temporal/nested types, CloudFetch, cancellation, retries, proxy, telemetry).Cost (documented, accepted)
Committed binaries can't be delta-compressed, so a full clone's history grows per release.
git clone --filter=blob:nonecollapses.gitto a few MB and does not grow with release count (measured: 5 releases → 304 MB full vs ~3 MB partial.git). The flag is a clone-time choice (documented + used in CI), and GitHub warns at 50 MB (the.ais 59 MB; under the 100 MB hard limit). The only alternative with zero committed binary is themake kernel-libdownload step, deliberately traded away here for a frictionlessgo get.Follow-ups
linux_amd64/linux_arm64/windows_amd64nested modules the same way (this PR does darwin/arm64, the platform verified on-device).--filter=blob:none.This pull request and its description were written by Isaac.