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: Add golang binding for OpenDAL #2488

Closed
wants to merge 1 commit into from
Closed

feat: Add golang binding for OpenDAL #2488

wants to merge 1 commit into from

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Jun 18, 2023

PLEASE DON'T MERGE.

A PoC of opendal golang binding just to show how bad a golang binding could be.

This PR is more like a question to both rust and golang community.

Can we make it better? Or is it simply because we didn't have a library like jni-rs to assist us in handling it? Can we avoid CGO entirely? How will the performance be compared to calling RPC?

What's we did in this PR?

We can link against with libopendal_c and calling it.


var OPENDAL_LIB uintptr

var opendal_operator_options_new func() uintptr
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @Ji-Xinyou, I will meet segmentation violation if I'm trying to pass the returning value of opendal_operator_options_new into opendal_operator_new. Maybe it's related to #2462?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can try to fix that now

@ImSingee
Copy link
Contributor

I think we can provide an interface instead of the struct.

This also provides an extra benefit: we can return a (maybe auto-generated) type which always return an error.

type Operator interface {
    DoSomething() error
}

type errOperator struct {}

func (o errOperator) DoSomething() error {
  return fmt.Errorf("not supported")
}

func NewOperator() Operator {
   if opendal not exist {
       return errOperator{}
   }

   return ...
}

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 13, 2023

Golang binding should not be implemented in this repo, we will start a new repo instead.

@Xuanwo Xuanwo closed this Jul 13, 2023
@Xuanwo Xuanwo deleted the go-binding branch July 13, 2023 17:19
@hwdef
Copy link

hwdef commented Sep 19, 2023

Any progress? Need help?

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 19, 2023

Any progress? Need help?

There has been no progress thus far. Additionally, due to the ASF limitation, we are unable to develop an opendal-go under this project (since we will need to build opendal as a binary in project like go-duckdb). Therefore, we should initiate it externally.

@hwdef
Copy link

hwdef commented Sep 19, 2023

Can I first confirm that go binding is indeed required? Because I see that even without go binding, there are not many related requirements

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 19, 2023

Can I first confirm that go binding is indeed required? Because I see that even without go binding, there are not many related requirements

No strong requirement yet.

@jiaoew1991
Copy link
Contributor

we need a go bindings too😭

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 21, 2023

we need a go bindings too😭

Even with CGO?

@jiaoew1991
Copy link
Contributor

we need a go bindings too😭

Even with CGO?

yes, it is acceptable. A unified API to access all kinds of Object store is very useful.

@jiaoew1991
Copy link
Contributor

Any progress? Need help?

There has been no progress thus far. Additionally, due to the ASF limitation, we are unable to develop an opendal-go under this project (since we will need to build opendal as a binary in project like go-duckdb). Therefore, we should initiate it externally.

May I ask why you need to split a separate repo? Is it because Go relies on commit IDs or tags for dependencies? If that's the case, we can refer to the Arrow project, where tags include paths to subdirectories. 🤔

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 27, 2023

May I ask why you need to split a separate repo?

ASF does not permit projects to include binaries such as libopendal.so or libopendal.a in the code. However, CGO may require them for linking purposes. Is there a method to construct libopendal using cgo?

@jiaoew1991
Copy link
Contributor

The kafka-go project provides dynamic compilation commands to find libraries through the pkg-config command. I think it is not necessary to provide binaries in the code. It would be sufficient to provide users with instructions on how to compile libopendal_c and generate the corresponding pkg-config file during the compilation process. 🤔

https://github.com/confluentinc/confluent-kafka-go/blob/dc61dda93a2546dbb3f16280b476f4c2c20b08a5/kafka/build_dynamic.go#L1-L10C62

@jiaoew1991
Copy link
Contributor

The kafka-go project provides dynamic compilation commands to find libraries through the pkg-config command. I think it is not necessary to provide binaries in the code. It would be sufficient to provide users with instructions on how to compile libopendal_c and generate the corresponding pkg-config file during the compilation process. 🤔

https://github.com/confluentinc/confluent-kafka-go/blob/dc61dda93a2546dbb3f16280b476f4c2c20b08a5/kafka/build_dynamic.go#L1-L10C62

I wrote a demo for this proposal #3204

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants