Code Review Request: Wrapping a C++ Library using autocxx, cxx

code review
1

Hey there!

I'm a committer on the Eclipse uProtocol project, where we're developing a higher-layer middleware that can bridge across other transports such as Zenoh and SOME/IP. The initial goal is to enable use in Software Defined Vehicle platforms in the Automotive domain.

I was successfully able to convince the project lead with a number of PoCs that we should develop some components in Rust.

One of them is a uProtocol transport library for SOME/IP (a communication protocol which is used in the Automotive domain) which wraps the C++ vsomeip library.

The trouble is that currently no one else working on the project feels up to task review the PR which wraps vsomeip. The wrapping process involves use of autocxx, cxx, and some C++ glue code written to bridge between Rust and the library.

Could I trouble y'all with taking a look and providing some feedback?

Here's a link to the PR: Implement vsomeip-sys to wrap vsomeip C++ library by PLeVasseur · Pull Request #3 · eclipse-uprotocol/up-transport-vsomeip-rust · GitHub

Appreciate any help!

Cheers

(Hope that cross-posting is okay. I also posted this over on the Rust sub-reddit, but yet to have takers, so also posting here

Link to sub-reddit post: Reddit - Dive into anything)

2

1- From the instructions:

VSOMEIP_LIB_PATH=<path/to/vsomeip/lib> GENERIC_CPP_STDLIB_PATH=<path/to/generic/cpp/stdlib> ARCH_SPECIFIC_CPP_STDLIB_PATH=<path/to/arch_specific/cpp/stdlib> cargo build

and the build.rs:

    let vsomeip_lib_path = env::var("VSOMEIP_LIB_PATH")
        .expect("You must supply the path to a vsomeip library install, e.g. /usr/local/lib");
    let generic_cpp_stdlib = env::var("GENERIC_CPP_STDLIB_PATH")
        .expect("You must supply the path to generic C++ stdlib, e.g. /usr/include/c++/11");
    let arch_specific_cpp_stdlib = env::var("ARCH_SPECIFIC_CPP_STDLIB_PATH").expect("You must supply the path to architecture-specific C++ stdlib, e.g. /usr/include/x86_64-linux-gnu/c++/11");

, it seems these *_CPP_STDLIB_* environment variables are all required. Is there a reason for that?
Couldn't they be made optional if needed?

2- There are several of these:

        line = line.replace(
            "pub use bindgen::root::std_chrono_duration_long_AutocxxConcrete",
            "#[allow(unused_imports)] pub use bindgen::root::std_chrono_duration_long_AutocxxConcrete"
        );

To me this is error-prone.
It would probably be better to place a global directive in the vsomeip-sys lib.rs, something like:

#![allow(unused_imports)]

Regarding custom derive trait implementations, I think bindgen was discussing support for that, but I'm not sure if that landed.

3- AFAIK all these println! statements in the build.rs won't be printed to the console, you already use println!("cargo:warning=...").

2 Likes
3

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.