Patchwork [discussion] perf: specify SLANG_INC dir for perf

login
register
mail settings
Submitter Liang Li
Date Aug. 14, 2012, 2:17 a.m.
Message ID <20120814021712.GB25748@localhost>
Download mbox | patch
Permalink /patch/34457/
State New
Headers show

Comments

Liang Li - Aug. 14, 2012, 2:17 a.m.
Hi Richard,

Ping ...

Hopefully you could recall sufficient context from this thread about
the 'include path for slang.h' cause compile error issue that we are
trying to fix here.

I saw your three commits in oecore like below:

commit b033000
Author: Richard Purdie <richard.purdie@linuxfoundation.org>
Date:   Tue Aug 7 22:21:38 2012 +0000

    linux-yocto-3.2: Apply slang workaround fixing perf builds to 3.2 kernels too
    
    Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>


---

I see benefits in above solution:

- no out of tree kernel patch to tweak kernel source code
- better semantic to indicate what is done

---

May you please give some advice to above solution.

Thanks,
		Liang Li

On 2012-08-08 11:37, Liang Li <liang.li@windriver.com> wrote:
> On 2012-08-07 22:02, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > On Fri, 2012-08-03 at 23:43 +0800, Liang Li wrote:
> > > Via EXTRA_CFLAGS, we can pass the sysroot include directory to perf to
> > > provide slang.h rather than hardcoded host dir in perf's Makefile.
> > > 
> > > Pass WERROR=0 to perf's Makefile to avoid warnings being treated
> > > as errors. Warnings are not fatal, and while they will be fixed in the
> > > future, there's no need for them to break the build.
> > 
> > No mention of the additional slang dependency is made here?
> > 
> 
> Forgot mentioned it. Good catch, but the one line change that add
> slang to DEPENDS seems clear enough for everyone, isn't? :)
> 
> > > Signed-off-by: Liang Li <liang.li@windriver.com>
> > > ---
> > >  meta/recipes-kernel/perf/perf_3.4.bb | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/meta/recipes-kernel/perf/perf_3.4.bb b/meta/recipes-kernel/perf/perf_3.4.bb
> > > index 505c7b8..537e926 100644
> > > --- a/meta/recipes-kernel/perf/perf_3.4.bb
> > > +++ b/meta/recipes-kernel/perf/perf_3.4.bb
> > > @@ -24,6 +24,7 @@ DEPENDS = "virtual/kernel \
> > >             ${MLPREFIX}binutils \
> > >             ${TUI_DEPENDS} \
> > >             ${SCRIPTING_DEPENDS} \
> > > +           slang \
> > >            "
> > >  
> > >  SCRIPTING_RDEPENDS = "${@perf_feature_enabled('perf-scripting', 'perl perl-modules python', '',d)}"
> > > @@ -63,6 +64,8 @@ EXTRA_OEMAKE = \
> > >  		AR="${AR}" \
> > >  		prefix=/usr \
> > >  		NO_GTK2=1 ${TUI_DEFINES} NO_DWARF=1 ${SCRIPTING_DEFINES} \
> > > +		WERROR=0 \
> > > +		EXTRA_CFLAGS=-I${STAGING_INCDIR} \
> > >  		'
> > 
> > This is is not acceptable since the include directory /usr/include/slang
> > is still being looked at and this just "hides" the error. STAGING_INCDIR
> > is on the compilers default search path anyway.
> 
> EXTRA_CFLAGS is processed early than the '-I/usr/include/slang', so
> its being used here, to specify a path for slang.h. That indeed
> 'hides' -I/usr/include/slang. And I agree that specify
> -I/usr/include/slang blindly in Makefile should be fixed. And I've
> discussed the fix for kernel tree several days before. However, the
> fix for kernel tree will not conflict than this patch, I would think
> that this patch for perf.bb would fix the issue for us for now, and
> smooth the adoption of future fix for the Makefile.
> 
> > 
> > So this patch is wrong in several different ways :(
> > 
> 
> ... I can't reproduce the 'warning -> error' on my host now, but as I
> explained above, this patch is just 'make use of existing mechanism of
> Makefile to specify correct location of slang.h before the warning is
> generated', even though the STAGING_INCDIR is in compilers default
> search path, seems no hurt in case we just make it float top a bit
> to get searched before wrong include directory is discovered, right?
> :)
> 
> > I've merged a temporary fix until we get this resolved properly.
> > 
> 
> I saw that, but the fix that we've discussed several days before seems
> is what we want:
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index b7a7a87..3365ad2 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -496,8 +496,10 @@ else
>  		msg := $(warning newt not found, disables TUI support. Please install newt-devel or libnewt-dev);
>  		BASIC_CFLAGS += -DNO_NEWT_SUPPORT
>  	else
> -		# Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
> -		BASIC_CFLAGS += -I/usr/include/slang
> +		# Some releases has /usr/include/slang/slang.h other than /usr/include/slang.h
> +		SLANG_INC ?= -I/usr/include/slang
> +		BASIC_CFLAGS += $(SLANG_INC)
> +
>  		EXTLIBS += -lnewt -lslang
>  		LIB_OBJS += $(OUTPUT)ui/setup.o
>  		LIB_OBJS += $(OUTPUT)ui/browser.o
> 
> ---
> 
> Then we still need a patch to perf.bb to specify SLANG_INC. Moreover,
> this patch(EXTRA_CFLAGS for perf.bb) could co-exists with patch for
> kernel. :)
> 
> Thanks,
> 		Liang Li
> 
> > Cheers,
> > 
> > Richard
Bruce Ashfield - Aug. 16, 2012, 3:33 p.m.
On 12-08-13 10:17 PM, Liang Li wrote:
> Hi Richard,
>
> Ping ...
>
> Hopefully you could recall sufficient context from this thread about
> the 'include path for slang.h' cause compile error issue that we are
> trying to fix here.

