Re: [PATCH] perf build: Add missing cflags when building with custom libtraceevent

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Nov 05, 2024 at 10:06:34AM +0800, Yicong Yang wrote:
> On 2024/11/4 20:51, Guilherme Amadio wrote:
> > Hi Namhyung,
> > 
> > On Mon, Nov 04, 2024 at 10:47:41AM +0000, Leo Yan wrote:
> >> On Fri, Nov 01, 2024 at 11:23:27PM -0700, Namhyung Kim wrote:
> >>
> >> [...]
> >>
> >>>>> Checked how we handle with libtracefs currently:
> >>>>>  ifeq ($(feature-libtracefs), 1)
> >>>>>     CFLAGS +=  $(shell $(PKG_CONFIG) --cflags libtracefs) # we've added the cflags from pkg-config
> >>>>>     LDFLAGS += $(shell $(PKG_CONFIG) --libs-only-L libtracefs)
> >>>>
> >>>> The libtracefs.pc file claims requesting libtraceevent, as a result, it
> >>>> returns the header paths for both libtraceevent and libtracefs:
> >>>>
> >>>>   $ pkg-config --cflags libtracefs
> >>>>   -I/usr/local/include/tracefs -I/usr/local/include/traceevent
> >>>>
> >>>> I checked the packages libtraceevent-dev and libtracefs-dev and either
> >>>> can be installed independently. Therefore, it seems to me that this
> >>>> patch is correct for finding the traceevent header path, with no
> >>>> dependency on tracefs.
> >>>
> >>> I'm ok with the patch itself.  Can I get your Acked-by, Leo?
> >>
> >> Yes.
> >>
> >> Reviewed-by: Leo Yan <leo.yan@arm.com>
> > 
> > I agree that this was a mistake I made in my patch. I added the cflags
> > to the feature check in tools/build/Makefile.feature, but in Makefile.config
> > I added it only for libtracefs and missed it for libtraceevent. The fixes tag
> > is correct. This patch fixes my mistake.
> > 
> > Reviewed-by: Guilherme Amadio <amadio@gentoo.org>
> > 
> 
> Thanks for the review by Leo and Guilherme!
> 
> Hi Namhyung,
> 
> As you noticed we should use the include path from pkg-config [1]. Would you like
> to post a patch yourself or want me to post this as well? It's correct and we also
> need this. (I didn't found this may because I changed my custom libtraceevent.pc when
> debugging and currently approach is ok if libtraceevent's installed system wide).

Please do that as a follow-up.

Thanks,
Namhyung