search

RustAudio / deepspeech-rs

Rust bindings for the deepspeech library
Other
295 stars 24 forks source link

Feature Request: Add ability to load shared library using `libloading`? #30

Closed ischeinkman closed 4 years ago

ischeinkman commented 4 years ago

Currently, this library relies on the system linker to find the shared library at both compile and link time. While this is fine in many environments, for use cases where we cannot ask the use to manually install libraries and environment variables it would be nice to be able to load the shared library from an arbitrary location. Currently, the best way this is done in Rust is via the libloading crate.

I have a preliminary version working at this branch, but I realized that it would make more sense to open an issue first so that an API could be decided upon.

This would require at least 1 breaking change, since the only way to actually disable the compile-time dynamic linking is via a cargo feature. In addition, there are questions relating to:

I'd be happy to make a PR once the API is decided!

est31 commented 4 years ago

Hi and thanks for your interest in improving deepspeech-rs!

Some remarks on your code:

E.g. like this:


macro_rules! make_wrappers {
    ($Lcx:ident, $call_fn:ident) => {
        pub struct Thing {
            #[allow(unused)]
            lcx: Lcx,
        }
        impl Thing {
            fn with_lcx(lcx: Lcx) -> Self {
                Self {
                    lcx
                }
            }
            fn something_else(&self, v: u8) {
                $call_fn!(&self.lcx, ffi::foo, (v));
            }
        }
    };
}

fn main() {
    without_arg::test();
    with_arg::test();
}

mod without_arg {
    type Lcx = ();
    mod ffi {
        pub fn foo(v: u8) {
            println!("v is: {}", v);
        }
    }
    macro_rules! call_fn {
        ($lcx:expr, $pat:path,($($param:expr),*)) => { $pat ($($param),*)};
    }
    make_wrappers!(Lcx, call_fn);
    impl Thing {
        pub fn new() -> Self {
            Self::with_lcx(())
        }
    }

    pub fn test() {
        let t = Thing::new();
        t.something_else(42);
    }
}

mod with_arg {
    type Lcx = String;
    mod ffi {
        pub fn foo(lcx: &super::Lcx, v: u8) {
            println!("v is: {} and lcx is '{}'", v, lcx);
        }
    }
    macro_rules! call_fn {
        ($lcx:expr, $pat:path, ($($param:expr),*)) => { $pat ($lcx, $($param),*)};
    }
    make_wrappers!(Lcx, call_fn);
    impl Thing {
        pub fn new(s: String) -> Self {
            Self::with_lcx(s)
        }
    }

    pub fn test() {
        let t = Thing::new("hello".to_owned());
        t.something_else(42);
    }
}

Has much less complicated wrapper code :)

ischeinkman commented 4 years ago
est31 commented 4 years ago

At the moment I'm leaning towards .maperror(|| ())? since this library uses () for its error type anyway

That solution is totally okay for now. We can improve that later on.

I will likely end up seeing if I can make an equivalent rustfmt.toml and also committing that, if that's alright.

The coding style I wrote this library in is a bit different from what can be configured with rustfmt but if you get it as close as possible it would be great. It's more important to me that it's easy to contribute than that my coding style is persisting.

There were actually 2 different tools: one was a modified bg tool that constructed the unsafe dynamic bindings from the -sys crate and one that constructed the safe bindings from the root-level lib.rs.

It's ok to keep the first tool, because it's an extension of the other bg tool, and converting the bindgen output to make it generic with macros is too hard. Just for the wrappers there is no need for a syn based generator as I'm convinced that a macro_rules based solution is possible. The first tool is 130 additional lines while the second tool is 250 lines, and the macros would cause much fewer lines. It's a question of avoiding overengineering :).

est31 commented 4 years ago

Fixed by #31 . Thanks!