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

refactor: make hex_as_bytes DRY #1409

Merged
merged 12 commits into from
Jun 26, 2024
Merged

Conversation

dhilipsiva
Copy link
Contributor

@dhilipsiva dhilipsiva commented Jun 18, 2024

  • Modify hex_as_bytes function to return an array of bytes based on input length
  • Remove mutable output parameter in hex_as_bytes function
  • Update usage of hex_as_bytes function to use generic type and constant size

- Modify `hex_as_bytes` function to return an array of bytes based on input length
- Remove mutable `output` parameter in `hex_as_bytes` function
- Update usage of `hex_as_bytes` function to use generic type and constant size
- Simplify the conversion of `DtlsFingerprint` to `web_rtc_transport::Fingerprint`
- Simplify the conversion of `web_rtc_transport::Fingerprint` to `DtlsFingerprint`
@ibc
Copy link
Member

ibc commented Jun 18, 2024

Please run cargo fmt, cargo clippy and cargo test and commit changes. CI is failing.

@nazar-pc
Copy link
Collaborator

Just a note: this will bump required Rust compiler version to one that supports const generics (which is relatively old at this point, but still)

@dhilipsiva
Copy link
Contributor Author

@ibc I ran all of those 3 commands before pushing the code. cargo fmt did not report any changes. All tests are passing. clippy does report a few things, but it is not from the code that I touched, it was already there. Should I be fixing those as well?

@ibc
Copy link
Member

ibc commented Jun 20, 2024

Looks like CI or you are running different Rust versions.

@nazar-pc, how to proceed here? AFAIU mediasoup-rust mandates a minimal Rust version but the user is free to use a major one. Does mediasoup-rust fail to build is running Rust version doesn't satisfy that minimum version declared somewhere in mediasoup files? Should we update that minimum version and match whatever version runs in CI servers?

@nazar-pc
Copy link
Collaborator

We have rust-toolchain.toml in the repo. Assuming Rust toolchain is installed with rustup and Cargo is used, it should be used by default unless overridden. Don't override it, please, use what it chooses out of the box.

P.S. I don't mind bumping version in rust-toolchain.toml to much newer release.

@ibc
Copy link
Member

ibc commented Jun 20, 2024

We have rust-toolchain.toml in the repo. Assuming Rust toolchain is installed with rustup and Cargo is used, it should be used by default unless overridden.

I don't understand this. rustup will update Rust to whatever the newest version is, not to the version we say in rust-toolchain.toml.

@ibc
Copy link
Member

ibc commented Jun 20, 2024

@dhilipsiva can you run rustup --version in your system? It prints this in my computer:

$ rustup --version

rustup 1.26.0 (5af9b9484 2023-04-05)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.72.0 (5680fa18f 2023-08-23)`

Which matches the Rust version declared in rust-toolchain.toml in mediasoup (1.72.0). And when I run all cargo commands (fmt, clippy, etc) I do not see any warning, so probably you are using another version of Rust.

@dhilipsiva
Copy link
Contributor Author

@nazar-pc @ibc

I just realized I have been using rust toolchain installed using brew.

rustc --version
rustc 1.78.0 (9b00956e5 2024-04-29) (Homebrew)

@nazar-pc
Copy link
Collaborator

I don't understand this. rustup will update Rust to whatever the newest version is, not to the version we say in rust-toolchain.toml.

If rust-toolchain.toml is present, that specified toolchain version will be used for the project. Even if default on the system is newer or older. That is the whole point of that file: to have predictable toolchain and set of its components. If you run rustc --version in project directory you'll get the same version as in rust-toolchain.toml.

@dhilipsiva
Copy link
Contributor Author

@ibc

I just installed my toolchain using rustup.

rustup --version
rustup 1.27.1 (54dd3d00f 2024-04-24)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.79.0 (129f3b996 2024-06-10)`

rustc --version
rustc 1.79.0 (129f3b996 2024-06-10)
  • cargo fmt reported/changed nothing
  • cargo test passed
  • cargo clippy reported parts of code I never touched.

@dhilipsiva
Copy link
Contributor Author

@ibc @nazar-pc I ran all the commands from the root of the repo. Should I have run them from rust folder instead?

@nazar-pc
Copy link
Collaborator

@ibc @nazar-pc I ran all the commands from the root of the repo. Should I have run them from rust folder instead?

If you do that in project directory you should get this:

nazar-pc@nazar-pc:/w/g/mediasoup> rustc --version
rustc 1.72.0 (5680fa18f 2023-08-23)

Even though global version is different:

