Re: [PATCH 1/2] perf build: Use pkg-config for feature check for libtrace{event,fs}

[Leo Yan <leo.yan@arm.com>]

On 6/6/24 16:33, amadio@gentoo.org wrote:
> From: Guilherme Amadio <amadio@gentoo.org>
>
> Needed to add required include directories for the feature detection
> to succeed. The header tracefs.h is installed either into the include
> directory /usr/include/tracefs/tracefs.h when using the Makefile, or
> into /usr/include/libtracefs/tracefs.h when using meson to build
> libtracefs. The header tracefs.h uses #include <event-parse.h> from
> libtraceevent, so pkg-config needs to pick the correct include directory
> for libtracefs and add the one for libtraceevent to succeed.
>
> Note that in baa2ca59ec1e31ccbe3f24ff0368152b36f68720 the variable
> LIBTRACEEVENT_DIR was introduced, and now the method to compile against
> non-standard locations requires PKG_CONFIG_PATH to be set instead, which
> works for both libtraceevent and libtracefs.

Should we advise users to use PKG_CONFIG_LIBDIR or PKG_CONFIG_PATH?

When I wrote the patch set [1], at the beginning I also used the env
PKG_CONFIG_PATH, but later I think PKG_CONFIG_LIBDIR is better for users
for only lib path setting.

[1]
https://lore.kernel.org/linux-perf-users/20240604093223.1934236-3-leo.yan@arm.com/T/#mec2e44d6091dc1cf5f827b6542592b2938f1b697

> Signed-off-by: Guilherme Amadio <amadio@gentoo.org>
> Cc: Ian Rogers <irogers@google.com>
> ---
>   tools/build/feature/test-libtracefs.c |  2 +-
>   tools/perf/Makefile.config            | 27 +++++++++++++--------------
>   2 files changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/tools/build/feature/test-libtracefs.c b/tools/build/feature/test-libtracefs.c
> index 8eff16c0c10b..29a757a7d848 100644
> --- a/tools/build/feature/test-libtracefs.c
> +++ b/tools/build/feature/test-libtracefs.c
> @@ -1,5 +1,5 @@
>   // SPDX-License-Identifier: GPL-2.0
> -#include <tracefs/tracefs.h>
> +#include <tracefs.h>
>
>   int main(void)
>   {
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 7f1e016a9253..54a6c6bf23c7 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -183,14 +183,12 @@ FEATURE_CHECK_CFLAGS-libzstd := $(LIBZSTD_CFLAGS)
>   FEATURE_CHECK_LDFLAGS-libzstd := $(LIBZSTD_LDFLAGS)
>
>   # for linking with debug library, run like:
> -# make DEBUG=1 LIBTRACEEVENT_DIR=/opt/libtraceevent/
> -TRACEEVENTLIBS := -ltraceevent
> -ifdef LIBTRACEEVENT_DIR
> -  LIBTRACEEVENT_CFLAGS  := -I$(LIBTRACEEVENT_DIR)/include
> -  LIBTRACEEVENT_LDFLAGS := -L$(LIBTRACEEVENT_DIR)/lib
> -endif
> -FEATURE_CHECK_CFLAGS-libtraceevent := $(LIBTRACEEVENT_CFLAGS)
> -FEATURE_CHECK_LDFLAGS-libtraceevent := $(LIBTRACEEVENT_LDFLAGS) $(TRACEEVENTLIBS)
> +# make DEBUG=1 PKG_CONFIG_PATH=/opt/libtraceevent/(lib|lib64)/pkgconfig
> +FEATURE_CHECK_CFLAGS-libtraceevent := $(shell $(PKG_CONFIG) --cflags libtraceevent)
> +FEATURE_CHECK_LDFLAGS-libtraceevent := $(shell $(PKG_CONFIG) --libs libtraceevent)
> +
> +FEATURE_CHECK_CFLAGS-libtracefs := $(shell $(PKG_CONFIG) --cflags libtracefs)
> +FEATURE_CHECK_LDFLAGS-libtracefs := $(shell $(PKG_CONFIG) --libs libtracefs)
>
>   FEATURE_CHECK_CFLAGS-bpf = -I. -I$(srctree)/tools/include -I$(srctree)/tools/arch/$(SRCARCH)/include/uapi -I$(srctree)/tools/include/uapi
>   # include ARCH specific config
> @@ -1178,10 +1176,10 @@ endif
>   ifneq ($(NO_LIBTRACEEVENT),1)
>     $(call feature_check,libtraceevent)
>     ifeq ($(feature-libtraceevent), 1)
> -    CFLAGS += -DHAVE_LIBTRACEEVENT $(LIBTRACEEVENT_CFLAGS)
> -    LDFLAGS += $(LIBTRACEEVENT_LDFLAGS)
> -    EXTLIBS += ${TRACEEVENTLIBS}
> -    LIBTRACEEVENT_VERSION := $(shell PKG_CONFIG_PATH=$(LIBTRACEEVENT_DIR) $(PKG_CONFIG) --modversion libtraceevent)
> +    CFLAGS += -DHAVE_LIBTRACEEVENT
> +    LDFLAGS += $(shell $(PKG_CONFIG) --libs-only-L libtraceevent)
> +    EXTLIBS += $(shell $(PKG_CONFIG) --libs-only-l libtraceevent)
> +    LIBTRACEEVENT_VERSION := $(shell $(PKG_CONFIG) --modversion libtraceevent)
>       LIBTRACEEVENT_VERSION_1 := $(word 1, $(subst ., ,$(LIBTRACEEVENT_VERSION)))
>       LIBTRACEEVENT_VERSION_2 := $(word 2, $(subst ., ,$(LIBTRACEEVENT_VERSION)))
>       LIBTRACEEVENT_VERSION_3 := $(word 3, $(subst ., ,$(LIBTRACEEVENT_VERSION)))
> @@ -1194,7 +1192,9 @@ ifneq ($(NO_LIBTRACEEVENT),1)
>
>     $(call feature_check,libtracefs)
>     ifeq ($(feature-libtracefs), 1)
> -    EXTLIBS += -ltracefs
> +    CFLAGS +=  $(shell $(PKG_CONFIG) --cflags libtracefs)
> +    LDFLAGS += $(shell $(PKG_CONFIG) --libs-only-L libtracefs)
> +    EXTLIBS += $(shell $(PKG_CONFIG) --libs-only-l libtracefs)
>       LIBTRACEFS_VERSION := $(shell $(PKG_CONFIG) --modversion libtracefs)
>       LIBTRACEFS_VERSION_1 := $(word 1, $(subst ., ,$(LIBTRACEFS_VERSION)))
>       LIBTRACEFS_VERSION_2 := $(word 2, $(subst ., ,$(LIBTRACEFS_VERSION)))
> @@ -1315,7 +1315,6 @@ ifeq ($(VF),1)
>     $(call print_var,LIBUNWIND_DIR)
>     $(call print_var,LIBDW_DIR)
>     $(call print_var,JDIR)
> -  $(call print_var,LIBTRACEEVENT_DIR)

Except above comment, the change looks good to me:

Reviewed-by: Leo Yan <leo.yan@arm.com>

>
>     ifeq ($(dwarf-post-unwind),1)
>       $(call feature_print_text,"DWARF post unwind library", $(dwarf-post-unwind-text)) $(info $(MSG))
> --
> 2.45.1
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.