Patchwork [discussion] perf: specify SLANG_INC dir for perf

login
register
mail settings
Submitter Liang Li
Date Aug. 21, 2012, 5:08 a.m.
Message ID <20120821050825.GB27917@localhost>
Download mbox | patch
Permalink /patch/35039/
State New
Headers show

Comments

Liang Li - Aug. 21, 2012, 5:08 a.m.
On 2012-08-20 22:48, Bruce Ashfield <bruce.ashfield@windriver.com> wrote:
> On 12-08-17 09:05 AM, Liang Li wrote:
> > On 2012-08-17 21:01, Liang Li<liang.li@windriver.com>  wrote:
> >> On 2012-08-17 18:53, Richard Purdie<richard.purdie@linuxfoundation.org>  wrote:
> >>> On Fri, 2012-08-17 at 18:00 +0800, Liang Li wrote:
> >>>> I am totally confused, you mentioned 'general kernel do_install', I
> >>>> assume it's oe-core kernel.bbclass concept. Then you mentioned 'get
> >>>> the fix upstream in the mainline kernel', how could that happen?
> >>>>
> >>>> We are discussing about the solution to 'fix the compile warning to
> >>>> error' stuff that triggered by the '-I/usr/include/slang', right?
> >>>
> >>> Yes.
> >>>
> >>>>   We do not necessarily have to change recipe to fix it since the issue
> >>>> is not introduced by the recipe, the hard coded '-I/usr/include/slang'
> >>>> in the Makefile cause the issue, we can fix the root cause by kernel
> >>>> patch(other than just comment the line out). I see your previous patch
> >>>> to kernel, by comment out the '-I/usr/include/slang' line in the
> >>>> Makefile, is the same behavior, but we won't have the change(comment
> >>>> out -I.. in Makefile) upstream to mainline, right?
> >>>
> >>> I am suggesting that firstly, someone send a patch to the mainline
> >>> kernel which changes -I/usr/include/slang to -I=/usr/include/slang in
> >>> that Makefile.
> >>>
> >>> Secondly, I'm suggesting that we add a line to kernel_do_install() in
> >>> kernel.bbclass which does a sed on the Makefile as installed into
> >>> $kerneldir which changes -I/usr/include/slang to -I=/usr/include/slang.
> >>>
> >>> We can then drop the patch I added to the linux-yocto kernels.
> >>>
> >>> This is all that should be needed, it should fix all the issues people
> >>> have reported in a way that is acceptable to everyone.
> >>>
> >>
> >> Ah, I see what you mean now. But we have push acceptable kernel patch
> >
> 
> One final (I hope) follow up on this.
> 
> Liang: were you going to put together (and test) the 'sed fix' for
> kernel.bbclass ?
> 

No problem, the patch for kernel.bbclass:

commit 60a0b06
Author: Liang Li <liang.li@windriver.com>
Date:   Tue Aug 21 11:06:01 2012 +0800

    kernel.bbclass: fix INC directory for SLANG
    
    The change is intend to fix the hardcoded '-I/usr/include/slang' in
    the Makefile to be able to aware of SYSROOT if its specified.
    
    A planned kernel patch almost did the same change, but the change here
    won't conflict with it so this change could work for all kernels.
    
    Signed-off-by: Liang Li <liang.li@windriver.com>


---

Comments? :)

Thanks,
		Liang Li

