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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, May 30, 2023 at 6:27 PM Manos Pitsidianakis
<manos.pitsidianakis@xxxxxxxxxx> wrote:
>
> Hello Bart,
>
> this error means pkg-config could not find libgpiod. Erik changed the
> linking logic, so now instead of linking a local copy of the library
> it lets pkg-config find it. But you haven't installed it locally.
>
> Running with these flags from a makefile in the commit:
>
> command = SYSTEM_DEPS_LIBGPIOD_NO_PKG_CO
> NFIG=1 \
> +
> SYSTEM_DEPS_LIBGPIOD_SEARCH_NATIVE="${PWD}/../../../lib/.libs/" \
> +               SYSTEM_DEPS_LIBGPIOD_LIB=gpiod \
> +               SYSTEM_DEPS_LIBGPIOD_INCLUDE="${PWD}/../../../include/"  \
> +               cargo build --release --lib
>
> Should work.
>

Ah, got it. Ok, now applied.

Bart

> On Tue, 30 May 2023 at 19:04, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > 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