ci: Add pipeline support to bundle Go binaries in NuGet packages #1730
Conversation
davidhcoe
requested review from
lidavidm,
zeroshade and
CurtHagenlocher
as code owners
csharp/src/Drivers/Interop/Snowflake/Apache.Arrow.Adbc.Drivers.Interop.Snowflake.csproj
Outdated
Show resolved
Hide resolved
| </Content> | ||
|
|
||
| <!-- Mac/dylib --> | ||
| <Content Include="libadbc_driver_snowflake.dylib"> |
There was a problem hiding this comment.
Are Go-built binaries "fat" or would we need separate binaries for x64 and arm64?
There was a problem hiding this comment.
You would need separate binaries for x64 and arm64. You can control the build by using GOARCH=arm64 or GOARCH=amd64 if you want to cross compile
lidavidm
changed the title
CurtHagenlocher
changed the title
csharp/src/Drivers/Interop/Snowflake/Apache.Arrow.Adbc.Drivers.Interop.Snowflake.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks like a significant improvement over what we have currently so I wouldn't want to block it from being checked-in (other than the osx-x64 thing). I wouldn't mind another set of eyes on the YAML, but if I ignore the deleted and commented-out stuff what remains looks fine to me.
It would be nice to add support for osx-arm64 -- not just because I have an M1 Mac but also because I suspect they're starting to be the rule rather than the exception (at least in the developer community). But that could also be done as a separate change.
|
One problem seems to be that the conditional checks are needed in the .csproj to not fail the C# tests, but then they skip the packaging the files in the NuGet package. Still working on it. |
…pache#1725) Hey! As discussed in apache#1723, here is the first PR of the new complete Rust implementation. It includes: - Traits and structs definitions for the public abstract API - An implementation of a dummy driver for testing purposes (no test currently because we need the driver manager and exporter to have relevant tests) - Constants from `adbc.h` I put the code in `/rust2` but I can move it to `/rust` if you prefer. CC @mbrobbel @paleolimbot @zeroshade @lidavidm
…1741) Doesn't add anything meaningfully new, just adds tests for types. Fixes apache#933.
Fixes apache#1743. We use PostgreSQL instead of SQLite for testing. Because the SQLite driver doesn't support GetStatistics yet.
Fixes apache#1747. This is the binding of `AdbcConnectionGetStatisticNames()`.
…adbc (apache#1746) Bumps [github.com/apache/arrow/go/v16](https://github.com/apache/arrow) from 16.0.0-20240401151559-71321841eb6d to 16.0.0. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: David Li <[email protected]>
Update Rust workflow to only use Apache allowed actions. This also adds `Cargo.lock` (used in cache key) to the Rust workspace following [this recommendation](https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html), and fixes some doc warnings.
As suggested in apache#1749 (comment).
Hey! Here is the second PR of the Rust implementation containing full FFI bindings. I've decided to manually implement bindings instead of relying on [bindgen](https://rust-lang.github.io/rust-bindgen/) because: - We don't want an additional dependency on clang (which bindgen depends on) and so we cannot generate bindings at compile-time - We could have checked-in the generated bindings in git but in that case manually editing is unavoidable because generated bindings are platform-dependent - Besides, we still have to implement default methods, conversion traits and release mechanisms. In the end, I found it simpler to have everything manually implemented rather than having some parts automatic and some others manual... [arrow-rs](https://github.com/apache/arrow-rs) seems to have followed the same path.
Fixes apache#1753. It's a wrapper of `GADBCConnection` for Apache Arrow C GLib integration.
…hcoe/arrow-adbc into dev/1720-GoDriversInNuGetFix
|
@lidavidm, as this is now passing ( and without any commented-out or deleted workflow tasks ;) ), I'd like to sign off on it -- but I'd feel a lot more comfortable if you could take a quick look too. |
|
Will do! |
| popd | ||
|
|
||
| - name: Upload Go binaries | ||
| uses: actions/upload-artifact@v3 |
There was a problem hiding this comment.
| uses: actions/upload-artifact@v3 | |
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
I changed them to v4 but it was breaking and not uploading the files correctly, so I went back to v3.
| @@ -146,10 +197,22 @@ jobs: | |||
| echo "schedule: ${{ github.event.schedule }}" >> $GITHUB_STEP_SUMMARY | |||
| echo "ref: ${{ github.ref }}" >> $GITHUB_STEP_SUMMARY | |||
|
|
|||
| - uses: actions/download-artifact@v3 | |||
There was a problem hiding this comment.
| - uses: actions/download-artifact@v3 | |
| - uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
I changed them to v4 but it was breaking and not downloading the files correctly, so I went back to v3.
|
Given what @davidhcoe says about switching to v4, I'm going to commit as-is. |
…che#1730) Adds support to the packages pipeline for building the Go binaries before the csharp pipeline. The output of these is that the Snowflake NuGet package is just an interop wrapper over the libraries, which should be included with the NuGet package. Fixes apache#1720 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: David Coe <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: David Li <[email protected]> Co-authored-by: Alexandre Crayssac <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Matthijs Brobbel <[email protected]>
Adds support to the packages pipeline for building the Go binaries before the csharp pipeline. The output of these is that the Snowflake NuGet package is just an interop wrapper over the libraries, which should be included with the NuGet package.
Fixes #1720