> I have my own set of issues that are consuming my time now, and I want
> to ensure that this doesn't fall through the cracks, since we need a
> full/real fix for this as soon as possible.
> 
> Cheers,
> 
> Bruce
> 
> 
> > Sorry, I mean 'we can ...' instead of 'we have ...', just typo.
Henning Heinold - Aug. 21, 2012, 8:51 a.m.
On Tue, Aug 21, 2012 at 01:08:25PM +0800, Liang Li wrote:
> --- a/meta/classes/kernel.bbclass
> +++ b/meta/classes/kernel.bbclass
> @@ -190,6 +190,9 @@ kernel_do_install() {
>  	for entry in $bin_files; do
>  	        rm -f $kerneldir/$entry
>  	done
> +
> +	# Fix SLNAG_INC for slang.h
> +	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
Hm,

is this "-I=" a new special feature of gcc?

-I/usr/include/slang
-I=/usr/include/slang

Bye Henning
Liang Li - Aug. 21, 2012, 9:19 a.m.
On 2012-08-21 16:51, Henning Heinold <heinold@inf.fu-berlin.de> wrote:
> On Tue, Aug 21, 2012 at 01:08:25PM +0800, Liang Li wrote:
> > --- a/meta/classes/kernel.bbclass
> > +++ b/meta/classes/kernel.bbclass
> > @@ -190,6 +190,9 @@ kernel_do_install() {
> >  	for entry in $bin_files; do
> >  	        rm -f $kerneldir/$entry
> >  	done
> > +
> > +	# Fix SLNAG_INC for slang.h
> > +	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
> Hm,
> 
> is this "-I=" a new special feature of gcc?
> 
> -I/usr/include/slang
> -I=/usr/include/slang
> 

Not something new/special feature of gcc, should be as old as
--sysroot, maybe more than 10 years elder I guess.

Cheers,
		Liang Li

> Bye Henning
Richard Purdie - Aug. 21, 2012, 10:40 a.m.
On Tue, 2012-08-21 at 10:51 +0200, Henning Heinold wrote:
> On Tue, Aug 21, 2012 at 01:08:25PM +0800, Liang Li wrote:
> > --- a/meta/classes/kernel.bbclass
> > +++ b/meta/classes/kernel.bbclass
> > @@ -190,6 +190,9 @@ kernel_do_install() {
> >  	for entry in $bin_files; do
> >  	        rm -f $kerneldir/$entry
> >  	done
> > +
> > +	# Fix SLNAG_INC for slang.h
> > +	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
> Hm,
> 
> is this "-I=" a new special feature of gcc?
> 
> -I/usr/include/slang
> -I=/usr/include/slang

It means the path is relative to any sysroot. It wasn't around for the
first implementations of sysroot but came shortly thereafter. As long as
its gcc 3.4 onwards it should be fine iirc.

Cheers,

Richard
Bruce Ashfield - Aug. 21, 2012, 1:07 p.m.
On 12-08-21 01:08 AM, Liang Li wrote:
> On 2012-08-20 22:48, Bruce Ashfield<bruce.ashfield@windriver.com>  wrote:
>> On 12-08-17 09:05 AM, Liang Li wrote:
>>> On 2012-08-17 21:01, Liang Li<liang.li@windriver.com>   wrote:
>>>> On 2012-08-17 18:53, Richard Purdie<richard.purdie@linuxfoundation.org>   wrote:
>>>>> On Fri, 2012-08-17 at 18:00 +0800, Liang Li wrote:
>>>>>> I am totally confused, you mentioned 'general kernel do_install', I
>>>>>> assume it's oe-core kernel.bbclass concept. Then you mentioned 'get
>>>>>> the fix upstream in the mainline kernel', how could that happen?
>>>>>>
>>>>>> We are discussing about the solution to 'fix the compile warning to
>>>>>> error' stuff that triggered by the '-I/usr/include/slang', right?
>>>>>
>>>>> Yes.
>>>>>
>>>>>>    We do not necessarily have to change recipe to fix it since the issue
>>>>>> is not introduced by the recipe, the hard coded '-I/usr/include/slang'
>>>>>> in the Makefile cause the issue, we can fix the root cause by kernel
>>>>>> patch(other than just comment the line out). I see your previous patch
>>>>>> to kernel, by comment out the '-I/usr/include/slang' line in the
>>>>>> Makefile, is the same behavior, but we won't have the change(comment
>>>>>> out -I.. in Makefile) upstream to mainline, right?
>>>>>
>>>>> I am suggesting that firstly, someone send a patch to the mainline
>>>>> kernel which changes -I/usr/include/slang to -I=/usr/include/slang in
>>>>> that Makefile.
>>>>>
>>>>> Secondly, I'm suggesting that we add a line to kernel_do_install() in
>>>>> kernel.bbclass which does a sed on the Makefile as installed into
>>>>> $kerneldir which changes -I/usr/include/slang to -I=/usr/include/slang.
>>>>>
>>>>> We can then drop the patch I added to the linux-yocto kernels.
>>>>>
>>>>> This is all that should be needed, it should fix all the issues people
>>>>> have reported in a way that is acceptable to everyone.
>>>>>
>>>>
>>>> Ah, I see what you mean now. But we have push acceptable kernel patch
>>>
>>
>> One final (I hope) follow up on this.
>>
>> Liang: were you going to put together (and test) the 'sed fix' for
>> kernel.bbclass ?
>>
>
> No problem, the patch for kernel.bbclass:
>
> commit 60a0b06
> Author: Liang Li<liang.li@windriver.com>
> Date:   Tue Aug 21 11:06:01 2012 +0800
>
>      kernel.bbclass: fix INC directory for SLANG
>
>      The change is intend to fix the hardcoded '-I/usr/include/slang' in
>      the Makefile to be able to aware of SYSROOT if its specified.
>
>      A planned kernel patch almost did the same change, but the change here
>      won't conflict with it so this change could work for all kernels.
>
>      Signed-off-by: Liang Li<liang.li@windriver.com>
>
> diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
> index 1afb9ab..282194d 100644
> --- a/meta/classes/kernel.bbclass
> +++ b/meta/classes/kernel.bbclass
> @@ -190,6 +190,9 @@ kernel_do_install() {
>   	for entry in $bin_files; do
>   	        rm -f $kerneldir/$entry
>   	done
> +
> +	# Fix SLNAG_INC for slang.h

s/SLNAG_INC/SLANG_INC/

> +	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile

It looks like your baseline for this patch is the denzil branch. We'd
want a version for master, which we could backport as required.

>   }
>
>   PACKAGE_PREPROCESS_FUNCS += "kernel_package_preprocess"
>
> ---
>
> The patch for kernel tree:
>
> commit 6b72896
> Author: Liang Li<liang.li@windriver.com>
> Date:   Wed Aug 1 14:31:24 2012 +0800
>
>      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.

I'd rephrase this slightly as:

CFLAGS was previously hard coded to contain "-I/usr/include/slang" to
work with hosts that have "/usr/include/slang/slang.h" as well as hosts
that have "/usr/include/slang.h". This path can cause compile warnings
in some cases:

  <put the warnings here>

.. and indicate that these warnings can actually cause build errors if
WERROR is enabled.

>
>      We could downgrade the priority of the default hard coded path, and
>      provide user a chance to specify correct location of slang.h then user
>      could specify SLANG_INC to avoid compile warnings like the
>      '/usr/include/slang' is not exists etc.

Another minor rephrase:

To fix this issue, we can use -idirafter to downgrade the priority of the
default hard coded path. We can also make the slang include directory
a variable, to allow the user to specify SLANG_INC and set their own
include location.

>
>      Signed-off-by: Liang Li<liang.li@windriver.com>
>
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index b7a7a87..e403c36 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 ?= -idirafter =/usr/include/slang

One more question, have you confirmed that gcc is fine with this being
in sysroot notation ? (I assume it is .. but I need to ask.

Cheers,

Bruce

> +		BASIC_CFLAGS += $(SLANG_INC)
> +
>   		EXTLIBS += -lnewt -lslang
>   		LIB_OBJS += $(OUTPUT)ui/setup.o
>   		LIB_OBJS += $(OUTPUT)ui/browser.o
>
> ---
>
> Comments? :)
>
> Thanks,
> 		Liang Li
>
>> I have my own set of issues that are consuming my time now, and I want
>> to ensure that this doesn't fall through the cracks, since we need a
>> full/real fix for this as soon as possible.
>>
>> Cheers,
>>
>> Bruce
>>
>>
>>> Sorry, I mean 'we can ...' instead of 'we have ...', just typo.

Patch

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index 1afb9ab..282194d 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -190,6 +190,9 @@  kernel_do_install() {
 	for entry in $bin_files; do
 	        rm -f $kerneldir/$entry
 	done
+
+	# Fix SLNAG_INC for slang.h
+	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
 }
 
 PACKAGE_PREPROCESS_FUNCS += "kernel_package_preprocess"

---

The patch for kernel tree:

commit 6b72896
Author: Liang Li <liang.li@windriver.com>
Date:   Wed Aug 1 14:31:24 2012 +0800

    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 could downgrade the priority of the default hard coded path, and
    provide user a chance to specify correct location of slang.h then user
    could specify SLANG_INC to avoid compile warnings like the
    '/usr/include/slang' is not exists etc.
    
    Signed-off-by: Liang Li <liang.li@windriver.com>

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index b7a7a87..e403c36 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 ?= -idirafter =/usr/include/slang
+		BASIC_CFLAGS += $(SLANG_INC)
+
 		EXTLIBS += -lnewt -lslang
 		LIB_OBJS += $(OUTPUT)ui/setup.o
 		LIB_OBJS += $(OUTPUT)ui/browser.o