feat(c,ci,go): add vcpkg-based build + ci, fix go imports#3591
Open
amoeba wants to merge 14 commits intoapache:mainfrom
Open
feat(c,ci,go): add vcpkg-based build + ci, fix go imports#3591amoeba wants to merge 14 commits intoapache:mainfrom
amoeba wants to merge 14 commits intoapache:mainfrom
Conversation
Co-Authored-By: Alamiri, Ali <Ali.Alamiri@sage.com>
Member
Author
|
Here's my local test output for adbc-driver-manager-test: adbc-driver-manager-test |
lidavidm
reviewed
Oct 20, 2025
lidavidm
reviewed
Oct 20, 2025
| endif() | ||
|
|
||
| # On Windows, install sqlite3.dll alongside the driver | ||
| if(WIN32 AND CMAKE_VERSION VERSION_GREATER_EQUAL "3.21") |
Member
There was a problem hiding this comment.
Why does this need a cmake version check?
Member
Author
There was a problem hiding this comment.
This whole block was mainly to copy runtime deps, I came up with using RUNTIME_DEPENDENCY_SET like this with LLM help and RUNTIME_DEPENDENCY_SET is 3.21, see https://cmake.org/cmake/help/latest/command/install.html. I tried something similar before this and didn't get something working.
Member
Author
There was a problem hiding this comment.
There's probably a much better way to do this, I can keep working on this part.
Member
There was a problem hiding this comment.
I guess then the problem is it won't work on older CMake!
Our minimum is 3.18 right now but maybe we can bump it up?
Member
Author
There was a problem hiding this comment.
Yeah I'll try to come up with something simpler in this PR. It boils down to copying one DLL in the case of sqlite and three in the case of postgresql.
amoeba
commented
Oct 20, 2025
| list(APPEND GO_ENV_VARS "GOARCH=arm64") | ||
| endif() | ||
|
|
||
| add_custom_command(OUTPUT "${LIBOUT_SHARED}.${ADBC_FULL_SO_VERSION}" |
Member
Author
There was a problem hiding this comment.
Reviewer note: The rest of this file I used a combination of Claude Code and the previous PR and comments therein to come up with something that worked. I want to go back over this tomorrow with fresh eyes but I thought I'd point it out in case it helps review.
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
2 participants
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.
I started adding a vcpkg preset for Windows and then saw #2858 which was most of the way there. I wanted to polish things up a bit so I'm opening a new PR and crediting the previous PR author as a co-author since I used parts of their original PR.
This PR includes:
Closes #2846
Ref #2557