Bump.

I'm holding off on merging a kernel patch for this while this is still
outstanding.

Can I distill this into the following (in the hope of resolving it).

   - do we want to fix this problem for all kernels, or just the linux-yocto
     ones ? And by 'fix', I mean without the requirement of porting
     a kernel patch to older recipes.

Cheers,

Bruce

>
> I saw your three commits in oecore like below:
>
> commit b033000
> Author: Richard Purdie<richard.purdie@linuxfoundation.org>
> Date:   Tue Aug 7 22:21:38 2012 +0000
>
>      linux-yocto-3.2: Apply slang workaround fixing perf builds to 3.2 kernels too
>
>      Signed-off-by: Richard Purdie<richard.purdie@linuxfoundation.org>
>
> diff --git a/meta/recipes-kernel/linux/linux-yocto_3.2.bb b/meta/recipes-kernel/linux/linux-yocto_3.2.bb
> index de716da..b254251 100644
> --- a/meta/recipes-kernel/linux/linux-yocto_3.2.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto_3.2.bb
> @@ -24,6 +24,8 @@ KMETA = "meta"
>
>   SRC_URI = "git://git.yoctoproject.org/linux-yocto-3.2;protocol=git;bareclone=1;branch=${KBRANCH},meta;name=machine,meta"
>
> +SRC_URI += "file://noslang.patch"
> +
>   COMPATIBLE_MACHINE = "(qemuarm|qemux86|qemuppc|qemumips|qemux86-64)"
>
>   # Functionality flags
>
> commit 6b4ed64
> Author: Richard Purdie<richard.purdie@linuxfoundation.org>
> Date:   Tue Aug 7 22:21:22 2012 +0000
>
>      linux-yocto-3.0: Apply slang workaround fixing perf builds to 3.0 kernels too
>
>      Signed-off-by: Richard Purdie<richard.purdie@linuxfoundation.org>
>
> diff --git a/meta/recipes-kernel/linux/linux-yocto_3.0.bb b/meta/recipes-kernel/linux/linux-yocto_3.0.bb
> index 2adbc46..3022f21 100644
> --- a/meta/recipes-kernel/linux/linux-yocto_3.0.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto_3.0.bb
> @@ -24,6 +24,8 @@ PV = "${LINUX_VERSION}+git${SRCPV}"
>
>   SRC_URI = "git://git.yoctoproject.org/linux-yocto-3.0;protocol=git;bareclone=1;branch=${KBRANCH},meta;name=machine,meta"
>
> +SRC_URI += "file://noslang.patch"
> +
>   COMPATIBLE_MACHINE = "(qemuarm|qemux86|qemuppc|qemumips|qemux86-64)"
>
>   # Functionality flags
>
> commit 4fd4b2e
> Author: Richard Purdie<richard.purdie@linuxfoundation.org>
> Date:   Tue Aug 7 12:17:16 2012 +0100
>
>      linux-yocto-3.4: Disable extra slang header search path
>
>      Add in a workaround to avoid host infection detection build failures
>      from the slang include directory in perf. I'll defer to Bruce to
>      fix this properly but we need a workaround now as this is breaking
>      builds.
>
>      Signed-off-by: Richard Purdie<richard.purdie@linuxfoundation.org>
>
> diff --git a/meta/recipes-kernel/linux/linux-yocto/noslang.patch b/meta/recipes-kernel/linux/linux-yocto/noslang.patch
> new file mode 100644
> index 0000000..9cada34
> --- /dev/null
> +++ b/meta/recipes-kernel/linux/linux-yocto/noslang.patch
> @@ -0,0 +1,20 @@
> +We (OE) install slang into /usr/include so we never need to look into
> +/usr/include/slang/. We never want to look into a hardcoded path like this
> +since it triggers host infection issues. For now, simply remove this
> +since it causes us problems.
> +
> +Upstream-Status: Pending (would need rework)
> +
> +Index: tools/perf/Makefile
> +===================================================================
> +--- linux.orig/tools/perf/Makefile	2012-08-07 10:29:43.020149620 +0000
> ++++ linux/tools/perf/Makefile	2012-08-07 10:30:08.128148098 +0000
> +@@ -504,7 +504,7 @@
> + 		BASIC_CFLAGS += -DNO_NEWT_SUPPORT
> + 	else
> + 		# Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
> +-		BASIC_CFLAGS += -I/usr/include/slang
> ++		# BASIC_CFLAGS += -I/usr/include/slang
> + 		EXTLIBS += -lnewt -lslang
> + 		LIB_OBJS += $(OUTPUT)util/ui/setup.o
> + 		LIB_OBJS += $(OUTPUT)util/ui/browser.o
> diff --git a/meta/recipes-kernel/linux/linux-yocto_3.4.bb b/meta/recipes-kernel/linux/linux-yocto_3.4.bb
> index 48333b3..5ab46b7 100644
> --- a/meta/recipes-kernel/linux/linux-yocto_3.4.bb
> +++ b/meta/recipes-kernel/linux/linux-yocto_3.4.bb
> @@ -20,6 +20,8 @@ SRCREV_meta ?= "7ff48aa47c50b6455d60ca93bc81260ce8fe1a1b"
>
>   SRC_URI = "git://git.yoctoproject.org/linux-yocto-3.4.git;protocol=git;nocheckout=1;branch=${KBRANCH},meta;name=machine,meta"
>
> +SRC_URI += "file://noslang.patch"
> +
>   LINUX_VERSION ?= "3.4.6"
>
>   PR = "${INC_PR}.0"
>
> ---
>
> Since you mentioned 'workaround' in commit log, I would like to submit
> another solution:
>
> #1. Merge below kernel patch to kernel tree:
>
>  From 7708f74d98e7233c7257b055eea0ffb914f4ce2c Mon Sep 17 00:00:00 2001
> From: Liang Li<liang.li@windriver.com>
> Date: Wed, 1 Aug 2012 14:31:24 +0800
> Subject: [PATCH] perf: add SLANG_INC for slang.h
>
> Previously we hard code '-I/usr/include/slang' to CFLAGS to works with
> some hosts that has /usr/include/slang/slang.h other than
> /usr/include/slang.h like Fedora. This will cause compiling warnings
> in some cases.
>
> We'd better to provide user a chance to specify correct location of
> slang.h then user could specify SLANG_INC to avoid compile warnings.
>
> Signed-off-by: Liang Li<liang.li@windriver.com>
> ---
>   tools/perf/Makefile | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index b7a7a87..067f2df 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -496,8 +496,10 @@ else
>   		msg := $(warning newt not found, disables TUI support. Please install newt-devel or libnewt-dev);
>   		BASIC_CFLAGS += -DNO_NEWT_SUPPORT
>   	else
> -		# Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
> -		BASIC_CFLAGS += -I/usr/include/slang
> +		# Some releases like Fedora has /usr/include/slang/slang.h other than /usr/include/slang.h
> +		SLANG_INC ?= -I/usr/include/slang
> +		BASIC_CFLAGS += $(SLANG_INC)
> +
>   		EXTLIBS += -lnewt -lslang
>   		LIB_OBJS += $(OUTPUT)ui/setup.o
>   		LIB_OBJS += $(OUTPUT)ui/browser.o
Richard Purdie - Aug. 16, 2012, 3:58 p.m.
On Thu, 2012-08-16 at 11:33 -0400, Bruce Ashfield wrote:
> On 12-08-13 10:17 PM, Liang Li wrote:
> > Hi Richard,
> >
> > Ping ...
> >
> > Hopefully you could recall sufficient context from this thread about
> > the 'include path for slang.h' cause compile error issue that we are
> > trying to fix here.
> 
> Bump.
> 
> I'm holding off on merging a kernel patch for this while this is still
> outstanding.
> 
> Can I distill this into the following (in the hope of resolving it).
> 
>    - do we want to fix this problem for all kernels, or just the linux-yocto
>      ones ? And by 'fix', I mean without the requirement of porting
>      a kernel patch to older recipes.

