Windows arm64, a checked ABI assumption, and a fuller CI matrix - #37
Closed
calvinchengx wants to merge 6 commits into
Closed
calvinchengx wants to merge 6 commits into
calvinchengx wants to merge 6 commits into
Conversation
This was referenced Aug 23, 2026
Contributor
Author
|
Added one commit since opening: Separately, I opened #38 with two Makefile fixes that are independent of this PR ( |
Contributor
Author
|
Already on main. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Windows was already supported and CI-tested here; what was missing was arm64, and a check on the assumption the workaround rests on. This adds both, widens the CI matrix to the platforms
COMPATIBILITY.mdclaims, and makes the musl case real.Every job below is green on my fork: calvinchengx#1
The ABI assumption is now stated and checked
register_result_windows.goregistersduckdb_fetch_chunkandduckdb_result_return_typewith a pointer argument because a 48-byte aggregate is passed indirectly. That bet was correct but implicit, and ifduckdb_resultwere ever reduced below the threshold the registration would read the wrong memory silently.abi_windows_amd64.goandabi_windows_arm64.go.One deliberate behaviour change worth your judgement: 32-bit Windows (
386,arm) now fails to build, with an explanatory type inabi_windows_unsupported.go. Those conventions push large aggregates onto the stack by value, so the workaround is wrong there rather than merely untested — and DuckDB publishes no 32-bit Windows library. Happy to drop this commit if you would rather leave those targets compiling.windows/arm64
No code change was needed beyond the threshold constant — the existing
_windowsfile already applies to both. It is now exercised onwindows-11-armand passes.CI matrix
macos-13has been retired by GitHub, so macOS amd64 was silently uncovered. The matrix is now linux amd64+arm64, macOS arm64 (macos-latest) and amd64 (macos-15-intel), and Windows amd64+arm64 — with the DuckDB asset chosen per target rather than per OS.musl
The Dockerfile said "Official DuckDB builds require glibc. Not musl." That is no longer true — v1.5.4 publishes
libduckdb-linux-{amd64,arm64}-musl.zip. There is now aDockerfile.musland a second integration job, and it passes.It surfaced one thing worth knowing:
libduckdb.solinks against libstdc++, which an Alpine base does not carry, so the image needsapk add libstdc++. A glibc-only CI would never have shown that, and an Alpine user would have hit it as "failed to load DuckDB library from any standard location".DLL discovery
Two changes:
LoadLibraryExWwithLOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS. That is safer than the legacy search, which reaches the current directory andPATH, and it lets a DuckDB DLL resolve dependencies sitting beside it. A bare name still goes through the standard search includingPATH, so anyone relying on that — including this repo's own CI — is unaffected. No new dependency:LoadLibraryExWis reached throughsyscall.NewLazyDLL, keeping #270 satisfied.Not addressed
When the library fails to load,
driver_test.gocontinues against a nil handle and panics on a nil pointer dereference rather than failing cleanly. I left it alone as unrelated to this change, but it makes a missing-DLL diagnosis noisier than it needs to be.