-
Notifications
You must be signed in to change notification settings - Fork 137
Description
I've been looking at this code base and noticed that there were warnings when I built this. This issue is not high priority but I thought I'd write down my findings in case they were helpful. :)
I built this project by running cargo build workspace -p livekit, I'm getting a bunch of warnings. Many of these are not serious (such as unused fields, unreachable statements) but some of them look more serious:
warning: the type `CompleteCallback` does not permit zero-initialization
--> libwebrtc/src/native/audio_source.rs:130:21
|
130 | std::mem::zeroed::<sys_at::CompleteCallback>(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| this code causes undefined behavior when executed
| help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
|
= note: `webrtc_sys::audio_track::CompleteCallback` must be non-null
= note: because function pointers must be non-null
= note: `#[warn(invalid_value)]` on by default
I haven't dug into the above on exactly what's going on but, if this is a non-issue, using a #[allow(...)] might be appropriate here if this is benign.
The current CI checks allow warnings but I suggest moving towards an approach to fail the build on warnings. This make it easier for individual developers to understand when they have written code that generates a new warning and address it before the code gets merged. I think allowing unaddressed warnings normalizes them to developers and they become less useful over time. At the worst case, more warnings like the above could be less easily noticed.
One way to incrementally address this would be to fix all warnings one crate at a time to keep reviews more manageable. Once a crate's warnings have been addressed, add a CI check specifically for that crate with -D warnings enabled. Once all crates are able to build without warnings, add -D warnings in the config.toml to ensure that builds fail when warnings are emitted.
I see that this is project is being built with the nightly toolchain. I think disallowing warnings and building with nightly builds will result in the CI becoming more unstable because new compiler warnings could be introduced at any time. A better approach might be to continue building on nightly and allow warnings but add a CI step to build build with warnings disallowed using the latest stable rust toolchain. You can pin the toolchain to the latest stable release in a rust-toolchain.toml file at the root of the workspace and ensure that the project builds without warnings on the latest stable with a CI check. This could be a good way to ensure that the project builds on nightly with possible new warnings while ensuring that the latest stable toolchain builds without warnings. If you pin to a specific version in rust-toolchain.toml, this must be updated every 6 weeks. If you let rust-toolchain.toml choose the latest stable, the CI may break with each new stable release (every 6 weeks) so there's a cost of maintenance here but maybe some of this could be addressed by automation.
Again, not high priority but I thought I'd write this down in case this is helpful :)