Skip to content
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

feat(go): add basic driver logging #1048

Merged
merged 1 commit into from
Sep 12, 2023
Merged

feat(go): add basic driver logging #1048

merged 1 commit into from
Sep 12, 2023

Conversation

lidavidm
Copy link
Member

Fixes #492.

@lidavidm lidavidm added this to the ADBC Libraries 0.7.0 milestone Sep 11, 2023
@lidavidm lidavidm requested a review from zeroshade as a code owner September 11, 2023 14:38
@lidavidm
Copy link
Member Author

Bah, the experimental slog requires Go 1.20?

@lidavidm
Copy link
Member Author

@zeroshade how do you feel about bumping us to Go 1.20 or 1.21? I think the main problem will be Linux packaging jobs where we won't be able to use the system golang...

@lidavidm
Copy link
Member Author

Either that, or we can do build tag shenanigans to only enable this on Go 1.20?

@lidavidm
Copy link
Member Author

It looks like Arrow itself was bumped to Go 1.20: https://github.com/apache/arrow/blob/main/go/go.mod

@zeroshade
Copy link
Member

Arrow currently maintains compatibility with go1.17-go1.20. But as of apache/arrow#37637 we're dropping explicit compatibility with go1.17-go1.18. We're still keeping compatibility with go1.19 though, using build tags.

What we could do here is put together a small interface and then use build tags to use https://pkg.go.dev/golang.org/x/exp/slog for go1.19 and slog directly for go1.20+ similar to how we handle json in Arrow for tinygo vs non-tinygo

@lidavidm
Copy link
Member Author

It looks like slog works on 1.19 too so I'll see about bumping to 1.19 here

@lidavidm
Copy link
Member Author

Once I get this working I'll split out the version bump into a separate PR so that it's clear

@@ -17,7 +17,7 @@

tmp_dir <- "src/go/tmp"

go_version <- Sys.getenv("R_ADBC_GO_VERSION_DOWNLOAD", "1.18.10")
go_version <- Sys.getenv("R_ADBC_GO_VERSION_DOWNLOAD", "1.19.13")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a canonical place in the repository where this value could be scraped? I am pretty sure I can make this automatic so that you don't have to do the version bump every time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the .env but it's not in sync with everything else anyways (I guess it only affects docker-compose)

@lidavidm
Copy link
Member Author

hmm, ubuntu-jammy fails because we install golang-1.20 but the package depends on golang. Let's see if we can finagle that...

@lidavidm
Copy link
Member Author

Windows verification is failing since libprotobuf now has a runtime dependency on gtest, and so when our verification script tries to remove it (since we want to use bundled gtest on windows) it also clobbers half our conda environment

conda-forge/libprotobuf-feedstock#186

@lidavidm
Copy link
Member Author

@kou do you know how to install golang-1.20 on Ubuntu Jammy and then have the APT package build be OK with having that installed in place of golang?

I also tried installing both golang and golang-1.20, but then go 1.18 ends up first on PATH and I couldn't get update-alternatives to work with go.

@lidavidm
Copy link
Member Author

lidavidm commented Sep 11, 2023

Hmm, let's see if alternative dependencies are the right thing to do...

@lidavidm
Copy link
Member Author

Alright, now the problem is that the golang-1.20 package doesn't add itself to PATH.

@zeroshade
Copy link
Member

@lidavidm you should be able to use this PPA to install golang-1.20

sudo add-apt-repository ppa:longsleep/golang-backports
sudo apt update
sudo apt install golang-go

@lidavidm
Copy link
Member Author

I don't think we want to ship binaries that depend on a PPA (and Go 1.20 is in the repos, just have to finagle the configuration)

@zeroshade
Copy link
Member

I didn't know there was an official package for go1.20 in the jammy repos

@lidavidm
Copy link
Member Author

@lidavidm
Copy link
Member Author

OK, so long as the apt changes look OK, I'll split the Go version bump into a new PR, then come back here once that's merged

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for APT related changes.

ci/linux-packages/debian/rules Outdated Show resolved Hide resolved
@lidavidm
Copy link
Member Author

Thanks Kou! Split into #1053

@lidavidm lidavidm merged commit 48eb1dd into apache:main Sep 12, 2023
@lidavidm lidavidm deleted the gh-492 branch September 12, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go/adbc/driver/flightsql: add extra logging
4 participants