Patchwork [discussion] perf: specify SLANG_INC dir for perf

login
mail settings
Submitter Liang Li
Date Aug. 17, 2012, 10 a.m.
Message ID <20120817100001.GA16621@localhost>
Download mbox | patch
Permalink /patch/34795/
State New
Headers show

Comments

Liang Li - Aug. 17, 2012, 10 a.m.
On 2012-08-17 17:35, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> On Fri, 2012-08-17 at 11:32 +0800, Liang Li wrote:
> > On 2012-08-16 23:58, 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?
> > > 
> > 
> > Good advise, so the kernel patch can be revised to:
> > 
> > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> > index 067f2df..071986c 100644
> > --- a/tools/perf/Makefile
> > +++ b/tools/perf/Makefile
> > @@ -497,7 +497,7 @@ else
> >  		BASIC_CFLAGS += -DNO_NEWT_SUPPORT
> >  	else
> >  		# Some releases like Fedora has /usr/include/slang/slang.h other than /usr/include/slang.h
> > -		SLANG_INC ?= -I/usr/include/slang
> > +		SLANG_INC ?= -idirafter /usr/include/slang
> >  		BASIC_CFLAGS += $(SLANG_INC)
> >  
> >  		EXTLIBS += -lnewt -lslang
> 
> Why do you need a kernel patch at all with the solution I mentioned
> above?
> 
> > With this, we might not have to touch perf.bb. So seems like we'll use
> > this solution? :)
> 
> You should need to add the sed to the general kernel do_install. We
> should also get this fixed upstream in the mainline kernel. We shouldn't
> need a kernel patch or any recipe change to perf.
> 

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? 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?

If you mean the logic in plan, the 'sed ..' stuff in do_install can be
push to upstream kernel, then what is the difference in your plan,
compared with my proposed change like the below:

commit c7e9626

    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>


---

If the change to kernel works, we do not have to change perf.bb,
do not have to patch kernel's do_install by 'sed..' and could send the
fix mainline etc.

Thanks,
		Liang Li

> Cheers,
> 
> Richard
Richard Purdie - Aug. 17, 2012, 10:53 a.m.
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.

Cheers,

Richard
Bruce Ashfield - Aug. 17, 2012, 12:55 p.m.
On 12-08-17 06:53 AM, Richard Purdie 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.

This is what I was shooting for as well, the oe-core fix is separate
from the upstream kernel fix. We'll fix our builds first, and then
work for a solution in perf upstream.

My only requirement was that our sed fix still work when kernels start
appearing with it fixed upstream .. something that shouldn't be hard
to do.

Cheers,

Bruce

>
> Cheers,
>
> Richard
>
>

Patch

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index b7a7a87..071986c 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