-
Notifications
You must be signed in to change notification settings - Fork 330
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
Add [email protected] #2523
base: main
Are you sure you want to change the base?
Add [email protected] #2523
Conversation
Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (go-duckdb) have been updated in this PR. Please review the changes. |
0e88a8c
to
db05009
Compare
|
||
go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps") | ||
go_deps.from_file(go_mod = "//:go.mod") | ||
use_repo( |
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.
just a note that this use_repo
clause is redundant as it's not importing any repos. no need to fix it in this PR but could fix upstream.
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.
did you mean to include this file? there's no arrow/cdata/BUILD.bazel
file in https://github.com/marcboeker/go-duckdb
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.
Yes, it's a transitive dependency of go-duckdb
. This is what it looks like in our deps.bzl file.
go_repository(
name = "com_github_apache_arrow_go_v14",
build_file_proto_mode = "disable_global",
importpath = "github.com/apache/arrow/go/v14",
patch_args = ["-p1"],
patches = ["@//bazel:patches/com_github_apache_arrow_go_v14_arrow_cdata.patch"],
sum = "h1:N8OkaJEOfI3mEZt07BIkvo4sC6XDbL+48MBPWO5IONw=",
version = "v14.0.2",
)
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.
Ah, I see. You need to translate this go_repository
call to an appropriate tag on the go_deps
module extension (probably see its docs for pointers). The patch file shouldn't be included in source.json as it doesn't apply to this repo itself.
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.
I'm out of my depth here. I didn't write the patch, we engaged @pcj to get the fix running internally, and I opened this to try and make it publicly available. I can follow some explicit instructions, but that's about it.
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.
@derekperkins You would have to turn github.com/apache/arrow/go/v14 into a Bazel module in the same way as go-duckdb
first and then add it to go-duckdb
as a bazel_dep
. Alternatively, we could try to make Gazelle smarter so that the patch on arrow isn't needed anymore.
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.
I'll probably just wait for that gazelle support to land. Not sure if you'd prefer that this PR is closed in the meantime, but I'm open to whatever
@@ -0,0 +1,10 @@ | |||
{ | |||
"url": "https://proxy.golang.org/github.com/marcboeker/go-duckdb/@v/v1.7.0.zip", |
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.
The Go proxy doesn't promise a stable hash, but neither does GitHub for its /archive
endpoint, so we might as well go with this.
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.
I just copied what was in the circl module. TBH, I didn't even realize the go proxy provided a download option like this
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.
@derekperkins You would have to turn github.com/apache/arrow/go/v14 into a Bazel module in the same way as go-duckdb
first and then add it to go-duckdb
as a bazel_dep
. Alternatively, we could try to make Gazelle smarter so that the patch on arrow isn't needed anymore.
I just copy/pasted and tried to mimic cloudflare/circl, without really having an understanding of the system, so feel free to make whatever suggestions you want.
Fixes #2517