-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Devel headers #18807
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
base: master
Are you sure you want to change the base?
Devel headers #18807
Conversation
By default, Automake flattens the directory structure when installing headers via `include_HEADERS`, placing all headers into a single `${prefix}/include/` directory. This breaks out-of-tree builds of dplane plugins because FRR headers refer to each other using their source-relative paths (e.g., `#include "lib/zebra.h"`). As a result, using the installed headers outside of the FRR source tree is not just inconvenient — it's impossible, since the directory structure they expect is lost. This patch replaces all `include_HEADERS` directives with `nobase_include_HEADERS` so that Automake preserves the subdirectory layout (e.g., `lib/zebra.h` is installed as `${prefix}/include/lib/zebra.h`). This makes the headers usable by external projects. Signed-off-by: Maxime Leroy <[email protected]>
Out-of-tree dplane plugins require access to FRR's `config.h` to compile successfully, but this header is not installed by default. This patch installs `config.h` into the top-level include directory using `pkginclude_HEADERS`, matching how it's referenced across the codebase (`#include "config.h"`). This ensures the header is available to external projects. Signed-off-by: Maxime Leroy <[email protected]>
Out-of-tree dplane plugins need access to `zebra/` headers, but these are not installed by default. This patch installs the necessary headers from the `zebra/` directory using `nobase_include_HEADERS` to preserve the directory structure. This is essential because many `zebra/` headers include other headers using paths relative to the FRR source tree (e.g., `#include "zebra/zebra_vrf.h"`). Without preserving this layout in the install tree, the headers are unusable out-of-tree. Signed-off-by: Maxime Leroy <[email protected]>
Out-of-tree dplane plugins require access to FRR's public headers and configuration macros, but the Debian packaging did not previously include these in any installable package. This patch adds `frr-dev` packages to the Debian build rules, installing headers and any required development files. This makes it possible to build external consumers — such as dplane plugins — against a binary-distributed FRR installation, without needing to build FRR from source. Signed-off-by: Maxime Leroy <[email protected]>
This patch updates the Red Hat packaging for FRR to make the `frr-devel` RPM actually usable for out-of-tree plugin and application development. Previously, `frr-devel` declared a subpackage but only included headers under `%{_includedir}/frr/`, omitting key components such as `lib/*.h`, `zebra/*.h`, and `config.h`, which are now installed by the build system via `nobase_include_HEADERS`. Signed-off-by: Maxime Leroy <[email protected]>
This patch updates the Dockerfile to install a recent version of libyang from source, ensuring compatibility and removing dependency issues. Signed-off-by: Maxime Leroy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks maxime :)
As discussed on Slack, it would be a good opportunity to also add a pkg-config file.
LGTM, it is aligned with last week's call. cc @mwinter-osr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 separate NAKs. Comment on how to do this "correctly" coming up below.
@@ -140,7 +140,9 @@ noinst_LIBRARIES = | |||
nodist_noinst_DATA = | |||
lib_LTLIBRARIES = | |||
module_LTLIBRARIES = | |||
pkginclude_HEADERS = | |||
pkginclude_HEADERS = config.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First NAK of two: a config.h
from autoconf should never be installed or depended on by development headers. This is discussed in https://sourceware.org/autobook/autobook/autobook_66.html#Installing-Header-Files
In the case of FRR, it is primarily lib/frratomic.h
that causes this dependency. The reasons for this are that the use of atomic operations in Quagga began before ISO C11 was widely available. This is no longer a problem and the correct fix is to simply remove the compatibility handling in that file, which then means config.h
does not need to be installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference — it's a valid point that exposing config.h in installed headers can lead to conflicts.
That said, the issue in FRR isn't limited to frratomic.h. Several headers use #ifdef HAVE_... checks, which makes some form of config header necessary. Removing the atomics compatibility code alone doesn't fully eliminate this dependency.
Renaming config.h (e.g., to frr_config.h) is a useful step to avoid name collisions and makes it safer to include in installed headers, though it still exposes some internal build configuration and ties the headers to specific platform checks.
Depends: ${misc:Depends}, frr (= ${binary:Version}) | ||
Description: Development files for FRRouting | ||
This package contains the development files (headers and static libraries) | ||
necessary to build software that links against FRR internals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second (independent) NAK of two: you're saying in your own description that you wrote "links against FRR internals."
This is counter to the purpose of a -dev
package. The point is to provide a stable entry point to other projects to use things. FRR does not have stable APIs at this point, and does not have testing for stability. This would essentially be a "trap package", and it would complicate future work on introducing stable APIs.
Further, I would apply the "test requirement" here: without tests, this will simply break. There are test tools for this, abicc and abidiff: https://lvc.github.io/abi-compliance-checker/ https://sourceware.org/libabigail/manual/abidiff.html
Considering that there is a better solution available (comment below), this simply should not be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a follow-up to the initial PoC proposing a new dplane plugin, which was submitted to get community input on integration. In the weekly call, we agreed the plugin must remain out-of-tree.
However, building it is currently not possible due to the broken frr-devel package on Red Hat and its absence on Debian. This PR fixes that.
There’s no claim of ABI stability — just the need to support out-of-tree development in a practical, working state.
The "good" way to do this is to create a pkg-config file during building FRR for the FRR source/build directory itself. This is common practice in other projects, for example see mesonbuild/meson#3472 The effect of this is that you build your out-of-tree project by referring to the FRR source directory (and making this easier with the use of the I need to be absolutely clear here that building out of tree has nothing to do with creating and installing headers/a dev package. You can already build out of tree, it's just annoying because you have to mess with a bunch of CFLAGS, LDFLAGS, and other things. I agree this is a problem and should be made easier. But this does not mean creating a dev package. It means improving access to an FRR build tree. The intended end result for your dplane plugin is that you will have a The module documentation also explicitly says we don't have stable APIs: https://github.com/FRRouting/frr/blob/master/doc/developer/modules.rst?plain=1#L18-L20 — this is the implication of "Your module must match a version of FRR". I'm happy to work with you to create a |
I've asked Brian to add this particular item to this coming Tuesday's tech meeting. If you are interested in this please attend |
I can build my dplane plugin locally by aiming CFLAGS, LDFLAGS, or an frr-uninstalled.pc file at my FRR build tree—easy enough for personal use. A distribution build is different. It runs in a clean chroot that contains only the binary packages named in Build-Depends/BuildRequires. Because the runtime frr package ships neither headers nor the .pc file, the compiler can’t find <frr/*.h> and the build fails. Pulling the FRR source tree into the chroot would violate policy (e.g., Debian Policy §4.9 requires every build input to come from a declared package). So, unless FRR ships a small frr-headers / frr-dev package that installs those headers (and perhaps an “uninstalled” .pc), I don’t see how a dplane plugin could be packaged for a mainstream distro. Such devel package does not require a strict ABI, the dplan plugin can be recompiled everytime frr packages are upgraded on the distros. Am I missing something? |
I see that David has done some changes to config.h and I missed the last tech meeting. Is there anything else we are planning on doing on this PR? |
I think there was the possibility of having a That way it limits the risk of people assuming frr ships a stable API/ABI while still allowing to build out of tree dplane plugins based on installed DEB/RPM frr packages. Now that the |
We're not quite there yet, note the PR title said |
Thanks @eqvinox — great progress! Even once config.h is fully removed and no header is generated at build-time, external code still needs frr headers (lib/log.h, zebra/zebra_dplane.h, …). From my understanding, distro tools never pull raw source trees into the build chroot; they install only binary packages declared in Build-Depends. “Source packages should specify which binary packages they require to be installed … in order to build correctly.” — Debian Policy § 4.2 So a plugin would use Build-Depends: frr-headers (= X.Y), and no further source-tree access is needed. If I’ve misunderstood anything about distro packaging please let me know, otherwise I can update this series to add the frr-headers package (instead of frr-dev). I will also clearly said that header don't provide any api stability in the package description. For config.h, as a mid-term fix we could rename config.h to frrconfig.h and ship it together with headers in a frr-headers binary package; the same package can also install a libfrr.pc file. I see you have already starting to work on that 👍 |
That shouldn't be necessary; #18884 hopefully handles the most relevant bits; at least as far as |
These commits allow compiling the dplane plugin out-of-tree by properly exporting headers in the development package.