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

cargo check breaks build_ffi_test #197

Closed
baumanj opened this issue Mar 3, 2020 · 4 comments · Fixed by #199
Closed

cargo check breaks build_ffi_test #197

baumanj opened this issue Mar 3, 2020 · 4 comments · Fixed by #199
Assignees
Labels

Comments

@baumanj
Copy link
Contributor

baumanj commented Mar 3, 2020

If cargo check is run before cargo test, build_ffi_test breaks in the following manner:

$ cargo clean && cargo check && cargo test --all
[…]
test build_ffi_test ... FAILED

failures:

---- build_ffi_test stdout ----
status: exit code: 2
--- stdout ---
rm -f test
rm -f *.a.out
rm -f *.o *.a
rm -f *.d
rustc -g --crate-type staticlib --crate-name mp4parse \
	  --emit dep-info,link=libmp4parse.a \
	  --print native-static-libs \
	  -L /Users/jbauman/src/mp4parse-rust_m-u-current/target/debug/build/mp4parse_capi-45f37232212af4dc/out/../../../deps ../src/lib.rs \
	  2> libmp4parse.a.out || cat libmp4parse.a.out >&2
c++ -g -Wall -std=c++11 -I../include/ -c test.cc
c++ -g -Wall -std=c++11 -I../include/ -o test *.o libmp4parse.a 

-- stderr ---
error[E0464]: multiple matching crates for `mp4parse`
  --> ../src/lib.rs:37:1
   |
37 | extern crate mp4parse;
   | ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: candidates:
           crate `mp4parse`: /Users/jbauman/src/mp4parse-rust_m-u-current/target/debug/deps/libmp4parse-d0bac2999dfaf13d.rlib

error[E0463]: can't find crate for `mp4parse`
  --> ../src/lib.rs:37:1
   |
37 | extern crate mp4parse;
   | ^^^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0463`.
clang: error: no such file or directory: 'libmp4parse.a'
make: *** [test] Error 1

thread 'build_ffi_test' panicked at 'assertion failed: output.status.success()', mp4parse_capi/tests/build_ffi_test.rs:19:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

See #193 (comment) for some background.

In the meantime, we can work around the issue by running cargo check (for the sake of checking feature-specific code, since we can't run cargo test with the mp4parse_fallible feature) after cargo test.

@ChunMinChang
Copy link
Member

ChunMinChang commented Mar 4, 2020

I am facing a similar issue when exposing the C API from Rust.

I am not sure if cc-rs can help in this case. It's used to integrate C/C++ code into a Rust library/application. But what mp4parse_capi wants is to integrate Rust code into C/C++ application.

The simplest way is to run cargo clean before cargo test. It should clean the duplicate crates built before. The following is another hacky way to avoid using example/Makefile and reuse the library built before:

fn build_ffi_test() {
    use std::process::Command;

    const C_FILE: &str = "examples/test.cc";
    const HEADER_DIR: &str = "include/";
    const LIBRARY_DIR: &str = "../target/debug/";
    const LIBRARY_NAME: &str = "mp4parse_capi";
    const EXECUTABLE: &str = "examples/test";

    // Generate a libmp4parse_capi.a if it doesn't exist.
    let build_library = Command::new("cargo")
        .arg("build")
        .output()
        .expect("failed to build library");
    assert!(build_library.status.success());

    let build_executable = Command::new("c++")
        .arg(C_FILE)
        .arg("-g")
        .arg("-Wall")
        .arg("-std=c++11")
        .arg(format!("-I{}", HEADER_DIR))
        .arg("-L")
        .arg(LIBRARY_DIR)
        .arg(format!("-l{}", LIBRARY_NAME))
        .arg("-o")
        .arg(EXECUTABLE)
        .output()
        .expect("failed to build executable");
    assert!(build_executable.status.success());

    let run_executable = Command::new(EXECUTABLE)
        .output()
        .expect("failed to run executable");
    //let output = String::from_utf8(run_executable.stdout).unwrap();
    //println!("{}", output);
    assert!(run_executable.status.success());

    let remove_executable = Command::new("rm")
        .arg(EXECUTABLE)
        .output()
        .expect("failed to remove executable");
    assert!(remove_executable.status.success());
}

with adding

[lib]
crate-type = ["staticlib", "rlib"]
# "staticlib" for `libmp4parse_capi.a` that will be linked to example/test.cc
# "rlib" for other code use `extern crate mp4parse_capi`

in Cargo.toml

The code will do the same thing as what Makefile used to do.

@kinetiknz
Copy link
Collaborator

kinetiknz commented Mar 5, 2020

Thanks Chun-Min. That's similar to the fix I'm working on.

We can fix this immediate issue by invoking cargo rather than rustc in the Makefile and depending less on (guessing) the layout of cargo's build artifacts. I've got a patch that does that.

cc-rs helps for easy cross-platform compiling, but it's restricted to producing libraries so it requires changing the structure of the test, and that introduces other build complexities.

If we don't mind adding build-deps, cmake-rs with a simple CMakeLists.txt might work as a suitable cross-platform replacement for the existing Makefile, and should allow the test to still be built as a binary.

Another possibility is to give up on trying to build and run C code as part of a Rust test and add extra test runner steps to the CI.

kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 6, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 9, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
@baumanj
Copy link
Contributor Author

baumanj commented Mar 10, 2020

It appears that e006607 broke build_ffi_test (even when doing a clean build) due to reordering these lines in mp4parse_capi/src/lib.rs:

extern crate byteorder;
extern crate mp4parse;

I haven't dug into why, but I wanted an issue to reference in the change to have rustfmt leave those lines alone.

baumanj added a commit that referenced this issue Mar 10, 2020
This was changed by rustfmt, but it breaks `build_ffi_test` for some reason.
Add annotation to keep rustfmt from reordering them again (at least until we
come up with a better fix).

See #197 (comment)
@kinetiknz
Copy link
Collaborator

#199 will fix these issues, but #202 is a good temporary fix, thanks!

kinetiknz pushed a commit that referenced this issue Mar 10, 2020
This was changed by rustfmt, but it breaks `build_ffi_test` for some reason.
Add annotation to keep rustfmt from reordering them again (at least until we
come up with a better fix).

See #197 (comment)
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 10, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Mar 11, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes mozilla#197
kinetiknz added a commit that referenced this issue Mar 11, 2020
The existing process invoked a Makefile from a Rust test, building test.cc
and a statically linkable mp4parse_capi, then running a series of C API
tests.  This wasn't very portable due the reliable on a Makefile, which also
made assumptions about the layout of cargo's build artifacts.

The new process introduces a new crate (test_ffi) and changes mp4parse_capi
to additionally build as a cdylib.  A build.rs in test_ffi builds
test.cc (which must now be written with a simple library API, rather than as
a binary with a main() due to cc-rs limitations) into a static libtest,
which is then linked to test_ffi's Rust binary in main.rs.

test.cc is now split into run_main for the binary dumping tool and test_main
for the API tests.  `cargo run -p test_ffi path/to/a.mp4` invokes run_main,
and test_ffi's ffi_test invokes test_main.

Fixes #197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants