- 
                Notifications
    
You must be signed in to change notification settings  - Fork 161
 
feat(c,ci,go): add vcpkg-based build + ci, fix go imports #3591
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
f885eba
              e74127e
              b07e5ae
              5fff1b6
              7da4cc6
              51e8553
              679be7d
              4430bfe
              490355d
              cbd9d94
              057af6f
              27db1e6
              1fe362c
              b0b9dbd
              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 | 
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| 
     | 
||
| vcpkg_installed/ | 
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -68,6 +68,20 @@ foreach(LIB_TARGET ${ADBC_LIBRARIES}) | |
| if(NOT ADBC_DEFINE_COMMON_ENTRYPOINTS) | ||
| target_compile_definitions(${LIB_TARGET} PRIVATE ${ADBC_TARGET_COMPILE_DEFINITIONS}) | ||
| endif() | ||
| 
     | 
||
| # On Windows, install sqlite3.dll alongside the driver | ||
| if(WIN32 AND CMAKE_VERSION VERSION_GREATER_EQUAL "3.21") | ||
| 
         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. Why does this need a cmake version check? 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. 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. 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. There's probably a much better way to do this, I can keep working on this part. 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. 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? 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. 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.  | 
||
| install(RUNTIME_DEPENDENCY_SET | ||
| ${LIB_TARGET}_runtime_deps | ||
| PRE_EXCLUDE_REGEXES | ||
| "api-ms-" | ||
| "ext-ms-" | ||
| POST_EXCLUDE_REGEXES | ||
| ".*system32/.*\\.dll" | ||
| DESTINATION | ||
| ${RUNTIME_INSTALL_DIR}) | ||
| install(TARGETS ${LIB_TARGET} RUNTIME_DEPENDENCY_SET ${LIB_TARGET}_runtime_deps) | ||
| endif() | ||
| endforeach() | ||
| 
     | 
||
| include(CheckTypeSize) | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "adbc-driver-sqlite", | ||
| "version-string": "1.0.0a0", | ||
| "dependencies": [ | ||
| "sqlite3" | ||
| ] | ||
| } | 
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.
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.