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(bindings/C): implement capability #3479

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Conversation

xyjixyjixyji
Copy link
Contributor

After implementing capability, the test framework for C is easier to implement. #3472

Two concerns along with this:

  • I think we need more documentions on the difference between full capabilities and native capabilities, the difference between them is not obvious to users. i.e. If they want to check the capabilities, should they use full/native?
  • Further work: we should replace all currently opendal_byte backed string like error message to leaked CString, which is more conventional for C. (This is my mistake at the very beginning 😢). I think this break the current API, so my plan would be changing this after our first release.

@Xuanwo
Copy link
Member

Xuanwo commented Nov 3, 2023

I think this break the current API, so my plan would be changing this after our first release.

Can you share more details about this decision? Why not change now instead?

@Xuanwo
Copy link
Member

Xuanwo commented Nov 4, 2023

  • If they want to check the capabilities, should they use full/native?
  • full tells users if they can do something.
  • native tells users if they can do something natively.

For example, s3 doesn't support blocking API natively, but they can do this after adding BlockingLayer.

I'm guessing it's ok for bindings just expose full as capabilities.

@xyjixyjixyji
Copy link
Contributor Author

I think this break the current API, so my plan would be changing this after our first release.

Can you share more details about this decision? Why not change now instead?

For example, now our error is backed by opendal_byte. Users typically use this by

opendal_byte *b = error->message;
for (int i = 0; i < b->len; ++I)
    printf("%s", b->data[i]);
opendal_byte_free(b)

But in the CString way, we are only giving a char*, user just do

char* msg = error->message:
printf(%s", msg);
free(msg);

This will break the current way of handling error, but it is more conventional

@Xuanwo
Copy link
Member

Xuanwo commented Nov 4, 2023

For example, now our error is backed by opendal_byte. Users typically use this by

You have the authority to make such changes as the committer of OpenDAL and code owner of OpenDAL C binding. You're empowered to suggest API changes when necessary.

The question I asked is about the timing: How about making those changes now before release, instead of after release?

@xyjixyjixyji
Copy link
Contributor Author

You have the authority to make such changes as the committer of OpenDAL and code owner of OpenDAL C binding. You're empowered to suggest API changes when necessary.

The question I asked is about the timing: How about making those changes now before release, instead of after release?

Project like milvus is using c binding right now. Are they manually linking the library or building from source? I am not really sure about this.

My original thought is that they are building from source. If this is the case, changing the upstream code might break their code that is using stuff like error.

But if they just use the static library and previous header and manually link to that, changing it now will be fine.

Does that make sense to you?

@Xuanwo
Copy link
Member

Xuanwo commented Nov 5, 2023

Project like milvus is using c binding right now. Are they manually linking the library or building from source? I am not really sure about this.

My original thought is that they are building from source. If this is the case, changing the upstream code might break their code that is using stuff like error.

Your concern is correct. milvus is using OpenDAL via clone it directly:

# git clone https://github.com/jiaoew1991/opendal.git opendal
git clone https://github.com/apache/incubator-opendal.git opendal
cd opendal
# git checkout blocking-layer
if command -v cargo >/dev/null 2>&1; then
    echo "cargo exists"
else
    bash -c "curl https://sh.rustup.rs -sSf | sh -s -- -y" || { echo 'rustup install failed'; exit 1;}
    source $HOME/.cargo/env
fi
pushd bindings/c
cargo build || { echo 'opendal_c build failed'; exit 1; }
popd
mkdir -p ${ROOT_DIR}/internal/core/output/lib
mkdir -p ${ROOT_DIR}/internal/core/output/include
cp target/debug/libopendal_c.a ${ROOT_DIR}/internal/core/output/lib/libopendal_c.a
cp bindings/c/include/opendal.h ${ROOT_DIR}/internal/core/output/include/opendal.h

From: https://github.com/milvus-io/milvus/blob/bf2f62c1e7231bb2f7f3f27c5807439eb63b6918/scripts/3rdparty_build.sh#L66-L82

It's by design to allow break on main branch and users should not rely on our main branch. Perhaps we should transition to commit-based, then shift to tag-based after formally releasing opendal-c? @jiaoew1991

But if they just use the static library and previous header and manually link to that, changing it now will be fine.

I'm worried that we're still a long way from releasing our first version of opendal-c. We may need to implement more significant changes for opendal-c. We can't simply delay them all and wait for the next version.


Conclusion:

  • Users should depend on tag if we are released.
  • Users should depend on commit if we aren't released.
  • OpenDAL's main branch is by design to break APIs from time to time.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.

bindings/c/Makefile Show resolved Hide resolved
@xyjixyjixyji
Copy link
Contributor Author

I agree with what @Xuanwo mentioned and prefer to make the string change before the release. (But I think we should do it in a separate PR, making it easier to review).

As for the test framework thing, I am gonna look into that later as well.

\cc @jiaoew1991, can you update the code in milvus to use the version by commit? Cuz we might break some APIs in the break.

@Xuanwo
Copy link
Member

Xuanwo commented Nov 5, 2023

But I think we should do it in a separate PR, making it easier to review

👍, Sure!

@jiaoew1991
Copy link
Contributor

I agree with what @Xuanwo mentioned and prefer to make the string change before the release. (But I think we should do it in a separate PR, making it easier to review).

As for the test framework thing, I am gonna look into that later as well.

\cc @jiaoew1991, can you update the code in milvus to use the version by commit? Cuz we might break some APIs in the break.

I will choose a stable commit 😅

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.

3 participants