katyo / aubio-rs

Rust bindings for Aubio library
37 stars 7 forks source link

Add correct aubio flag for linking #2

Closed Polochon-street closed 4 years ago

Polochon-street commented 4 years ago

Without this, trying to build anything with aubio-rs crate as a dep and actually using aubio content yields linking error. This tells the compiler to add the -L audio flag to the linking step

katyo commented 4 years ago

There are multiple options to link with libaubio.

The preferred way is using aubio-lib. A system-wide libaubio is also supported (technically). In any case we should support different options by providing selection to application crates. So I removed linking flags from aubio-sys and aubio-rs crates.

I think, we may simply create additional crate for linking with system-installed library (say aubio-dev or aubio-dist).

Polochon-street commented 4 years ago

Hm, after checking other libs, I saw f.ex. rust-ffmpeg (https://github.com/meh/rust-ffmpeg-sys) that is actually doing everything in the same sys crate, which seems to be something sensible?

Like, trying to find if we have a system-wide library using pkg-config, and if not, building it yourself?

They also use a build feature flag that basically forces to build the library yourself (like aubio-lib), I think that would be nice to have that instead of 2 different lib and dev crates? What do you think?

katyo commented 4 years ago

Initially the aubio-rs followed same way. But in this case it has several hard to solve problems.

First, it isn't so trivial to support cross-compilation for mobile phones (Android). Currently, I solved this issue using bundled library (aubio-lib crate). Similar solution is used in fftw crates.

Another problems is windows support. I'm not an expert in windows, but it seems there is nothing like pkg-config here.

Of course, It will be prefer to avoid extra crates like aubio-lib. But currently I don't know another solution with same flexibility.

katyo commented 4 years ago

Using crate features to select preferred linking way has other stupid problem.

Say, your application depends from two crates which both dependes from aubio-rs. What happen when both depended crates selects different ways of linking? This is not so clear to give univocal answer.

Polochon-street commented 4 years ago

Oh, okay, thanks a lot for the detailed explanation. It makes a lot of sense, then.

What really bothers me is that by default, when you try to just use aubio-rs as a crate, it simply doesn't compile properly, and I think it should at least try to get the system-wide aubio, hence my PR. Or specify that you shouldn't use aubio-rs directly, but use aubio-lib?

It kind of feels awkward to have a crate that doesn't compile when you use everything default and you have the system-wide lib on your system :sweat_smile:

Polochon-street commented 4 years ago

Also, here https://github.com/katyo/aubio-rs/blob/master/aubio-lib/README.md, I think aubio = "^0.1" should be replaced by aubio-rs = "^0.1" because aubio seems to belong to some other crate or something?

Like, this https://docs.rs/aubio/0.1.0/aubio/ doesn't seem correct?

katyo commented 4 years ago

I completely agree, but the question what is reasonable default for all use cases. I haven't simple answer at this moment.

katyo commented 4 years ago

Also, here https://github.com/katyo/aubio-rs/blob/master/aubio-lib/README.md, I think aubio = "^0.1" should be replaced by aubio-rs = "^0.1" because aubio seems to belong to some other crate or something?

Like, this https://docs.rs/aubio/0.1.0/aubio/ doesn't seem correct?

Oh, of course!

Polochon-street commented 4 years ago

@katyo I've changed my PR a bit to use pkg-config; apparently it just doesn't print anything, so it should be harmless for windows & android (cf https://users.rust-lang.org/t/cargo-windows-pkg-config-and-build-dependencies/4595/2)

Tell me if that's something you'd consider (cause then we need to find a prettier https://github.com/katyo/aubio-rs/pull/2/files#diff-6655eb347db33eae2975119018ff4d25R11 - we can't access mod source directly from main), or if you really don't wanna go in that direction. In that case, I'll close the issue :)

katyo commented 4 years ago

If I understand right, you would like to add linking directives here (i.e. cargo:rustc-link-lib=...). Ok, but we should provide compatibility with aubio-lib in this case.

Polochon-street commented 4 years ago

That's the magic,

    match pkg_config::Config::new()
        .atleast_version("0.4.9")
        .probe("aubio")
    {   
        Ok(paths) => {
            for path in paths.include_paths {
                println!("{}", path.display());
            }   
        }   
        Err(_) => (), 
    }; 

gives

cargo:rustc-link-search=native=/usr/lib
cargo:rustc-link-lib=aubio

I've changed some stuff to be able to still build using aubio-lib - mainly added a dep to aubio-sys to be able to have a feature flag. Things are working with the system flag, but since I haven't ever been able to get it working with aubio-lib, maybe you want to double-check that everything's in order from your side :smile:

Polochon-street commented 4 years ago

hm, builds are failing weirdly, but I think it's on Travis side - could you maybe restart them later to see if it's fixed? :smile:

katyo commented 4 years ago

Looks working. I'm on a way to merge this PR after hand testing.