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

Requirements for updating this repo #38

Closed
kw217 opened this issue Feb 23, 2022 · 4 comments
Closed

Requirements for updating this repo #38

kw217 opened this issue Feb 23, 2022 · 4 comments

Comments

@kw217
Copy link
Collaborator

kw217 commented Feb 23, 2022

@alexeiakimov asked in Metaswitch/cassandra-rs#111 about what the requirements are for updating this repo.

  • Obviously the usual https://github.com/Metaswitch/cassandra-sys-rs/blob/master/CONTRIBUTING.md guidance applies.
  • Please use cargo fmt to format the code. If it's not already properly formatted, please put the reformatting into a separate commit so it is easier to review; don't combine it with actual changes.
  • There are no tests in this repo, but there are examples which at least use most features. If it makes sense, when you're adding a feature please add to the examples.
  • Please do ensure you carry your changes through to https://github.com/Metaswitch/cassandra-rs and add tests there (if that makes sense).
  • There's an example at Update cassandra driver #18. @alexeiakimov accurately summarised the steps as:
    • update cassandra.h from the latest driver
    • regenerate the binding code with bindgen utility
    • update the changelog
    • update the CI files to download the correct driver library
  • In general: I know this code isn't as nice as it could be, but please ensure new code does things better, and if you have an opportunity to clean up old code then please do so.

One important note though:

  • This repo needs to move to use GitHub Actions rather than Travis; this has already been done for cassandra-rs.

Thanks!

@alexeiakimov
Copy link
Contributor

Thank you @kw217 for the detailed explanation. It seems that there is a subtle problem here - the version of bindgen utility. Since it is not specified, the new Rust code can be incompatible with the old one. In particular

  • in times of Cassandra driver 2.10 the bindgen mapped size_t to usize
  • later after size_t vs usize rust-lang/rust-bindgen#1671 bindgen start to map size_t to some other type depending on the target platform. On my machine it is u64
  • it means that some functions from public API will have different signature, for example get_future_error_message
  • formally the new generated code is not backward compatible, although the original C++ drivers are backward compatible

The old behavior can be restored by using the --size_t-is-usize option with bindgen. @kw217 do you think it is necessary to preserve the original size_t -> usize binding, otherwise a new major version of the library should be released? Thank you.

@kw217
Copy link
Collaborator Author

kw217 commented Mar 4, 2022

Thanks for the analysis. I think sticking with usize is the best approach. It will require a new major version anyway (since clients are forced to install a new libcassandra), but I'd prefer not to force them to make any other changes.

@kw217
Copy link
Collaborator Author

kw217 commented Mar 4, 2022

FTR I've updated CONTRIBUTING.md to include my notes from above.

@kw217
Copy link
Collaborator Author

kw217 commented Mar 7, 2022

Closing - #39 has upgraded the driver, and #41 has added the notes above to the docs.

@kw217 kw217 closed this as completed Mar 7, 2022
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

2 participants