nazar-pc@nazar-pc:~> rustc --version
rustc 1.77.2 (25ef9e3d8 2024-04-09)

Make sure your brew version is not overriding rustup's version.

@nazar-pc
Copy link
Collaborator

Root works just fine as well

@dhilipsiva
Copy link
Contributor Author

Can confirm that I have uninstalled brew version of rust before running the cargo commands.

which rustc
/home/dhilipsiva/.cargo/bin/rustc
which rustup
/home/dhilipsiva/.cargo/bin/rustup

@nazar-pc
Copy link
Collaborator

well, just bump rust-toolchain.toml to 1.79.0 then, it'll match your current verison and everything should be fine.

@ibc
Copy link
Member

ibc commented Jun 20, 2024

If rust-toolchain.toml is present, that specified toolchain version will be used for the project. Even if default on the system is newer or older. That is the whole point of that file: to have predictable toolchain and set of its components. If you run rustc --version in project directory you'll get the same version as in rust-toolchain.toml.

Imagine I have rustc 1.72.0 in my system and rust-toolchain.toml says 1.79.0. Obviously things are gonna fail due to version mismtach.

Imagine I have rustc 1.79.0 in my system and rust-toolchain.toml says 1.72.0. What would happen? I don't have rustc 1.72.0. installed in my system.

@nazar-pc
Copy link
Collaborator

If rust-toolchain.toml is present, that specified toolchain version will be used for the project. Even if default on the system is newer or older. That is the whole point of that file: to have predictable toolchain and set of its components. If you run rustc --version in project directory you'll get the same version as in rust-toolchain.toml.

Imagine I have rustc 1.72.0 in my system and rust-toolchain.toml says 1.79.0. Obviously things are gonna fail due to version mismtach.

Imagine I have rustc 1.79.0 in my system and rust-toolchain.toml says 1.72.0. What would happen? I don't have rustc 1.72.0. installed in my system.

It will not fail in either case, It'll install (if not installed already) correct version and will use correct version in the project and nothing else.

@dhilipsiva
Copy link
Contributor Author

well, just bump rust-toolchain.toml to 1.79.0 then, it'll match your current verison and everything should be fine.

Sorry. Should I update the version, commit and push to this branch? Is that what you meant? 😅

@nazar-pc
Copy link
Collaborator

Yes

- Update Rust toolchain channel from "1.72.0" to "1.79.0" in the [rust-toolchain.toml] file.
@dhilipsiva
Copy link
Contributor Author

Update rust version in rust-toolchain.toml to 1.79.0

* v3:
  chore: Update Rust toolchain channel to version 1.79.0
@ibc
Copy link
Member

ibc commented Jun 21, 2024

Let's wait for CI.

@ibc
Copy link
Member

ibc commented Jun 21, 2024

@ibc
Copy link
Member

ibc commented Jun 21, 2024

CI failing in cargo clippy: https://github.com/versatica/mediasoup/actions/runs/9610107155/job/26507750935?pr=1409

I assume those are due to new version of Rust.

