Conversation
ed14bd0 to
259a74f
Compare
|
I wonder if I should just move driverbase to be part of flightsql (since we don't really expect to export it anyways) to get the OpenTelemetry dependency out of core, or even use the thirdparty driverbase fork |
I think that's fine since we have we have https://github.com/adbc-drivers/driverbase-go |
|
Ah, but we still have |
|
Alright, that gets gRPC entirely out of the core API...we still depend on the core/trace OTel packages, but they seem to have minimal dependencies, and we still have Protobuf due to some error metadata definitions, but that should also hopefully be OK. |
|
You're going to need to update the release scripts to add a new tag when we do releases in addition to the |
|
Hmm, how is this supposed to work with the go.mod override? Rust/Cargo handles this properly with workspaces, but I suppose for a release we need to remove the override and point at the new tag? |
|
Ok, I think I'll go with using a Go workspace, and updating scripts/instructions so we auto-publish go/adbc, and then we need a second maintainer step to update and publish go/driver. |
|
I think for now, I'm not gonna fiddle with go.work, although it means the Flight SQL driver technically uses older API definitions, but I think that's OK since we moved all the driver code anyways. Perhaps at some point we should consider splitting the Flight SQL driver into a separate Apache repository |
zeroshade
left a comment
There was a problem hiding this comment.
The split itself is well-executed: clean renames, go mod tidy is a no-op for both modules, both build and vet clean, the shared_utils.go split has zero duplication, and license regeneration looks right (only test-only deps dropped, otelgrpc added). The CVE-isolation goal is achieved.
Two issues are worth blocking on, both from references to the old paths that the rename didn't catch. Details inline; summarizing the two big ones here since one of them is in a file this PR doesn't touch.
1. Driver version stamping is silently broken
go/adbc/pkg/Makefile:51 and c/cmake_modules/GoUtils.cmake:209 both still inject:
-X github.com/apache/arrow-adbc/go/adbc/driver/internal/driverbase.infoDriverVersion=...
but driverbase moved to go/driver/internal/driverbase. The Go linker silently ignores -X for symbols it can't resolve — no warning, no error. Verified by building ./flightsql/pkg with both symbol paths set:
v8.8.8-CORRECT <- new path, injected
(v9.9.9-STALE absent) <- old path, silently dropped
So driver.go:97's if infoDriverVersion != "" is now always false and ADBC_INFO_DRIVER_VERSION is never registered. Shipped Flight SQL libraries stop reporting their version via AdbcConnectionGetInfo.
GoUtils.cmake isn't in this PR's diff so I can't comment inline, but it's the path the CMake/release build actually uses — it needs the same fix.
2. pkg packages no longer build from the Go module cache
See the inline note on _tmpl/driver.go.tmpl. The cgo include now escapes the module boundary into the sibling go/adbc module, so it only resolves in a source checkout.
Also worth fixing
-
go/adbc/pkg/doc.gogo:generatedirectives are stale (file isn't in the diff, so no inline). The Makefileregeneratetarget was updated but these weren't://go:generate go run ./gen -prefix "FlightSQL" -driver ../driver/flightsql -o flightsql //go:generate go run ./gen -prefix "PanicDummy" -driver ../driver/panicdummy -o panicdummy
../driver/flightsqlno longer exists (it's../../driver/flightsql) and-oshould be../../driver/*/pkg. Probably the remaining half of the unchecked "Update regenerate command in Makefile" TODO. -
I couldn't run
make regenerateto confirm the checked-in generated files are in sync — noclang-formatavailable on my machine.
Question on the overall design
No replace / go.work — intentional? go/driver resolves go/adbc v1.12.0 from the module proxy, so in-tree changes to the core are never exercised by go/driver CI. A breaking core change merges green and surfaces only after release. post-04-go.sh clearly handles the tag ordering, so I assume this is deliberate — but a go.work would restore "CI tests the tree as it is" without changing published module metadata.
Not yours, but adjacent
TestDefaultDriver/TestCustomizedDriver (driverbase) and TestADBCFlightSQLWithHeader/TestMetadataGetInfo fail on this branch — DriverArrowVersion expects (unknown or development build), gets v18.8.0. I ran both against the merge base (86667c4d7) and they fail identically there, so this is pre-existing. Worth a separate issue, though note it's adjacent to finding #1.
Minor: the golangci-lint bump (v2.9.0 → v2.13.2) plus a new root .golangci.toml is unrelated churn — no golangci config existed before, so this changes lint behavior for go/adbc too.
| # than remembering the internal dependency structure of the go sources. | ||
| files_to_vendor <- list.files( | ||
| "../../go/adbc", | ||
| "../../go", |
There was a problem hiding this comment.
Vendoring widened from go/adbc to all of go/, but since go/driver resolves go/adbc from the proxy rather than from disk, the vendored src/go/adbc is now dead weight — shipped source that isn't what actually gets built.
Not breaking (the build already needs network for arrow-go etc.), but it does mean the source package no longer contains the core that the resulting binary is built against.
e05ddde to
9a71e7b
Compare
This way we don't leak CVEs/dependencies from the driver into the core ADBC package.
Breaking changes:
Closes #4623.