-
Notifications
You must be signed in to change notification settings - Fork 481
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(bindings/go): Add full native support from C to Go. #4886
Conversation
22fb917
to
db32f24
Compare
Bindings Go CI failed because the Go version is lower than 1.22.4 (1.20). And considering 1.20 is out of maintenance, I think it is time to update to the latest version.
IMO we need to update the CI configuration because it is tailored for CGO. Here is an example from my POC. It is really simple and easy. |
Great, welcome to bump the go version in a new PR. |
go test -bench=. -tags dynamic . | ||
```bash | ||
# Run all tests | ||
CGO_ENABLE=0 go test -v -run TestBehavior |
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.
CGO_ENABLE=0
!
This PR is outstanding overall! Excellent implementation, well-documented, and comprehensive tests (fully integrated testing). This is the best PR I've encountered this year! Let's first address the CI and merge it into the main branch, then we can refine the API naming and other details. |
bindings/go/go.mod
Outdated
@@ -17,12 +17,22 @@ | |||
|
|||
module opendal.apache.org/go |
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.
Let's use github.com/apache/opendal/bindings/go
so users can use go binding from git now.
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 not sure whether go support get sub directory.
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.
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.
It doesn't work since we are now pointintg to opendal.apache.org/go
. After we change our mod to github.com/apache/opendal/bindings/go
. It should work.
8242328
to
9724a68
Compare
Ok, seems the ci changes need to go along with this PR. |
Here's my personal thought. The original Go binding is a full-function and cross-platform binding. The new Go binding has a good tech design and the ability to extend the function. But for now, I think we need a lot of time to solve some issues, such as binary release and distribution and cross-platform test(for now, I believe we specialize in Linux), keep the service we support is same with the old Go bindings. So I think maybe we can make this feature as experimental and we add a new directory |
Yes. I closed it. |
I'm ok with that. But I don't think the old version is full functional. Seems it only implements read and write. |
New Go Binding! |
Signed-off-by: Hanchin Hsieh <me@yuchanns.xyz>
9724a68
to
7b830b0
Compare
Benchmark is in absence. I should add one Later. |
Hi, @Zheaoli, Thanks a lot for raising your concern.
This is incorrect. In fact, it cannot even be used with
This is true. I have a feeling that we will finally reintroduce CGO support for other platforms. It would be great if someone could take on the task of making the Go bindings cross-platform and fully functional. However, if no one is interested, I'm satisfied with the Go bindings working on Linux platforms for now.
The old Go binding has never been usable, published, or maintained. The most recent change to the Go binding was four months ago, with the latest meaningful update occurring on October 21, 2023. I prefer not to keep the old go binding unless someone steps up to help maintain 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.
Mostly LGTM, perfect PR!
Hi, @Zheaoli, would you like to leave a final comment? |
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.
LGTM
PR for #4848.
Let's rock!
I'm so excited that I can't help but complete it all at once. If this is too big to review, I can split it into smaller chunks.
writer
,read_with
, eg.NOTE: this PR contains two experimental services
memory
andaliyun-drive
to show how it works for behavior test purposes. Officially distributions need to be managed by @Xuanwo.