I propose we add a sed expression to the general kernel do_install which
changes the -I/usr/include/slang -> -I=/usr/include/slang. That should
be generic, acceptable to upstream and fixes all kernel versions.

Comments?

Cheers,

Richard
Bruce Ashfield - Aug. 16, 2012, 4 p.m.
On 12-08-16 11:58 AM, Richard Purdie wrote:
> On Thu, 2012-08-16 at 11:33 -0400, Bruce Ashfield wrote:
>> On 12-08-13 10:17 PM, Liang Li wrote:
>>> Hi Richard,
>>>
>>> Ping ...
>>>
>>> Hopefully you could recall sufficient context from this thread about
>>> the 'include path for slang.h' cause compile error issue that we are
>>> trying to fix here.
>>
>> Bump.
>>
>> I'm holding off on merging a kernel patch for this while this is still
>> outstanding.
>>
>> Can I distill this into the following (in the hope of resolving it).
>>
>>     - do we want to fix this problem for all kernels, or just the linux-yocto
>>       ones ? And by 'fix', I mean without the requirement of porting
>>       a kernel patch to older recipes.
>
> I propose we add a sed expression to the general kernel do_install which
> changes the -I/usr/include/slang ->  -I=/usr/include/slang. That should
> be generic, acceptable to upstream and fixes all kernel versions.

That'll work as well, as long as it is captured in the staging directory
perf will build.

As long as we construct it to check before sed'ing (is that a verb), it'll
adapt to kernels that do and don't have a patch already.

Liang and I can take care of putting something together. Does that work
for you ?

Cheers,

Bruce

>
> Comments?
>
> Cheers,
>
> Richard
>
McClintock Matthew-B29882 - Aug. 16, 2012, 4:12 p.m.
On Thu, Aug 16, 2012 at 10:58 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Thu, 2012-08-16 at 11:33 -0400, Bruce Ashfield wrote:
>> On 12-08-13 10:17 PM, Liang Li wrote:
>> > Hi Richard,
>> >
>> > Ping ...
>> >
>> > Hopefully you could recall sufficient context from this thread about
>> > the 'include path for slang.h' cause compile error issue that we are
>> > trying to fix here.
>>
>> Bump.
>>
>> I'm holding off on merging a kernel patch for this while this is still
>> outstanding.
>>
>> Can I distill this into the following (in the hope of resolving it).
>>
>>    - do we want to fix this problem for all kernels, or just the linux-yocto
>>      ones ? And by 'fix', I mean without the requirement of porting
>>      a kernel patch to older recipes.
>
> I propose we add a sed expression to the general kernel do_install which
> changes the -I/usr/include/slang -> -I=/usr/include/slang. That should
> be generic, acceptable to upstream and fixes all kernel versions.
>
> Comments?

