Re: [PATCH libgpiod v2 2/2] bindings: rust: build against pkg-config info

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, May 26, 2023 at 5:28 PM Erik Schilling
<erik.schilling@xxxxxxxxxx> wrote:
>
> This change replaces building against "bundled" headers by always
> building agains system headers (while following standard conventions to
> allow users to specify the version to build against).
>
> Reasoning:
>
> Previously, the code generated the bindings based on the headers, but
> then links against `-lgpiod` without further specifying where that is
> coming from.
>
> This results in some challenges and problems:
>
> 1. Packaging a Rust crate with `cargo package` requires the folder
>    containing the Cargo.toml to be self-contained. Essentially, a tar
>    ball with all the sources of that folder is created. Building against
>    that tar ball fails, since the headers files passed to bindgen are
>    a relative path pointing outside of that folder.
>
> 2. While, for example, the cxx bindings are built AND linked against
>    the build results, the packaging situation for C++ libraries is a
>    bit different compared to Rust libs. The C++ libs will likely get
>    built as part of the larger libgpiod build and published together
>    with the C variant.
>
>    In Rust, the vast majority of people will want to build the glue-code
>    during the compilation of the applications that consume this lib.
>
>    This may lead to inconsistencies between the bundled headers and the
>    libraries shipped by the user's distro. While ABI should hopefully
>    be forward-compatible within the same MAJOR number of the .so,
>    using too new headers will likely quickly lead to mismatches with
>    symbols defined in the lib.
>
> 3. Trying to build the core lib as part of the Rust build quickly runs
>    into similar packaging issues as the existing solution. The source
>    code of the C lib would need to become part of some package
>    (often people opt to pull it in as a submodule under their -sys crate
>    or even create a separate -src package [1]). This clearly does not
>    work well with the current setup...
>
> Since building against system libs is probably? what 90%+ of the people
> want, this change hopefully addresses the problems above. The
> system-deps dependency honors pkg-config conventions, but also allows
> users flexible ways to override the defaults [2]. Overall, this keeps
> things simple while still allowing maximum flexibility.
>
> Since the pkg-config interface is just telling us which include paths to
> use, we switch back to a wrapper.h file that includes the real gpiod.h.
>
> Once Rust bindings require a lower version floor, the version metadata
> can also be updated to help telling users that their system library is
> too old.
>
> In order to support people hacking on the Rust bindings, building with
> make will automatically set the right set of environment variables.
> In case people want to customize it when building without `make`, a
> reference to the system_deps documentation is given in the README.md.
>
> [1] https://github.com/alexcrichton/openssl-src-rs
> [2] https://docs.rs/system-deps/latest/system_deps/#overriding-build-flags
>
> Signed-off-by: Erik Schilling <erik.schilling@xxxxxxxxxx>
> ---
>  README                                |  4 +++-
>  bindings/rust/libgpiod-sys/Cargo.toml |  4 ++++
>  bindings/rust/libgpiod-sys/README.md  |  8 +++++++
>  bindings/rust/libgpiod-sys/build.rs   | 40 +++++++++++++++++++++++------------
>  bindings/rust/libgpiod-sys/wrapper.h  |  1 +
>  bindings/rust/libgpiod/Makefile.am    |  8 ++++++-
>  6 files changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/README b/README
> index 85b6300..5764eee 100644
> --- a/README
> +++ b/README
> @@ -218,7 +218,9 @@ the PYTHON_CPPFLAGS and PYTHON_LIBS variables in order to point the build
>  system to the correct locations. During native builds, the configure script
>  can auto-detect the location of the development files.
>
> -Rust bindings require cargo support.
> +Rust bindings require cargo support. When building the Rust bindings along the
> +C library using make, they will be automatically configured to build against the
> +build results of the C library.
>
>  TESTING
>  -------
> diff --git a/bindings/rust/libgpiod-sys/Cargo.toml b/bindings/rust/libgpiod-sys/Cargo.toml
> index 2b20fa6..8b17039 100644
> --- a/bindings/rust/libgpiod-sys/Cargo.toml
> +++ b/bindings/rust/libgpiod-sys/Cargo.toml
> @@ -18,3 +18,7 @@ edition = "2021"
>
>  [build-dependencies]
>  bindgen = "0.63"
> +system-deps = "2.0"
> +
> +[package.metadata.system-deps]
> +libgpiod = "2"
> diff --git a/bindings/rust/libgpiod-sys/README.md b/bindings/rust/libgpiod-sys/README.md
> index 1cb3b0a..90198d8 100644
> --- a/bindings/rust/libgpiod-sys/README.md
> +++ b/bindings/rust/libgpiod-sys/README.md
> @@ -8,6 +8,14 @@ SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>  Automatically generated Rust FFI bindings via
>         [bindgen](https://github.com/rust-lang/rust-bindgen).
>
> +## Build requirements
> +
> +A compatible variant of the C library needs to detectable using pkg-config.
> +Alternatively, one can inform the build system about the location of the
> +libs and headers by setting environment variables. The mechanism for that is
> +documented in the
> +[system_deps crate documentation](https://docs.rs/system-deps/6.1.0/system_deps/#overriding-build-flags).
> +
>  ## License
>
>  This project is licensed under either of
> diff --git a/bindings/rust/libgpiod-sys/build.rs b/bindings/rust/libgpiod-sys/build.rs
> index 0ac2730..9e6a93c 100644
> --- a/bindings/rust/libgpiod-sys/build.rs
> +++ b/bindings/rust/libgpiod-sys/build.rs
> @@ -1,25 +1,44 @@
>  // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
> -// SPDX-FileCopyrightText: 2022 Linaro Ltd.
> +// SPDX-FileCopyrightText: 2022-2023 Linaro Ltd.
>  // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> +// SPDX-FileCopyrightText: 2023 Erik Schilling <erik.schilling@xxxxxxxxxx>
>
>  use std::env;
>  use std::path::PathBuf;
>
> -fn generate_bindings() {
> +fn main() {
> +    // Probe dependency info based on the metadata from Cargo.toml
> +    // (and potentially other sources like environment, pkg-config, ...)
> +    // https://docs.rs/system-deps/latest/system_deps/#overriding-build-flags
> +    let libs = system_deps::Config::new().probe().unwrap();
> +
>      // Tell cargo to invalidate the built crate whenever following files change
> -    println!("cargo:rerun-if-changed=../../../include/gpiod.h");
> +    println!("cargo:rerun-if-changed=wrapper.h");
>
>      // The bindgen::Builder is the main entry point
>      // to bindgen, and lets you build up options for
>      // the resulting bindings.
> -    let bindings = bindgen::Builder::default()
> +    let mut builder = bindgen::Builder::default()
>          // The input header we would like to generate
>          // bindings for.
> -        .header("../../../include/gpiod.h")
> +        .header("wrapper.h")
>          // Tell cargo to invalidate the built crate whenever any of the
>          // included header files changed.
> -        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
> -        // Finish the builder and generate the bindings.
> +        .parse_callbacks(Box::new(bindgen::CargoCallbacks));
> +
> +    // Inform bindgen about the include paths identified by system_deps.
> +    for (_name, lib) in libs {
> +        for include_path in lib.include_paths {
> +            builder = builder.clang_arg("-I").clang_arg(
> +                include_path
> +                    .to_str()
> +                    .expect("Failed to convert include_path to &str!"),
> +            );
> +        }
> +    }
> +
> +    // Finish the builder and generate the bindings.
> +    let bindings = builder
>          .generate()
>          // Unwrap the Result and panic on failure.
>          .expect("Unable to generate bindings");
> @@ -30,10 +49,3 @@ fn generate_bindings() {
>          .write_to_file(out_path.join("bindings.rs"))
>          .expect("Couldn't write bindings!");
>  }
> -
> -fn main() {
> -    generate_bindings();
> -
> -    println!("cargo:rustc-link-search=./../../lib/.libs/");
> -    println!("cargo:rustc-link-lib=gpiod");
> -}
> diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h
> new file mode 100644
> index 0000000..8a8bd41
> --- /dev/null
> +++ b/bindings/rust/libgpiod-sys/wrapper.h
> @@ -0,0 +1 @@
> +#include <gpiod.h>
> diff --git a/bindings/rust/libgpiod/Makefile.am b/bindings/rust/libgpiod/Makefile.am
> index e9a10c1..92edbfc 100644
> --- a/bindings/rust/libgpiod/Makefile.am
> +++ b/bindings/rust/libgpiod/Makefile.am
> @@ -2,7 +2,13 @@
>  # SPDX-FileCopyrightText: 2022 Linaro Ltd.
>  # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> -command = cargo build --release --lib
> +# We do not want to build against the system libs when building with make. So we
> +# specify the paths to the build directory of the C lib.
> +command = SYSTEM_DEPS_LIBGPIOD_NO_PKG_CONFIG=1 \
> +               SYSTEM_DEPS_LIBGPIOD_SEARCH_NATIVE="${PWD}/../../../lib/.libs/" \
> +               SYSTEM_DEPS_LIBGPIOD_LIB=gpiod \
> +               SYSTEM_DEPS_LIBGPIOD_INCLUDE="${PWD}/../../../include/"  \
> +               cargo build --release --lib
>
>  if WITH_TESTS
>  command += --tests
>
> --
> 2.40.0
>

When I build the rust bindings in-tree and run them like:

sudo LD_LIBRARY_PATH=<snip>/libgpiod/lib/.libs/
CARGO_TARGET_DIR=/tmp/libgpiod-rust /root/.cargo/bin/cargo test

I get

error: failed to run custom build command for `libgpiod-sys v0.1.0
(<snip>/libgpiod/bindings/rust/libgpiod-sys)`

Caused by:
  process didn't exit successfully:
`/tmp/libgpiod-rust/debug/build/libgpiod-sys-1d9e7999a2f148d2/build-script-build`
(exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=LIBGPIOD_NO_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG
  cargo:rerun-if-env-changed=PKG_CONFIG
  cargo:rerun-if-env-changed=LIBGPIOD_STATIC
  cargo:rerun-if-env-changed=LIBGPIOD_DYNAMIC
  cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
  cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_PATH
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64-unknown-linux-gnu
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64_unknown_linux_gnu
  cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
  cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR

  --- stderr
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err`
value: PkgConfig(`"pkg-config" "--libs" "--cflags" "libgpiod"
"libgpiod >= 2"` did not exit successfully: exit status: 1
  error: could not find system library 'libgpiod' required by the
'libgpiod-sys' crate

  --- stderr
  Package libgpiod was not found in the pkg-config search path.
  Perhaps you should add the directory containing `libgpiod.pc'
  to the PKG_CONFIG_PATH environment variable
  Package 'libgpiod', required by 'virtual:world', not found
  Package 'libgpiod', required by 'virtual:world', not found
  )', libgpiod-sys/build.rs:13:51
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

What am I doing wrong?

Bart