- Update Rust toolchain channel from "1.72.0" to "1.79.0" in the [rust-toolchain.toml] file.
* v3:
  chore: Update Rust toolchain channel to version 1.79.0
  Fix issue versatica#1374
  cosmetic
  fix Simulcast IncreaseLayer bug when producer score is zero (versatica#1410)
  Update NPM deps
  TcpConnectionHandle.cpp: properly close handle
  fix asan error for new-delete-type-mismatch (versatica#1411)
cargo clippy --fix --lib -p mediasoup
- Added `#[allow(dead_code)]` attribute to `ChannelReadCallback` in `channel_read_fn.rs`
- Modified and added test cases in `webrtc_server.rs`
- Made changes to the `Producer` struct and related functions in `producer.rs`
- Made updates to the `ScalabilityMode` implementation in `scalability_modes.rs`
- Implemented various functions and callbacks in `router.rs`
- Made modifications and fixes in `ortc/tests.rs`
- Updated test cases in `webrtc_transport.rs`
- Added attributes to struct definitions in `channel_write_fn.rs` and `consumer.rs`
- Modified the code in `worker/build.rs`
@dhilipsiva
Copy link
Contributor Author

dhilipsiva commented Jun 22, 2024

@ibc @nazar-pc Most of the clippy errors were due to planus codegen. I have updated the build script to include #![allow(clippy::all) within the generated module to exclude it from clippy. I have done so using a simple string replace method in the worker build.rs script. I hope this is an acceptable solution.

Edit: can confirm that all clippy warnings are resolved when tested on my local.

@versatica versatica deleted a comment from dhilipsiva Jun 23, 2024
@nazar-pc
Copy link
Collaborator

Well, the goal is not to get clippy to shut up by suppressing lints/commenting code without any explanation, but to actually address it.

I wanted to fix it, but you have sent PR from an org rather than personal account, so I can't do that. I have pushed commit with fixes here: https://github.com/nazar-pc/mediasoup/tree/dry-DtlsFingerprint-from_fbs-fix-lints

@dhilipsiva
Copy link
Contributor Author

Oh, wow! Using Drop to silence the dead_code error is such a neat trick. :) It didn't cross my mind.

@nazar-pc
Copy link
Collaborator

Well, another way is to actually use named field (rather than tuple), where name starts with _, that way clippy will not complain either. Probably should have done that instead.

@nazar-pc
Copy link
Collaborator

The key though is to leave an explanation why that is okay/expected

@ibc
Copy link
Member

ibc commented Jun 24, 2024

So is this ready to merge?

@nazar-pc
Copy link
Collaborator

I expect one more update as described above

@dhilipsiva
Copy link
Contributor Author

Oh sincere apologies. I misunderstood. I mistakenly assumed that you will merge the changes you made on your fork. Let me do this in a couple of hours.

- Add a new method `new` to the `ChannelReadCallback` struct in `channel_read_fn.rs`
- Modify the implementation of the `ChannelReadCallback` struct to use the new method
- Update the `prepare_channel_read_fn` function to use the `new` method of `ChannelReadCallback`
- Add a typedef `CallbackType` to `channel_write_fn.rs` to avoid warnings
- Refactor the `ChannelReadCallback` struct in `channel_write_fn.rs` to use `CallbackType`
- Add a constructor method `new` to `ChannelReadCallback` in `channel_write_fn.rs`
- Update the `prepare_channel_write_fn` function to use the new `ChannelReadCallback` constructor in `channel_write_fn.rs`
- Modify the `RtpCodecCapability` struct in `consumer.rs` to include additional parameters for audio and video codecs
- Add `RtpHeaderExtension` structs for various media kinds and URIs in `consumer.rs`
- Refactor the `ExecutorGuard` struct in `consumer.rs` to use a new `new` method instead of the `impl` block
- Modify the `create_executor` function in `consumer.rs` to use the new `ExecutorGuard` initialization
- Modify the `init` function in `consumer.rs` to return multiple values rather than a single tuple
- Remove the `#[allow(dead_code)]` attribute from the `impl RtpParameters` block in `rtp_parameters.rs`
@dhilipsiva
Copy link
Contributor Author

I have removed ignoring dead_code warnings. I have used named fields as suggested. Please let me know if there is anything I need to change :)

@dhilipsiva
Copy link
Contributor Author

While going through the commit @nazar-pc made on their personal repo, I realized I missed something:

Silencing clippy lints on files generated by plaun codegen: nazar-pc@7cdf648#diff-4bce653f545fe2edb899732221932e371b14c992e8304ab8f6a9677a142a7ec8R4

I did this same exact change and I can confirm that this is what caused #1413

../../../../../../worker/meson.build:284:20: ERROR: Subproject exists but has no meson.build file.

I don't think I am smart enough to figure this one out. I deleted the registry folder as mentioned in #1413' s thread - but the same error kept happening.

@nazar-pc
Copy link
Collaborator

During development files are stored in the project's directory. Delete everything in worker/subprojects except wrap files and you should be good to go. make clean-all should work in worker directory too.

@ibc
Copy link
Member

ibc commented Jun 25, 2024

Now this is ready to merge, right?

@ibc ibc requested review from jmillan and nazar-pc June 25, 2024 09:24
Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Not all comments/suggestions were ported into here still, Inaki

Box<dyn (FnMut(UvAsyncT) -> Option<Vec<u8>>) + Send + 'static>,
);
pub(super) struct ChannelReadCallback {
_callback: Box<dyn (FnMut(UvAsyncT) -> Option<Vec<u8>>) + Send + 'static>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd really like for comments to be present at such locations

@ds-colligence
Copy link

I have made all required changes. Please let me know if I have missed any. If there are no more changes and the tests succeeds, we can merge this.

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for contribution and patience!

@ibc
Copy link
Member

ibc commented Jun 26, 2024

Merging, thanks a lot.

@ibc ibc merged commit 9490546 into versatica:v3 Jun 26, 2024
35 checks passed
@ibc
Copy link
Member

ibc commented Jun 26, 2024

I've updated Rust CHANGELOG for next release: a80630a

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.

None yet

4 participants