-
Notifications
You must be signed in to change notification settings - Fork 64
kernel: distribute darwin/arm64 lib as nested per-platform module (go get, no build step) #440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,3 +52,12 @@ require ( | |
| github.com/rs/zerolog v1.28.0 | ||
| golang.org/x/sys v0.45.0 // indirect | ||
| ) | ||
|
|
||
| // Nested per-platform kernel library modules. Each carries one platform's | ||
| // prebuilt kernel static archive + its cgo link directive, so a build downloads | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — The nested module is pulled in with an unconditional But This directly contradicts the README section added in this PR ("works straight from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — The nested-module wiring here works only for in-tree builds — which is all the PR's verification actually exercised (
Net effect: the |
||
|
|
||
| replace github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 => ./internal/backend/kernel/kernellib/darwin_arm64 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium — The nested module is wired with
require .../kernellib/darwin_arm64 v0.0.0+ a localreplace. This works for in-tree builds, but note two things that undercut the PR's "works straight fromgo 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 getconsumer 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 therequireto match. Until then, externalgo getof a tagged release would break even for Thrift-only users.