musl doesn't implement argp.h and requires an explicit lib for it, so
we must test for -largp presence and use it if appropriate
Signed-off-by: Dominique Martinet <[email protected]>
---
After having done this work I noticed runqslower is not actually
installed, so ideally instead of all of this it'd make more sense to
just not build it: would it make sense to take it out of the defaults
build targets?
I could just directly build the appropriate targets from tools/bpf
directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but
ideally I'd like to keep alpine's build script way of calling make from
the tools parent directory, and 'make bpf' there is all or nothing.
OTOH, we might as well keep this to allow people on alpine/void linux to
build runqslower if they want to. I didn't add libargp to default features
check so it shouldn't change much except for runqslower itself.
As an example it might be better to keep it independant from kbuild but
it already wasn't so I don't see much harm here.
tools/bpf/runqslower/Makefile | 30 +++++++++++++++++++++++++++++-
tools/build/feature/Makefile | 4 ++++
tools/build/feature/test-all.c | 4 ++++
tools/build/feature/test-libargp.c | 14 ++++++++++++++
4 files changed, 51 insertions(+), 1 deletion(-)
create mode 100644 tools/build/feature/test-libargp.c
diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
index da6de16a3dfb..20a1d9a2a908 100644
--- a/tools/bpf/runqslower/Makefile
+++ b/tools/bpf/runqslower/Makefile
@@ -23,6 +23,34 @@ VMLINUX_BTF_PATHS := $(if $(O),$(O)/vmlinux) \
VMLINUX_BTF_PATH := $(or $(VMLINUX_BTF),$(firstword \
$(wildcard $(VMLINUX_BTF_PATHS))))
+# musl requires linking with an external libargp
+FEATURE_USER = .runqslower
+FEATURE_TEST = libargp
+FEATURE_DISPLAY =
+
+check_feat := 1
+NON_CHECK_FEAT_TARGETS := clean
+ifdef MAKECMDGOALS
+ifeq ($(filter-out $(NON_CHECK_FEAT_TARGETS),$(MAKECMDGOALS)),)
+ check_feat := 0
+endif
+endif
+
+ifeq ($(check_feat),1)
+ifeq ($(FEATURES_DUMP),)
+srctree := $(abspath ../../..)
+include $(srctree)/tools/build/Makefile.feature
+else
+include $(FEATURES_DUMP)
+endif
+endif
+
+LIBS = -lelf -lz
+$(call feature_check,libargp)
+ifeq ($(feature-libargp), 1)
+LIBS += -largp
+endif
+
ifeq ($(V),1)
Q =
else
@@ -49,7 +77,7 @@ clean:
libbpf_hdrs: $(BPFOBJ)
$(OUTPUT)/runqslower: $(OUTPUT)/runqslower.o $(BPFOBJ)
- $(QUIET_LINK)$(CC) $(CFLAGS) $^ -lelf -lz -o $@
+ $(QUIET_LINK)$(CC) $(CFLAGS) $^ $(LIBS) -o $@
$(OUTPUT)/runqslower.o: runqslower.h $(OUTPUT)/runqslower.skel.h \
$(OUTPUT)/runqslower.bpf.o | libbpf_hdrs
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index de66e1cc0734..ceb4224a0ede 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -37,6 +37,7 @@ FILES= \
test-libtraceevent.bin \
test-libtracefs.bin \
test-libcrypto.bin \
+ test-libargp.bin \
test-libunwind.bin \
test-libunwind-debug-frame.bin \
test-libunwind-x86.bin \
@@ -205,6 +206,9 @@ $(OUTPUT)test-libtracefs.bin:
$(OUTPUT)test-libcrypto.bin:
$(BUILD) -lcrypto
+$(OUTPUT)test-libargp.bin:
+ $(BUILD) -largp
+
$(OUTPUT)test-gtk2.bin:
$(BUILD) $(shell $(PKG_CONFIG) --libs --cflags gtk+-2.0 2>/dev/null) -Wno-deprecated-declarations
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 5ffafb967b6e..149d3ef4a439 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -146,6 +146,10 @@
# include "test-libcrypto.c"
#undef main
+#define main main_test_libargp
+# include "test-libargp.c"
+#undef main
+
#define main main_test_sdt
# include "test-sdt.c"
#undef main
diff --git a/tools/build/feature/test-libargp.c b/tools/build/feature/test-libargp.c
new file mode 100644
index 000000000000..63b65d1f11fe
--- /dev/null
+++ b/tools/build/feature/test-libargp.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <argp.h>
+
+const char *argp_program_version = "test-libargp";
+static const struct argp_option opts[] = { {} };
+
+int main(int argc, char **argv)
+{
+ static const struct argp argp = {
+ .options = opts,
+ };
+ argp_parse(&argp, argc, argv, 0, NULL, NULL);
+ return 0;
+}
--
2.35.1
Daniel Borkmann wrote on Mon, Apr 25, 2022 at 11:35:41PM +0200:
> > I've dropped this patch from my alpine MR[1] and built things directly
> > with make bpftool etc as suggested above, so my suggestion to make it
> > more easily buildable that way is probably the way to go?
> > [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554
>
> Thanks for looking into this, Dominique! I slightly massaged patch 3 & 4
> and applied it to bpf-next tree.
Thanks!
> I don't really mind about patch 1 & 2, though out of tools/bpf/ the only
> one you /really/ might want to package is bpftool. The other tools are on
> the legacy side of things and JIT disasm you can also get via bpftool anyway.
I was thinking the other tools still had their uses, but I'll readily
admit I've never had a need for them so wasn't sure if I should package
them together or not.
I can see the use of bpf_dbg, but it's occasional enough that people who
need it can just build it when they need... Let's drop both patches and
I'll remove the other legacy tools from package as well.
My last concern would then just be to build it more easily. I just
noticed I can actually 'make bpf/bpftool' directly from the tools/
parent directory, but there's no equivalent for _install rules.
Would something like this make sense? (I can resend as proper patch if
so)
----
diff --git a/tools/Makefile b/tools/Makefile
index db2f7b8ebed5..743d242aebb3 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -112,6 +112,9 @@ cpupower_install:
cgroup_install counter_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
$(call descend,$(@:_install=),install)
+bpf/%_install: FORCE
+ $(call descend,$(@:_install=),install)
+
selftests_install:
$(call descend,testing/$(@:_install=),install)
----
> Given this is not covered by BPF CI, are you planning to regularly check
> for musl compatibility before a new kernel is cut?
alpine doesn't update the 'tools' subpackage with every kernel release,
I'm not sure what the exact schedule is but from the looks of it it
tracks LTS releases with updates every few months within the stable
release or to the next one.
I don't really have any resource to run a regular CI, but I guess I can
check from time to time.. If I ever get around to adding a linux-next
test to work's CI I can check bpftool builds at the same time, but who
knows when that'll ever be.
OTOH I had a first look last year (back when I tried to push
ACTIONRETVAL to musl) and there haven't been any new incompatibility, so
I think it's fine to just deal with minor hiccups when alpine upgrades
once in a while.
--
Dominique