Publishing to crates.io? (I'll help!)

[vivyir]

Hiya!

I'd love to use your vorbis library in one of my projects (because as you've already mentioned, the current vorbis ecosystem on crates.io is really outdated, has bad code quality and just straight up doesn't even work because of dependency hell in the case of the vorbis crate which is currently on crates.io!); I absolutely love what you've done here, it's really clean, high quality code, all the packages!

In my opinion you should publish these crates to crates.io, maybe under the name of "vorbis-next", "ogg-sys-next", and "vorbis-sys-next" perhaps, your call really, by doing so the ecosystem of rust itself would improve by a lot; Seriously, I spent around 20 hours today (yes I didn't sleep) looking for different lossy audio encoders, vorbis, opus, etc to use in my project, and while I did find an ogg-opus crate it couldn't be played or decoded easily on the web so it was useless for me, I also saw the current vorbis crate and tried to use it but that dependency hell thing got me, your code was really clean however, a breath of fresh air really, while I can continue using your library with git in cargo I think it deserves to be on crates.io, it'd save me and many others like me who are trying to just decode/encode vorbis a lot of time.

If you'd like to publish these crates I can help get them ready by writing nice readmes (in your style) for the packages and also separating the code in the main vorbis bindings so that instead of 1 giant 1k SLOC file it's structured nicely and easier to contribute to/maintain and also writing whatever kind of example I can think of for it, overall just polish them up and then open a PR for it, I'll make sure that the function signatures stay the exact same so your project PackSquash which depends on this library using git won't encounter any problems (I can try building PackSquash from source to confirm as well but I may need your help with that), so this'd only be a change in this library and won't require any change in PackSquash.

Lemme know what you think, if you do approve, lemme know how I can build PackSquash and then I'll get to work!

[AlexTMjugador]

Hi!

First of all, I'm delighted you liked my approach to the problem and found these bindings useful for your needs! :heart:

I agree that publishing vorbis-rs on crates.io can be a good idea. As you've said, it can give this project some due visibility and recognition and be helpful to the ecosystem. However, I think that there is some work to do for a proper crates.io publication:

• [x] Review and polish the API. As stated in the readme, this project was meant to address the encoding and decoding needs of PackSquash, another project I maintain. The API offered by libvorbis is pretty flexible and rich, so it'd be appropriate for vorbis-rs to expose all of its functionality, not only what I needed. If I recall correctly, I omitted some (minor) things, likely some possible bitrate management combinations, and some notable things, suck as any kind of seeking support.
• [x] Make it work on non-nightly Rust. Most crates on crates.io work on stable Rust, but vorbis-rs requires nightly Rust due to its usage of unstable Rust features. This was not a concern to me, but it might be a deal-breaker for others.
• [ ] Review that every unsafe use is sound. I tried my best to document every use of unsafe, and these bindings were tested to work as expected in practical scenarios. I didn't notice any glaring memory management issues, either. But that doesn't prove correctness. Even though I consider myself seasoned in Rust, these are my first Rust bindings for a C library ever, so some peer-review would be due.
• [x] Improve the documentation and project structure. This is the task you described :wink:

Some of these tasks might not be strictly necessary for a first crates.io release. For example, getting a formal proof of unsafe usage soundness is something that only the most popular crates can ask for. But it'd be nice to at least consider them.

Please feel free to work on any of the aforementioned tasks. I'll gladly welcome any PRs for them, and I'm looking forward to a crates.io release! :smile:

If it's useful for you, you can take the optivorbis crate, another library I maintain, as an example of how I like to do things.

About building PackSquash, you can find its source on its GitHub repository. It is another Rust project, so the standard Cargo commands (cargo build, cargo test...) work as you'd expect. You can check out its contributing guidelines for more details.

Lastly, I've added the hacktoberfest label to this issue, so you could be eligible for a prize for your contributions. If you are interested, you can check out the Hacktoberfest website for more information.

[vivyir]

Lovely! thanks for the hacktoberfest label by the way! <3; I completely forgot that was a thing.

To be completely honest with you I'm a complete beginner in terms of unsafe usage and making sure everything is sound, I think I'd be able to take care of point 2 and 4 however, about point 2, would you happen to know how one could replicate what the functions gated behind the new_uninit feature do in stable rust? I tried to look around and see how I could replicate it in stable rust and while I did understand some things; most of them went right over my head :P.

I've also checked out the optivorbis crate and will be using it as a guide, much appreciated!

[AlexTMjugador]

about point 2, would you happen to know how one could replicate what the functions gated behind the new_uninit feature do in stable rust? I tried to look around and see how I could replicate it in stable rust and while I did understand some things; most of them went right over my head :P.

Box::new_uninit is, luckily, just a more elegant (and potentially slightly more performant, see https://github.com/rust-lang/rust/issues/63291) way of getting a Box<MaybeUninit<T>>. You should be able to replace constructs like:

let mut vorbis_info = Box::new_uninit();

By the following slightly less elegant, but stable-friendly alternative:

let mut vorbis_info = Box::new(MaybeUninit::uninit());

With no behavior changes.

Let me know if you have any further questions or news to share!

Edit: the assume_init method added to Box<MaybeUninit<T>> by the new_uninit feature can also be emulated on stable Rust by converting the box to a raw pointer, casting the raw pointer to the target type, and reconstructing the box from that.

[vivyir]

May be a newbie mistake but as i was compiling for the first time (latest nightly) without any changes after updating submodules this error occured:

   Compiling vorbis v0.0.1 (/home/vivyir/programming/rust/vorbis-rs/packages/vorbis)
error[E0382]: borrow of moved value: `ogg_packet`
   --> packages/vorbis/src/lib.rs:874:6
    |
871 |                 let mut ogg_packet = MaybeUninit::uninit();
    |                     --------------   --------------------- this reinitialization might get skipped
    |                     |
    |                     move occurs because `ogg_packet` has type `MaybeUninit<ogg_packet>`, which does not implement the `Copy` trait
...
874 |                     ogg_packet.as_mut_ptr()
    |                     ^^^^^^^^^^^^^^^^^^^^^^^ value borrowed here after move
...
878 |                         ogg_packet: ogg_packet.assume_init()
    |                                                ------------- `ogg_packet` moved due to this method call, in previous iteration of loop
    |
note: this function takes ownership of the receiver `self`, which moves `ogg_packet`
   --> /home/vivyir/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/maybe_uninit.rs:623:37
    |
623 |     pub const unsafe fn assume_init(self) -> T {
    | 

Any idea how this could be fixed?

[AlexTMjugador]

That error looks like a false positive, as that variable is always reinitialized before it is moved. I've run cargo test on a fresh clone of the repository successfully on the latest nightly, and the CI workflows ran successfully some days ago, so I can't reproduce it. This is my rustc -vV output:

rustc 1.66.0-nightly (8b0c05d9a 2022-10-07)
binary: rustc
commit-hash: 8b0c05d9ad7121cdb97600f261bcd5f04c8db20d
commit-date: 2022-10-07
host: x86_64-unknown-linux-gnu
release: 1.66.0-nightly
LLVM version: 15.0.2

Have you tried running cargo clean? Are you totally sure you're using a recent nightly build?

[vivyir]

Just checked by running cargo clean and then doing cargo test again, same error, I also ran rustc -vV and got the exact same output as you, letter by letter, really weird, I'll try updating my system and restarting, maybe that'll fix it?

[AlexTMjugador]

Perhaps it could. If it doesn't, I think you can work around that error in your local copy by wrapping that MaybeUninit in an Option and unwrap() or take() it before calling assume_init. Remember to not commit that change, though!

[vivyir]

Nothing I tried worked, no updating, no wrapping in Option, gonna have to leave it here until I find time again, very weird compiler issue.

[AlexTMjugador]

I'm sad to hear that. Did it build fine for you at some point? I could try to dig further into this issue, although it puzzles me how it works fine on my end.

[vivyir]

Nope unfortunately, this happened on the first build i tried, I have seen on the internet that incorrect ”this reinitialization might be skipped” errors have happened but i don't see how they can happen here, and also the fact that it only happens on my end, I could try to get a hold of another system to try compiling on later however.

[AlexTMjugador]

By the way, in case it's helpful to you, this is the Option workaround I was thinking on. It should be enough to fool your compiler into thinking the right thing, should you need to do so.

diff --git a/packages/vorbis/src/lib.rs b/packages/vorbis/src/lib.rs
index a7ed12b..749bde9 100644
--- a/packages/vorbis/src/lib.rs
+++ b/packages/vorbis/src/lib.rs
@@ -854,6 +854,7 @@ impl<W: Write> VorbisEncoder<W> {
    fn write_pending_blocks(&mut self) -> Result<(), VorbisError> {
        // SAFETY: we assume the functions inside this unsafe block follow their
        // documented contract
+       let mut ogg_packet;
        unsafe {
            while libvorbis_return_value_to_result!(vorbis_analysis_blockout(
                &mut *self.vorbis_encoding_state.vorbis_dsp_state,
@@ -868,7 +869,8 @@ impl<W: Write> VorbisEncoder<W> {
                    &mut *self.vorbis_encoding_state.vorbis_block
                ))?;

-               let mut ogg_packet = MaybeUninit::uninit();
+               ogg_packet = Some(MaybeUninit::uninit());
+               let mut ogg_packet = ogg_packet.take().unwrap();
                while libvorbis_return_value_to_result!(vorbis_bitrate_flushpacket(
                    &mut *self.vorbis_encoding_state.vorbis_dsp_state,
                    ogg_packet.as_mut_ptr()

[vivyir]

Finally got time again and that uninitialized error magically went away! I made sure the code could compile on stable rust and also separated it by taking inspiration from the way you structured your optivorbis crate, couldn't improve the documentation though because what you had already written there was pretty perfect. (#6)

[AlexTMjugador]

I'm happy to announce that, after dealing with release chores and polishing several rough edges here and there, vorbis_rs is now on crates.io! :tada:

Notably, I had to drop the build-time dependency on git2 because I learned the hard way that crates.io does not publish any source control information, so trying to find submodules in build scripts didn't work. This should result in faster builds!