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

Want to make futures's compact feature used by fbthrift-git-rs optional #12

Open
GG2002 opened this issue Sep 20, 2024 · 12 comments
Open

Comments

@GG2002
Copy link

GG2002 commented Sep 20, 2024

Hello, I am creating a new client for NebulaGraph, so I have reused some of the code from nebula-rs. Thank you in advance for your work.

I couldn't find the issue section in the fbthrift-git-rs repo, so that's why I'm speaking here. I found that fbthrift-git depends on futures v0.1.31 (a fairly old version). After investigation, I found that it is because fbthrift-git has enabled the compat feature of futures, which fixes futures on this old version and adds this constraint to all crates that use the NebulaGraph client (in fact, I am trying to add NebulaGraph support to OpenDAL, so this will cause all futures dependencies of OpenDAL services to become v0.1.31).

I'm not quite sure why fbthrift-git needs to enable the compat feature of futures. If possible, could this feature be made optional or removed directly?

@wey-gu
Copy link

wey-gu commented Sep 20, 2024

Dear @vkill ,

I hugely respect and am grateful for your amazing efforts in rustifying NebulaGraph!

Thanks a lot for the support :-D

love from NebulaGraph community

@wey-gu
Copy link

wey-gu commented Sep 20, 2024

Dear @vkill , I wonder if it is possible to grant contributors from the OpenDAL and NebulaGraph community as maintainers of some of your repos, which would be smoother for further collaborations and maintenance. What do you think, please?

If that's feasible, @GG2002 would be the best candidate for the maintainer.

BR//Wey

@vkill
Copy link
Contributor

vkill commented Sep 25, 2024

@GG2002 I will deal with this issue of futures

@vkill
Copy link
Contributor

vkill commented Sep 25, 2024

@wey-gu PRs are welcome :)

@vkill
Copy link
Contributor

vkill commented Sep 25, 2024

Hi @GG2002
The feature compat is also enabled in the latest official version. I don't think there is any problem with it.

https://github.com/facebook/fbthrift/blob/v2024.09.23.00/thrift/lib/rust/Cargo.toml#L18

@vkill
Copy link
Contributor

vkill commented Sep 25, 2024

I found that fbthrift-git depends on futures v0.1.31 (a fairly old version).
Hi @GG2002 It cannot be v.0.1.31

futures = { version = "0.3.30", features = ["async-await", "compat"] }

@GG2002
Copy link
Author

GG2002 commented Sep 25, 2024

Thank you very much for your prompt reply.

I found that fbthrift-git depends on futures v0.1.31 (a fairly old version).
Hi @GG2002 It cannot be v.0.1.31

futures = { version = "0.3.30", features = ["async-await", "compat"] }

Yes, futures here are the latest version, but enabling compat will cause its dependent futures-util also to enable compat, and then futures-utils will depend on v0.1.31.

Hi @GG2002 The feature compat is also enabled in the latest official version. I don't think there is any problem with it.

https://github.com/facebook/fbthrift/blob/v2024.09.23.00/thrift/lib/rust/Cargo.toml#L18

I'm sorry, I didn't notice this before, it's indeed a problem. I need to communicate with @Xuanwo again

@Xuanwo
Copy link

Xuanwo commented Sep 25, 2024

Hi, compat is used to facilitate compatibility between futures 0.1 and futures 0.3. This is unnecessary since the entire community now relies on futures 0.3 instead. Adding the compat feature only introduces new dependencies for futures 0.1 without any benefits.

image

@vkill
Copy link
Contributor

vkill commented Sep 25, 2024

Hi @Xuanwo I think you can submit a PR to https://github.com/facebook/fbthrift

@vkill
Copy link
Contributor

vkill commented Sep 25, 2024

Yes, futures here are the latest version, but enabling compat will cause its dependent futures-util also to enable compat, and then futures-utils will depend on v0.1.31.

Hi @GG2002 I don't think there's anything wrong with this. Will it cause dependency incompatibility?
At least I didn't find any problems while using it.

@Xuanwo
Copy link

Xuanwo commented Sep 25, 2024

Hi @GG2002 I don't think there's anything wrong with this. Will it cause dependency incompatibility?

Hi, this will add an extra futures 0.1 dependency for all projects that use our library like this one: apache/opendal#5116 (comment)

@GG2002
Copy link
Author

GG2002 commented Oct 5, 2024

Yes, futures here are the latest version, but enabling compat will cause its dependent futures-util also to enable compat, and then futures-utils will depend on v0.1.31.

Hi @GG2002 I don't think there's anything wrong with this. Will it cause dependency incompatibility? At least I didn't find any problems while using it.

The compat feature of futures is used by futures v0.3 to interact with futures v0.1. I haven't found a place in the current Rust implementation where this feature is needed (or maybe I didn't read it carefully enough, and I'm worried about it).

Maybe I can remove it with a few PRs. I simply removed the compat in Cargo.toml, including those in the root directory and src/dep_tests, and then cargo build and cargo test had no problem.

If this plan is feasible, then there is one more question. I found that nebula-fbthrift-storage-v3, nebula-fbthrift-graph-v3, nebula-fbthrift-meta-v3 and nebula-fbthrift-common-v3 depend on fbthrift-git v0.0.7. I couldn't find this branch in the fbthrift-git repository, and changing their dependency directly to the current v0.0.8 would result in many errors.

I have no idea about this, can you provide the code for v0.0.7? Or can we update the dependencies of these libraries to v0.0.8 together and just remove compat?

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

No branches or pull requests

4 participants