That would work for me as well ;)

-M

Patch

diff --git a/meta/recipes-kernel/linux/linux-yocto_3.2.bb b/meta/recipes-kernel/linux/linux-yocto_3.2.bb
index de716da..b254251 100644
--- a/meta/recipes-kernel/linux/linux-yocto_3.2.bb
+++ b/meta/recipes-kernel/linux/linux-yocto_3.2.bb
@@ -24,6 +24,8 @@  KMETA = "meta"
 
 SRC_URI = "git://git.yoctoproject.org/linux-yocto-3.2;protocol=git;bareclone=1;branch=${KBRANCH},meta;name=machine,meta"
 
+SRC_URI += "file://noslang.patch"
+
 COMPATIBLE_MACHINE = "(qemuarm|qemux86|qemuppc|qemumips|qemux86-64)"
 
 # Functionality flags

commit 6b4ed64
Author: Richard Purdie <richard.purdie@linuxfoundation.org>
Date:   Tue Aug 7 22:21:22 2012 +0000

    linux-yocto-3.0: Apply slang workaround fixing perf builds to 3.0 kernels too
    
    Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

diff --git a/meta/recipes-kernel/linux/linux-yocto_3.0.bb b/meta/recipes-kernel/linux/linux-yocto_3.0.bb
index 2adbc46..3022f21 100644
--- a/meta/recipes-kernel/linux/linux-yocto_3.0.bb
+++ b/meta/recipes-kernel/linux/linux-yocto_3.0.bb
@@ -24,6 +24,8 @@  PV = "${LINUX_VERSION}+git${SRCPV}"
 
 SRC_URI = "git://git.yoctoproject.org/linux-yocto-3.0;protocol=git;bareclone=1;branch=${KBRANCH},meta;name=machine,meta"
 
+SRC_URI += "file://noslang.patch"
+
 COMPATIBLE_MACHINE = "(qemuarm|qemux86|qemuppc|qemumips|qemux86-64)"
 
 # Functionality flags

commit 4fd4b2e
Author: Richard Purdie <richard.purdie@linuxfoundation.org>
Date:   Tue Aug 7 12:17:16 2012 +0100

    linux-yocto-3.4: Disable extra slang header search path
    
    Add in a workaround to avoid host infection detection build failures
    from the slang include directory in perf. I'll defer to Bruce to
    fix this properly but we need a workaround now as this is breaking
    builds.
    
    Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

