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

Rust port of mediasoup! #518

Merged
merged 278 commits into from
Mar 29, 2021
Merged

Rust port of mediasoup! #518

merged 278 commits into from
Mar 29, 2021

Conversation

nazar-pc
Copy link
Collaborator

What this massive PR includes:

  • Complete Rust port of the TypeScript mediasoup library
  • Port of the relevant tests from TypeScript
  • Some Rust-specific tests
  • Built-in documentation (as generally expected in Rust ecosystem)
  • Echo example app (server in Rust + client in TypeScript)
  • CI config that checks formatting with rustfmt, lints code with clippy and runs tests

There are minor cleanups in worker and existing tests, but nothing significant (let me know if that is an issue).
They are primarily related to stricter typing generally used in Rust.

Since there is #397 open and adding CI for Rust into existing setup would be challenging, I decided to use GitHub actions instead, hopefully that is OK.

Code style used is default for Rust ecosystem with no tweaks. For TypeScript code used in echo example (the only one so far) I used already established code style from this repository to make it consistent.

I acknowledge how big this is and would be happy to address any feedback there might be. Please start with documentation on docs.rs, it explains some high-level information about this port.

References:

As discussed with Iñaki, there will be some complementary activities related to this around website documentation and such.

… closing router on worker side, some trivial methods on Router, methods comments
Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Amazing. I must do a proper review and it will take some days before I finish. For now just a few comments:

  • I'd like to move current Travis-CI to GitHub CI entirely and not just for Rust.
  • I don't like the idea of including frontend examples in mediasoup/rust/ folder.
  • I don't understand why there is rust/.gitignore instead of just adding entries to main .gitignore.
  • Why is mediasoup rust version 0.5.0?

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Mar 1, 2021

I'd like to move current Travis-CI to GitHub CI entirely and not just for Rust.

Files for Rust and Node.js will most likely be different, so one doesn't depend on the other. I will try to find time for Node.js workflow in a separate PR. Here I just didn't want to submit Rust version without CI in place from the beginning.

I don't like the idea of including frontend examples in mediasoup/rust/ folder.

There is a convension in Rust (supported by Cargo) to have examples folder, so you can do cargo run --example echo.
So it felt natural to have frontend directory nearby.

There is no identical backend example in TypeScript so it seems fine to me to have it there.

I have no stong feelings about it if you want me to move it somewhere.
I can even move Rust examples and corresponding frontend to the root or maybe even to separate repo if you want (but that would be harder to maintain, cargo workspaces are great and everything is compiled together in CI).

I don't understand why there is rust/.gitignore instead of just adding entries to main .gitignore.

Moved .gitignore entries to the root in last commit

Why is mediasoup rust version 0.5.0?

This is less similar to TypeScript library and more similar to mediasoup-client case.
Rust version will have different things that will be considered a breaking change (for instance #503 broke dump in Rust because they are heavily typed there) and I don't think versioning will line-up perfectly.
And there will probably be more things that will break until it reaches the level of API stability of mediasoup itself.

I am open to suggestions how to make it clearer which version of the worker/mediasoup Rust version corresponds to (I'm also exploring how to turn worker into Rust crate to it can be installed as a dependency automatically).

Updated ticket with similarities/differences with version info.

@ibc
Copy link
Member

ibc commented Mar 3, 2021

I'd like to move current Travis-CI to GitHub CI entirely and not just for Rust.

Files for Rust and Node.js will most likely be different, so one doesn't depend on the other. I will try to find time for Node.js workflow in a separate PR. Here I just didn't want to submit Rust version without CI in place from the beginning.

Thanks. In fact we are having problems with Travis-CI due to its new limits.

I don't like the idea of including frontend examples in mediasoup/rust/ folder.

There is a convension in Rust (supported by Cargo) to have examples folder, so you can do cargo run --example echo.
So it felt natural to have frontend directory nearby.

Perfect then :)

Why is mediasoup rust version 0.5.0?

This is less similar to TypeScript library and more similar to mediasoup-client case.
Rust version will have different things that will be considered a breaking change (for instance #503 broke dump in Rust because they are heavily typed there) and I don't think versioning will line-up perfectly.
And there will probably be more things that will break until it reaches the level of API stability of mediasoup itself.

Ok, let's do this way for now and let's see in the future.

NOTE: I still have to review more things and won't be able to finish this week. Will do next one, promise.

@nazar-pc
Copy link
Collaborator Author

Tests are still a bit flaky at times, but I'll probably be ironing them out in next version instead of introducing more changes to this PR.

Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Please address just these two comments and let's merge.

@ibc
Copy link
Member

ibc commented Mar 29, 2021

waiting for tests to pass in CI

@ibc ibc merged commit 6b112a2 into versatica:v3 Mar 29, 2021
@nazar-pc nazar-pc deleted the rust branch April 4, 2021 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants