Skip to content

onload-devel: ship an open_onload.pc pkg-config file#334

Open
tcasett wants to merge 1 commit into
Xilinx-CNS:masterfrom
tcasett:install-pc-file
Open

onload-devel: ship an open_onload.pc pkg-config file#334
tcasett wants to merge 1 commit into
Xilinx-CNS:masterfrom
tcasett:install-pc-file

Conversation

@tcasett

@tcasett tcasett commented Jul 9, 2026

Copy link
Copy Markdown

Summary

Onload ships the extensions library (libonload_ext) and its header (onload/extensions.h) in the devel package, but nothing tells pkg-config they exist. Anything that wants to build against Onload has to hardcode
-lonload_ext and the include path by hand. This adds a proper open_onload.pc so a downstream build can just do
pkg-config --cflags --libs open_onload and get the right flags.

We've been carrying this as a local spec-file patch for a while (a cat <<EOF heredoc bolted into %install that writes the .pc inline). It works, but it only ever helped us and was a manual update each time. Here's a pull request so we don't have to be responsible for doing this ourselves forever :)

New files and changes

Impact is really targeted and only touches what's necessary to reach parity with what we've been doing internally for a while:

  • scripts/onload_misc/open_onload.pc.in (new) - a real, tracked template with a @VERSION@ token. onload_mkdist already copies the whole onload_misc/ directory into the tarball, so no mkdist change is needed.

  • scripts/onload_install - a small install_pkgconfig function called from the headers flow. It renders the template, substitutes the version, and installs via install_f so the file is tracked in the uninstall manifest like every other onload_misc/ file. The version comes from onload_version_gen, the same source already used two lines up for onload_version.h, so the .pc Version: tracks the packaged release.

  • scripts/onload_misc/openonload.spec - one line in %files devel listing the file.

This update will work for debian installs as well, I think. It looks like debian/rules installs devel files through the
same scripts/onload_install --headers call, so RPM and DEB should both pick up the .pc from one place.

Testing

Built the full RPM set from a clean tree on RHEL 9:

./scripts/onload_mkpackage --rpm --version 9.0.2 --verbose --kernel-package-deps

The file lands in the devel package at the expected path:

$ rpm -qpl onload-devel-9.0.2-1.el9.noarch.rpm | grep pkgconfig
/usr/lib64/pkgconfig/open_onload.pc

Because it's now a well-formed .pc in the standard location, RPM's own
dependency generator picks it up without any help:

$ rpm -qp --provides onload-devel-9.0.2-1.el9.noarch.rpm | grep pkgconfig
pkgconfig(open_onload) = 9.0.2
$ rpm -qp --requires onload-devel-9.0.2-1.el9.noarch.rpm | grep pkg-config
/usr/bin/pkg-config

Rendered contents, with the version substituted from onload_version_gen:

prefix=/usr
exec_prefix=${prefix}
libdir=${prefix}/lib64
includedir=${prefix}/include/onload
install_suffix=

Name: OpenOnload
Description: Onload is a high performance user-level network stack, which accelerates TCP and UDP network I/O for applications using the BSD sockets on Linux.
URL: https://github.com/Xilinx-CNS/onload
Version: 9.0.2
Cflags: -I${includedir}
Libs: -L${libdir} -lonload_ext${install_suffix}

Debian notes

I have not built the Debian packages myself, so a second pair of eyes on the DEB devel package would be welcome (the code path is shared, but I'd rather someone confirm than take my word for it).

The devel package installs libonload_ext and onload/extensions.h but
nothing tells pkg-config they exist, so downstreams hardcode -lonload_ext
and the include path by hand. Add a pkg-config file so they can query it
with `pkg-config --cflags --libs open_onload`.

Do it through onload_install rather than the RPM spec so both packaging
paths get it: debian/rules installs devel files through the same
scripts/onload_install --headers call.

- scripts/onload_misc/open_onload.pc.in: tracked template with a @Version@
  token; onload_mkdist already copies onload_misc/ into the tarball
- scripts/onload_install: install_pkgconfig renders the template and
  installs via install_f so it lands in the uninstall manifest; the
  version comes from onload_version_gen, the same source already used
  for onload_version.h
- scripts/onload_misc/openonload.spec: list the file in %files devel
@tcasett tcasett requested a review from a team as a code owner July 9, 2026 17:27
@matsavill

Copy link
Copy Markdown

@tcasett A review form Claude 😄

● I've reviewed the full PR plus the surrounding onload_install script to check it against the codebase's actual path conventions. This is a clean, well-motivated change with good testing discipline, but
  there are two real correctness bugs in the templated .pc content — and one of them is exactly the Debian case the author flagged.

  PR #334 — ship open_onload.pc pkg-config file

  Verdict: REQUEST CHANGES (two path bugs in the template; mechanics are otherwise solid)

  ---
  🔴 Bug 1 — includedir breaks the documented #include <onload/extensions.h> convention

  The template sets:
  includedir=${prefix}/include/onload
  Cflags: -I${includedir}          # → -I/usr/include/onload

  But headers are installed to $i_include/onload/extensions.h (i.e. /usr/include/onload/extensions.h), and the public API is consumed as:
  #include <onload/extensions.h>
  That requires -I/usr/include, not -I/usr/include/onload. With the current Cflags, a downstream pkg-config-driven build would fail: the compiler would look for /usr/include/onload/onload/extensions.h. It
  also breaks any sibling includes (extensions_zc.h, extensions_timestamping.h) that extensions.h pulls in via the onload/ prefix.

  Fix: includedir=${prefix}/include (and Cflags: -I${includedir}, which then correctly resolves the <onload/...> and <etherfabric/...> trees — and is effectively a harmless no-op since /usr/include is already
  a default search path).

  ---
  🔴 Bug 2 — hardcoded libdir=/usr/lib64 is wrong on Debian/Ubuntu multiarch (the case you asked about)

  The template hardcodes:
  libdir=${prefix}/lib64
  But the install script does not hardcode lib64 — calculate_install_paths() calls detect_lib64_path():
  detect_lib64_path() {
    lib64dir="$usrdir/lib64"
    local libc="/lib/$(uname -m)-linux-gnu/libc.so.6"
    if [ -e "$libc" ]; then
      lib64dir="$usrdir/$(dirname $libc)"   # e.g. /usr/lib/x86_64-linux-gnu
    fi
  }
  So on Debian multiarch the .pc is installed to the correct path ($i_lib64/pkgconfig/... → /usr/lib/x86_64-linux-gnu/pkgconfig/), but its content still says libdir=/usr/lib64, and libonload_ext.so lives at
  the multiarch path, not /usr/lib64. Result: pkg-config --libs open_onload emits -L/usr/lib64, which contains no Onload libraries on Debian → link failure.

  This is precisely the "second pair of eyes on DEB" concern in the description — and the answer is: it's broken on DEB as written. RPM/RHEL is fine only because lib64dir happens to equal /usr/lib64 there.

  Fix: Substitute the real detected lib dir into the template rather than hardcoding it. Add a token (e.g. @LIBDIR@) and render both:
  try sed -e "s|@VERSION@|$version|" \
          -e "s|@LIBDIR@|$lib64dir|" \
          -e "s|@INCLUDEDIR@|$includedir|" \
          "$template" > "$pc"
  with the template using libdir=@LIBDIR@ / includedir=@INCLUDEDIR@. That also makes the file honor --lib64dir=, --includedir=, and --usrdir= (all of which the script supports and the current hardcoded
  prefix=/usr silently ignores).

  ---
  🟡 Minor

  1. prefix=/usr ignores --usrdir/--prefix. Same root cause as above — the template is a fixed string while the script is fully parameterized. For the RPM/DEB packaging path this is /usr in practice, so it's
  lower-severity than the lib/include bugs, but rendering prefix from $usrdir would make it correct for staged/relocated installs too. Note the .pc won't reflect $i_prefix (DESTDIR) — which is correct, since
  DESTDIR shouldn't leak into installed metadata.
  2. install_suffix= is dead. It's hardcoded empty and never substituted, so -lonload_ext${install_suffix} is always just -lonload_ext. Looks like a vestige of the internal heredoc patch. Either wire it up or
  drop it to avoid implying configurability that doesn't exist.
  3. No guard on empty version. If the sed extraction ever yields nothing, you get Version: with an empty value and no error. A quick [ -n "$version" ] || fail "could not determine onload version" would fail
  loudly instead of shipping a malformed .pc. (The regex itself is fine — the [[:space:]] after ONLOAD_VERSION correctly avoids matching ONLOAD_VERSION_HEX etc.)
  4. noarch + %{_libdir} fragility. The devel package is noarch, but the file goes under %{_libdir}/pkgconfig. On an x86_64/aarch64 builder %_libdir=/usr/lib64 and it matches the script's $i_lib64, so your
  build worked. On a builder where %_libdir resolves to /usr/lib (some 32-bit/noarch configs), the %files path and the install destination could diverge and break the RPM build. Not blocking, just fragile —
  and it's really a symptom of the same "lib dir isn't a constant" issue as Bug 2.

  ---
  What's good

  - Right approach: a tracked .pc.in template beats the internal cat <<EOF heredoc; onload_mkdist already ships onload_misc/, so no mkdist change is needed — correctly identified.
  - Correct hook point: install_pkgconfig is called from install_all_headers(), and version is sourced from onload_version_gen — the exact same source used two lines up for onload_version.h, so the version
  stays in lockstep with the release.
  - Uninstall-clean: using install_f means the file lands in the uninstall manifest like every other onload_misc/ file. Good citizenship.
  - RPM auto-provides/requires (pkgconfig(open_onload) / /usr/bin/pkg-config) fall out for free once it's a well-formed .pc in the standard dir — nice.
  - Testing shown with real rpm -qpl/--provides/--requires output.
  - try wrapping and temp-file cleanup match the surrounding style; running onload_version_gen/mktemp outside $do is consistent with the neighboring onload_version.h code path.

  Bottom line: the plumbing is right and merge-worthy in shape, but the two hardcoded paths (includedir and libdir) will produce a .pc that doesn't work for the very use case it's meant to enable — broken
  Cflags everywhere, and broken Libs specifically on Debian multiarch. Render those from $includedir/$lib64dir like the version token and it's solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants