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

Fix for Rust 1.67.0 and latest Mumble APIs #1

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

citelao
Copy link

@citelao citelao commented Feb 6, 2023

Today, this crate does not compile. This change updates the crate to compile on modern Rust, plus adds some missing APIs and updates the README for clarity.

What changed?

  • Updated README with more complete build instructions.
  • Updated outdated dependencies.
  • Removed outdated compiler flags.
  • Updated libs to handle more modern bindgen output.
  • Updated enum generation to match previous names (after Mumble made changes to their naming).
  • Add missing mute APIs.

How tested?

  • Compiles my working plugin

TODO

  • Support 1.2 version
  • Support more idiomatic enum variant names.
  • Fully update README, including document known good Mumble versions
  • Add example, for easier testing

Appendix

Rust version info (via rustup show):

stable-x86_64-pc-windows-msvc (default)
rustc 1.67.0 (fc594f156 2023-01-24)

@Andicraft
Copy link

Please 🙏 Would love to see this updated.

@citelao
Copy link
Author

citelao commented May 10, 2024

@Andicraft, for now you can pull from my fork if you'd like to start working. I don't have any plans to continue contributing to the PR at this time, and I haven't heard anything from @Dessix.

I'm new to Rust, so I'd welcome any contributions to this PR.

@Dessix
Copy link
Owner

Dessix commented May 10, 2024

My apologies for the delay- It seems that submission of a draft PR doesn't actually trigger a notification to maintainers. @citelao as I've evidently fallen behind on this, I'd gladly offer maintainer permissions on the repository.

Copy link
Owner

@Dessix Dessix left a comment

Choose a reason for hiding this comment

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

The unchecked unwraps seem to kill the point of the Option change- I'd prefer they be at least standard unwraps unless the performance benefit is significant, and these FFI calls shouldn't be running in a very tight loop as it stands, so the impact should be negligible. Beyond that, this looks fine.

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

Successfully merging this pull request may close these issues.

3 participants