diff --git a/meta/recipes-kernel/linux/linux-yocto/noslang.patch b/meta/recipes-kernel/linux/linux-yocto/noslang.patch
new file mode 100644
index 0000000..9cada34
--- /dev/null
+++ b/meta/recipes-kernel/linux/linux-yocto/noslang.patch
@@ -0,0 +1,20 @@ 
+We (OE) install slang into /usr/include so we never need to look into 
+/usr/include/slang/. We never want to look into a hardcoded path like this
+since it triggers host infection issues. For now, simply remove this
+since it causes us problems.
+
+Upstream-Status: Pending (would need rework)
+
+Index: tools/perf/Makefile
+===================================================================
+--- linux.orig/tools/perf/Makefile	2012-08-07 10:29:43.020149620 +0000
++++ linux/tools/perf/Makefile	2012-08-07 10:30:08.128148098 +0000
+@@ -504,7 +504,7 @@
+ 		BASIC_CFLAGS += -DNO_NEWT_SUPPORT
+ 	else
+ 		# Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
+-		BASIC_CFLAGS += -I/usr/include/slang
++		# BASIC_CFLAGS += -I/usr/include/slang
+ 		EXTLIBS += -lnewt -lslang
+ 		LIB_OBJS += $(OUTPUT)util/ui/setup.o
+ 		LIB_OBJS += $(OUTPUT)util/ui/browser.o
diff --git a/meta/recipes-kernel/linux/linux-yocto_3.4.bb b/meta/recipes-kernel/linux/linux-yocto_3.4.bb
index 48333b3..5ab46b7 100644
--- a/meta/recipes-kernel/linux/linux-yocto_3.4.bb
+++ b/meta/recipes-kernel/linux/linux-yocto_3.4.bb
@@ -20,6 +20,8 @@  SRCREV_meta ?= "7ff48aa47c50b6455d60ca93bc81260ce8fe1a1b"
 
 SRC_URI = "git://git.yoctoproject.org/linux-yocto-3.4.git;protocol=git;nocheckout=1;branch=${KBRANCH},meta;name=machine,meta"
 
+SRC_URI += "file://noslang.patch"
+
 LINUX_VERSION ?= "3.4.6"
 
 PR = "${INC_PR}.0"

---

Since you mentioned 'workaround' in commit log, I would like to submit
another solution:

#1. Merge below kernel patch to kernel tree:

From 7708f74d98e7233c7257b055eea0ffb914f4ce2c Mon Sep 17 00:00:00 2001
From: Liang Li <liang.li@windriver.com>
Date: Wed, 1 Aug 2012 14:31:24 +0800
Subject: [PATCH] perf: add SLANG_INC for slang.h

Previously we hard code '-I/usr/include/slang' to CFLAGS to works with
some hosts that has /usr/include/slang/slang.h other than
/usr/include/slang.h like Fedora. This will cause compiling warnings
in some cases.

We'd better to provide user a chance to specify correct location of
slang.h then user could specify SLANG_INC to avoid compile warnings.

Signed-off-by: Liang Li <liang.li@windriver.com>
---
 tools/perf/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index b7a7a87..067f2df 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -496,8 +496,10 @@  else
 		msg := $(warning newt not found, disables TUI support. Please install newt-devel or libnewt-dev);
 		BASIC_CFLAGS += -DNO_NEWT_SUPPORT
 	else
-		# Fedora has /usr/include/slang/slang.h, but ubuntu /usr/include/slang.h
-		BASIC_CFLAGS += -I/usr/include/slang
+		# Some releases like Fedora has /usr/include/slang/slang.h other than /usr/include/slang.h
+		SLANG_INC ?= -I/usr/include/slang
+		BASIC_CFLAGS += $(SLANG_INC)
+
 		EXTLIBS += -lnewt -lslang
 		LIB_OBJS += $(OUTPUT)ui/setup.o
 		LIB_OBJS += $(OUTPUT)ui/browser.o
-- 

#2. Set SLANG_INC to a reasonable directory in perf.bb:

diff --git a/meta/recipes-kernel/perf/perf_3.4.bb b/meta/recipes-kernel/perf/perf_3.4.bb
index 9c07fb7..d6900f9 100644
--- a/meta/recipes-kernel/perf/perf_3.4.bb
+++ b/meta/recipes-kernel/perf/perf_3.4.bb
@@ -63,6 +63,7 @@  EXTRA_OEMAKE = \
 		AR="${AR}" \
 		prefix=/usr \
 		NO_GTK2=1 ${TUI_DEFINES} NO_DWARF=1 ${SCRIPTING_DEFINES} \
+		SLANG_INC=-I${STAGING_INCDIR} \
 		'
 
 do_compile() {