[PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Liang Li]

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.

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} \
 		'
 
 do_compile() {
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

Adding Richard.

Richard: did this patch not fix the problem for you ? .. it fixes it here, and
should replace the patch that you made earlier today .. this may have slipped
through the cracks.

I've never seen the perf slang failure, so I'm hesitant to take this
patch, apply
it and drop yours.

Is there anyway that someone seeing this problem can test this patch ?

Cheers,

Bruce

On Fri, Aug 3, 2012 at 11:43 AM, Liang Li <liang.li@windriver.com> 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.
>
> 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} \
>                 '
>
>  do_compile() {
> --
> 1.7.12.rc0.22.gcdd159b
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

On Tue, 2012-08-07 at 08:56 -0400, Bruce Ashfield wrote:
> Adding Richard.
> 
> Richard: did this patch not fix the problem for you ? .. it fixes it here, and
> should replace the patch that you made earlier today .. this may have slipped
> through the cracks.
> 
> I've never seen the perf slang failure, so I'm hesitant to take this
> patch, apply
> it and drop yours.
> 
> Is there anyway that someone seeing this problem can test this patch ?

Reproducing the problem is easy "sudo mkdir /usr/include/slang", then
build perf.

Cheers,

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

On Tue, Aug 7, 2012 at 10:12 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Tue, 2012-08-07 at 08:56 -0400, Bruce Ashfield wrote:
>> Adding Richard.
>>
>> Richard: did this patch not fix the problem for you ? .. it fixes it here, and
>> should replace the patch that you made earlier today .. this may have slipped
>> through the cracks.
>>
>> I've never seen the perf slang failure, so I'm hesitant to take this
>> patch, apply
>> it and drop yours.
>>
>> Is there anyway that someone seeing this problem can test this patch ?
>
> Reproducing the problem is easy "sudo mkdir /usr/include/slang", then
> build perf.

That's fine .. but if I take the existing patch, and drop the kernel
temp one, would
you take the change ? I'd rather not spend my time reproducing and fixing
something to generate a patch that won't be taken :)

Cheers,

Bruce

>
> Cheers,
>
> Richard
>
>
>



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

On Tue, 2012-08-07 at 10:19 -0400, Bruce Ashfield wrote:
> On Tue, Aug 7, 2012 at 10:12 AM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > On Tue, 2012-08-07 at 08:56 -0400, Bruce Ashfield wrote:
> >> Adding Richard.
> >>
> >> Richard: did this patch not fix the problem for you ? .. it fixes it here, and
> >> should replace the patch that you made earlier today .. this may have slipped
> >> through the cracks.
> >>
> >> I've never seen the perf slang failure, so I'm hesitant to take this
> >> patch, apply
> >> it and drop yours.
> >>
> >> Is there anyway that someone seeing this problem can test this patch ?
> >
> > Reproducing the problem is easy "sudo mkdir /usr/include/slang", then
> > build perf.
> 
> That's fine .. but if I take the existing patch, and drop the kernel
> temp one, would
> you take the change ? I'd rather not spend my time reproducing and fixing
> something to generate a patch that won't be taken :)

Which patch is "the existing patch"?

To be really clear, I'm not hacking around this issue, I do not
want /usr/include/slang in the compiler search path at all, ever.

Cheers,

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

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?

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

So this patch is wrong in several different ways :(

I've merged a temporary fix until we get this resolved properly.

Cheers,

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

On 12-08-07 10:02 AM, Richard Purdie 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?
>
>> 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.

But this makes it first, which is what we were going for. For
an external toolchain, this also might not be true.

>
> So this patch is wrong in several different ways :(
>
> I've merged a temporary fix until we get this resolved properly.

I think this is a point of view thing, since from here, this is a
better temporary fix than commenting out the path in the perf
Makefile, which will break standalone tree cases if I merge the patch
into the tree, and I don't want to go to a milestone freeze with
an out of tree patch.

So I'd ask again that we reconsider this patch, since what's in tree
has issues with other use cases.

Cheers,

Bruce

>
> Cheers,
>
> Richard
>

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

On Tue, 2012-08-07 at 10:07 -0400, Bruce Ashfield wrote:
> On 12-08-07 10:02 AM, Richard Purdie 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?
> >
> >> 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.
> 
> But this makes it first, which is what we were going for. For
> an external toolchain, this also might not be true.

You have an external toolchain which does not search the sysroot
usr/include directory? Really? ;-)

I accept we won't search usr/include/slang but we don't install slang in
that directory anyway so its irrelevant and that isn't the path you're
adding.

> > So this patch is wrong in several different ways :(
> >
> > I've merged a temporary fix until we get this resolved properly.
> 
> I think this is a point of view thing, since from here, this is a
> better temporary fix than commenting out the path in the perf
> Makefile, which will break standalone tree cases if I merge the patch
> into the tree, and I don't want to go to a milestone freeze with
> an out of tree patch.

I doubt it will actually break anything.

> So I'd ask again that we reconsider this patch, since what's in tree
> has issues with other use cases.

No, I'm flatly refusing it, sorry.

Cheers,

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

On 12-08-07 10:22 AM, Richard Purdie wrote:
> On Tue, 2012-08-07 at 10:07 -0400, Bruce Ashfield wrote:
>> On 12-08-07 10:02 AM, Richard Purdie 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?
>>>
>>>> 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.
>>
>> But this makes it first, which is what we were going for. For
>> an external toolchain, this also might not be true.
>
> You have an external toolchain which does not search the sysroot
> usr/include directory? Really? ;-)

I do. Trust me on this. I'm not talking about Wind River, I'm
not talking about oe-core or yocto here.

>
> I accept we won't search usr/include/slang but we don't install slang in
> that directory anyway so its irrelevant and that isn't the path you're
> adding.
>
>>> So this patch is wrong in several different ways :(
>>>
>>> I've merged a temporary fix until we get this resolved properly.
>>
>> I think this is a point of view thing, since from here, this is a
>> better temporary fix than commenting out the path in the perf
>> Makefile, which will break standalone tree cases if I merge the patch
>> into the tree, and I don't want to go to a milestone freeze with
>> an out of tree patch.
>
> I doubt it will actually break anything.

It will, and does if I merge the patch to the kernel tree where
it belongs. That is my point.

>
>> So I'd ask again that we reconsider this patch, since what's in tree
>> has issues with other use cases.
>
> No, I'm flatly refusing it, sorry.

That's your prerogative, but I dislike the kernel patch that you
did with the same flatness.

I'm not trying to raise a fuss here, but you have to see the
other point of view on this and acknowledge that the existing change
is just as unpalatable to me, as the recipe space one is to you.

In the end, I don't really care, but I just don't like the optics
of an unsigned, out of tree patch, that I'm not going to merge
to the kernel, and one that will have to sit where it is for M3.

Cheers,

Bruce

>
> Cheers,
>
> Richard
>

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

On Tue, 2012-08-07 at 10:29 -0400, Bruce Ashfield wrote:
> On 12-08-07 10:22 AM, Richard Purdie wrote:
> > On Tue, 2012-08-07 at 10:07 -0400, Bruce Ashfield wrote:
> >> On 12-08-07 10:02 AM, Richard Purdie 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?
> >>>
> >>>> 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.
> >>
> >> But this makes it first, which is what we were going for. For
> >> an external toolchain, this also might not be true.
> >
> > You have an external toolchain which does not search the sysroot
> > usr/include directory? Really? ;-)
> 
> I do. Trust me on this. I'm not talking about Wind River, I'm
> not talking about oe-core or yocto here.

Well, for such a toolchain, you need to pass STAGING_INCDIR into the
CFLAGS anyway and are likely doing so at a different higher level. I
don't see how this patch changes anything.

> > I accept we won't search usr/include/slang but we don't install slang in
> > that directory anyway so its irrelevant and that isn't the path you're
> > adding.
> >
> >>> So this patch is wrong in several different ways :(
> >>>
> >>> I've merged a temporary fix until we get this resolved properly.
> >>
> >> I think this is a point of view thing, since from here, this is a
> >> better temporary fix than commenting out the path in the perf
> >> Makefile, which will break standalone tree cases if I merge the patch
> >> into the tree, and I don't want to go to a milestone freeze with
> >> an out of tree patch.
> >
> > I doubt it will actually break anything.
> 
> It will, and does if I merge the patch to the kernel tree where
> it belongs. That is my point.

Can you please explain how it breaks? Previously all you've said it "it
might break". Now you say it does break but not how.

As I've explained, I can't see how it would do that as we should never
be looking in /usr/include/slang/.

I'm going to guess that this isn't an external toolchain problem and is
if you build the kernel standalone against your native system with no
cross compilation involved? Its likely a fedora system rather than
ubuntu/debian derived?

It would be nice to not have to guess at what the problem is though...

> >> So I'd ask again that we reconsider this patch, since what's in tree
> >> has issues with other use cases.
> >
> > No, I'm flatly refusing it, sorry.
> 
> That's your prerogative, but I dislike the kernel patch that you
> did with the same flatness.
> 
> I'm not trying to raise a fuss here, but you have to see the
> other point of view on this and acknowledge that the existing change
> is just as unpalatable to me, as the recipe space one is to you.

Well, I'm trying to illustrate how and why its a bad idea, you were just
talking about hypothetical issues with no details. If it does break
something, fine but at least please give some indication of what/how/why
and then we can discuss it.

> In the end, I don't really care, but I just don't like the optics
> of an unsigned, out of tree patch, that I'm not going to merge
> to the kernel, and one that will have to sit where it is for M3.

We had the autobuilder showing read for 5 days with radio silence and
admittedly some confusion over a patch status. What am I meant to do
given I'd really like to take the vacation I have planned this week?

Anyhow, lets see if we can resolve this. Can you answer my previous
question about whether changing this to -I=/usr/include/slang would
work?

Cheers,

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

On 12-08-07 10:44 AM, Richard Purdie wrote:
> On Tue, 2012-08-07 at 10:29 -0400, Bruce Ashfield wrote:
>> On 12-08-07 10:22 AM, Richard Purdie wrote:
>>> On Tue, 2012-08-07 at 10:07 -0400, Bruce Ashfield wrote:
>>>> On 12-08-07 10:02 AM, Richard Purdie 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?
>>>>>
>>>>>> 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.
>>>>
>>>> But this makes it first, which is what we were going for. For
>>>> an external toolchain, this also might not be true.
>>>
>>> You have an external toolchain which does not search the sysroot
>>> usr/include directory? Really? ;-)
>>
>> I do. Trust me on this. I'm not talking about Wind River, I'm
>> not talking about oe-core or yocto here.
>
> Well, for such a toolchain, you need to pass STAGING_INCDIR into the
> CFLAGS anyway and are likely doing so at a different higher level. I
> don't see how this patch changes anything.

I wasn't clear, I'm building the tree (and tools) standalone using
both a hand generated cross toolchain and the native toolchain. Neither
uses a sysroot, so if I merged that patch into the tree, then they
wouldn't find slang and the build breaks.

>
>>> I accept we won't search usr/include/slang but we don't install slang in
>>> that directory anyway so its irrelevant and that isn't the path you're
>>> adding.
>>>
>>>>> So this patch is wrong in several different ways :(
>>>>>
>>>>> I've merged a temporary fix until we get this resolved properly.
>>>>
>>>> I think this is a point of view thing, since from here, this is a
>>>> better temporary fix than commenting out the path in the perf
>>>> Makefile, which will break standalone tree cases if I merge the patch
>>>> into the tree, and I don't want to go to a milestone freeze with
>>>> an out of tree patch.
>>>
>>> I doubt it will actually break anything.
>>
>> It will, and does if I merge the patch to the kernel tree where
>> it belongs. That is my point.
>
> Can you please explain how it breaks? Previously all you've said it "it
> might break". Now you say it does break but not how.
>
> As I've explained, I can't see how it would do that as we should never
> be looking in /usr/include/slang/.
>
> I'm going to guess that this isn't an external toolchain problem and is
> if you build the kernel standalone against your native system with no
> cross compilation involved? Its likely a fedora system rather than
> ubuntu/debian derived?
>
> It would be nice to not have to guess at what the problem is though...

Sorry about that, 400 unread email have me moving quickly this morning
and not taking enough time to put all the detail into what I am
writing. I added that above. And yes, it is some older RHEL/Suse boxes
and other ones that are derived from non debian systems that give me
pain.

>
>>>> So I'd ask again that we reconsider this patch, since what's in tree
>>>> has issues with other use cases.
>>>
>>> No, I'm flatly refusing it, sorry.
>>
>> That's your prerogative, but I dislike the kernel patch that you
>> did with the same flatness.
>>
>> I'm not trying to raise a fuss here, but you have to see the
>> other point of view on this and acknowledge that the existing change
>> is just as unpalatable to me, as the recipe space one is to you.
>
> Well, I'm trying to illustrate how and why its a bad idea, you were just
> talking about hypothetical issues with no details. If it does break
> something, fine but at least please give some indication of what/how/why
> and then we can discuss it.
>
>> In the end, I don't really care, but I just don't like the optics
>> of an unsigned, out of tree patch, that I'm not going to merge
>> to the kernel, and one that will have to sit where it is for M3.
>
> We had the autobuilder showing read for 5 days with radio silence and
> admittedly some confusion over a patch status. What am I meant to do
> given I'd really like to take the vacation I have planned this week?

Sorry, I thought when Liang sent the patch the issue was resolved :(
We at least did. That's how it goes sometimes.

>
> Anyhow, lets see if we can resolve this. Can you answer my previous
> question about whether changing this to -I=/usr/include/slang would
> work?

It should, but there's a distinct lack of precedent for sysroot style
include paths in the kernel tools (behind the times), but in the end,
if it fixes the oe-core case, and doesn't break standalone builds
(the toolchain just has to no error on the syntax), it should be fine.

I've already asked that a full kernel fix be generated, but I don't think
it will make the deadline for your vacation, or M3, so merging a sysroot
based include is probably the reasonable middle ground that I'll end
on.

Cheers,

Bruce

>
> Cheers,
>
> Richard
>
>
>
>

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

On Tue, 2012-08-07 at 11:26 -0400, Bruce Ashfield wrote:
> I wasn't clear, I'm building the tree (and tools) standalone using
> both a hand generated cross toolchain and the native toolchain. Neither
> uses a sysroot, so if I merged that patch into the tree, then they
> wouldn't find slang and the build breaks.

I'm not sure you want that hand generated cross toolchain finding slang
in that location, but anyhow... :)

> > It would be nice to not have to guess at what the problem is though...
> 
> Sorry about that, 400 unread email have me moving quickly this morning
> and not taking enough time to put all the detail into what I am
> writing. I added that above. And yes, it is some older RHEL/Suse boxes
> and other ones that are derived from non debian systems that give me
> pain.

Ok, I understand the problem now.

> > We had the autobuilder showing read for 5 days with radio silence and
> > admittedly some confusion over a patch status. What am I meant to do
> > given I'd really like to take the vacation I have planned this week?
> 
> Sorry, I thought when Liang sent the patch the issue was resolved :(
> We at least did. That's how it goes sometimes.

Right, some of it is misunderstanding and bad timing etc. but I needed
to do something. I agree its suboptimal but its a better position for
OE-Core than we were in and we can recover from it.

> > Anyhow, lets see if we can resolve this. Can you answer my previous
> > question about whether changing this to -I=/usr/include/slang would
> > work?
> 
> It should, but there's a distinct lack of precedent for sysroot style
> include paths in the kernel tools (behind the times), but in the end,
> if it fixes the oe-core case, and doesn't break standalone builds
> (the toolchain just has to no error on the syntax), it should be fine.
> 
> I've already asked that a full kernel fix be generated, but I don't think
> it will make the deadline for your vacation, or M3, so merging a sysroot
> based include is probably the reasonable middle ground that I'll end
> on.

Lets try the sysroot prefix (=) approach then. The kernel is sparse on
these since it never actually has to deal with sysroots (or userspace)
often and is still learning in that area. I suspect this should be the
fix upstream might take.

Cheers,

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

On Tue, Aug 7, 2012 at 11:41 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Tue, 2012-08-07 at 11:26 -0400, Bruce Ashfield wrote:
>> I wasn't clear, I'm building the tree (and tools) standalone using
>> both a hand generated cross toolchain and the native toolchain. Neither
>> uses a sysroot, so if I merged that patch into the tree, then they
>> wouldn't find slang and the build breaks.
>
> I'm not sure you want that hand generated cross toolchain finding slang
> in that location, but anyhow... :)
>
>> > It would be nice to not have to guess at what the problem is though...
>>
>> Sorry about that, 400 unread email have me moving quickly this morning
>> and not taking enough time to put all the detail into what I am
>> writing. I added that above. And yes, it is some older RHEL/Suse boxes
>> and other ones that are derived from non debian systems that give me
>> pain.
>
> Ok, I understand the problem now.
>
>> > We had the autobuilder showing read for 5 days with radio silence and
>> > admittedly some confusion over a patch status. What am I meant to do
>> > given I'd really like to take the vacation I have planned this week?
>>
>> Sorry, I thought when Liang sent the patch the issue was resolved :(
>> We at least did. That's how it goes sometimes.
>
> Right, some of it is misunderstanding and bad timing etc. but I needed
> to do something. I agree its suboptimal but its a better position for
> OE-Core than we were in and we can recover from it.
>
>> > Anyhow, lets see if we can resolve this. Can you answer my previous
>> > question about whether changing this to -I=/usr/include/slang would
>> > work?
>>
>> It should, but there's a distinct lack of precedent for sysroot style
>> include paths in the kernel tools (behind the times), but in the end,
>> if it fixes the oe-core case, and doesn't break standalone builds
>> (the toolchain just has to no error on the syntax), it should be fine.
>>
>> I've already asked that a full kernel fix be generated, but I don't think
>> it will make the deadline for your vacation, or M3, so merging a sysroot
>> based include is probably the reasonable middle ground that I'll end
>> on.
>
> Lets try the sysroot prefix (=) approach then. The kernel is sparse on
> these since it never actually has to deal with sysroots (or userspace)
> often and is still learning in that area. I suspect this should be the
> fix upstream might take.

Agreed. And to close the loop, I'd prefer to leave what you merged as-is for
today, and by next week, have another pull request with an in tree and upstream
submitted patch to replace it.

Does that work for you ?

Bruce

>
> Cheers,
>
> Richard
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Richard Purdie]

On Tue, 2012-08-07 at 11:54 -0400, Bruce Ashfield wrote:
> On Tue, Aug 7, 2012 at 11:41 AM, Richard Purdie

> > Lets try the sysroot prefix (=) approach then. The kernel is sparse on
> > these since it never actually has to deal with sysroots (or userspace)
> > often and is still learning in that area. I suspect this should be the
> > fix upstream might take.
> 
> Agreed. And to close the loop, I'd prefer to leave what you merged as-is for
> today, and by next week, have another pull request with an in tree and upstream
> submitted patch to replace it.
> 
> Does that work for you ?

Yes, that works, thanks.

Richard

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Liang Li]

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

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Darren Hart]

On 08/07/2012 08:37 PM, Liang Li 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? :)

Nope, the patch header declares the intent of the patch.

--
Darren

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


-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Liang Li]

On 2012-08-09 08:36, Darren Hart <darren.hart@intel.com> wrote:
> On 08/07/2012 08:37 PM, Liang Li 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? :)
> 
> Nope, the patch header declares the intent of the patch.
> 

Can't agree with you anymore. :) But we don't list all changes(even
one line straight forward change) in patch header, don't we? :)

Thanks,
		Liang Li

> --
> Darren
> 
> > 
> >>> 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
> 
> 
> -- 
> Darren Hart
> Intel Open Source Technology Center
> Yocto Project - Linux Kernel

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Darren Hart]

On 08/08/2012 06:24 PM, Liang Li wrote:
> On 2012-08-09 08:36, Darren Hart <darren.hart@intel.com> wrote:
>> On 08/07/2012 08:37 PM, Liang Li 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? :)
>>
>> Nope, the patch header declares the intent of the patch.
>>
> 
> Can't agree with you anymore. :) But we don't list all changes(even
> one line straight forward change) in patch header, don't we? :)

OK, so we're getting off into the weeds here for a relatively minor
change. However, the answer to your question is YES we do list all the
changes in the patch header. We don't list them individually, but rather
group them functionally. Adding a dependency to a recipe is a very clear
example of a functional change that should be made explicit in the
change log in my opinion.

--
Darren

> 
> Thanks,
> 		Liang Li
> 
>> --
>> Darren
>>
>>>
>>>>> 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
>>
>>
>> -- 
>> Darren Hart
>> Intel Open Source Technology Center
>> Yocto Project - Linux Kernel


-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Liang Li]

On 2012-08-09 09:41, Darren Hart <darren.hart@intel.com> wrote:
> On 08/08/2012 06:24 PM, Liang Li wrote:
> > On 2012-08-09 08:36, Darren Hart <darren.hart@intel.com> wrote:
> >> On 08/07/2012 08:37 PM, Liang Li 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? :)
> >>
> >> Nope, the patch header declares the intent of the patch.
> >>
> > 
> > Can't agree with you anymore. :) But we don't list all changes(even
> > one line straight forward change) in patch header, don't we? :)
> 
> OK, so we're getting off into the weeds here for a relatively minor
> change. However, the answer to your question is YES we do list all the
> changes in the patch header. We don't list them individually, but rather
> group them functionally.

Still agree. Yes, we did this all along the time, no confusion here.
Then you think the one line change in this patch, to add slang to
DEPENDS, is functional, *should* be listed out in patch header
explicitly. That is our deviation here, I thought that is
straightforward, and the patch is *mainly* about *fix* an issue other
than add a function, the one line change is just part of fix so I was
mainly focus on explain intention of the fix. :)

> Adding a dependency to a recipe is a very clear
> example of a functional change that should be made explicit in the
> change log in my opinion.
> 

No problem, add it to patch header would be fine to me, but here let's
focusing on 'do we need changes in this patch'.

Thanks,
		Liang Li

> --
> Darren
> 
> > 
> > Thanks,
> > 		Liang Li
> > 
> >> --
> >> Darren
> >>
> >>>
> >>>>> 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
> >>
> >>
> >> -- 
> >> Darren Hart
> >> Intel Open Source Technology Center
> >> Yocto Project - Linux Kernel
> 
> 
> -- 
> Darren Hart
> Intel Open Source Technology Center
> Yocto Project - Linux Kernel

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Darren Hart]

On 08/08/2012 06:52 PM, Liang Li wrote:
> On 2012-08-09 09:41, Darren Hart <darren.hart@intel.com> wrote:
>> On 08/08/2012 06:24 PM, Liang Li wrote:
>>> On 2012-08-09 08:36, Darren Hart <darren.hart@intel.com> wrote:
>>>> On 08/07/2012 08:37 PM, Liang Li 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? :)
>>>>
>>>> Nope, the patch header declares the intent of the patch.
>>>>
>>>
>>> Can't agree with you anymore. :) But we don't list all changes(even
>>> one line straight forward change) in patch header, don't we? :)
>>
>> OK, so we're getting off into the weeds here for a relatively minor
>> change. However, the answer to your question is YES we do list all the
>> changes in the patch header. We don't list them individually, but rather
>> group them functionally.
> 
> Still agree. Yes, we did this all along the time, no confusion here.
> Then you think the one line change in this patch, to add slang to
> DEPENDS, is functional, *should* be listed out in patch header
> explicitly. That is our deviation here, I thought that is
> straightforward, and the patch is *mainly* about *fix* an issue other
> than add a function, the one line change is just part of fix so I was
> mainly focus on explain intention of the fix. :)
> 
>> Adding a dependency to a recipe is a very clear
>> example of a functional change that should be made explicit in the
>> change log in my opinion.
>>
> 
> No problem, add it to patch header would be fine to me, but here let's
> focusing on 'do we need changes in this patch'.

As for me, I am happy with the patch content itself.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH] perf: pass STAGING_INCDIR(sysroot) to perf

[Bruce Ashfield]

On Wed, Aug 8, 2012 at 8:36 PM, Darren Hart <darren.hart@intel.com> wrote:
> On 08/07/2012 08:37 PM, Liang Li 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? :)
>
> Nope, the patch header declares the intent of the patch.

I think the point that Liang is trying to make is that the contents of
the patch
are trying to fix a problem in perf, and that he'd prefer to focus on
the technical
issues in the body of the patch, since the header cleanup is simple enough and
something that he's used to doing when dealing with me ;)

We will make sure that the header is correct, and that the important parts are
mentioned.

Cheers,

Bruce

>
> --
> Darren
>
>>
>>>> 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
>
>
> --
> Darren Hart
> Intel Open Source Technology Center
> Yocto Project - Linux Kernel
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end"

[discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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>

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() {

---

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Bruce Ashfield]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Richard Purdie]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Bruce Ashfield]

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
>

Re: [discussion] perf: specify SLANG_INC dir for perf

[McClintock Matthew-B29882]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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

---

With this, we might not have to touch perf.bb. So seems like we'll use
this solution? :)

Thanks,
		Liang Li

> Cheers,
> 
> Richard

Re: [discussion] perf: specify SLANG_INC dir for perf

[Richard Purdie]

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.

Cheers,

Richard

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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>

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

---

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Richard Purdie]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Bruce Ashfield]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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
to linux-yocto kernel first, and propose it to mainline kernel in the
meantime. With proper kernel patch in linux-yocto kernel(s), we does
not have to do the second above, right? :)

And I see what you mean here, let me paste the revised change to
kernel here:

commit 5236988
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

---

Thanks for clarification.

Regards,
		Liang Li

> Cheers,
> 
> Richard
> 

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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

Sorry, I mean 'we can ...' instead of 'we have ...', just typo.

Re: [discussion] perf: specify SLANG_INC dir for perf

[Bruce Ashfield]

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 ?

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.

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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

---

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.

Re: [discussion] perf: specify SLANG_INC dir for perf

[Henning Heinold]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Richard Purdie]

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Bruce Ashfield]

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.

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

On 2012-08-21 21:07, Bruce Ashfield <bruce.ashfield@windriver.com> wrote:
> 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.
> 

Yes, the change for the master branch is almost the same, except line
number context:

diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
index f34e632..fdef1be 100644
--- a/meta/classes/kernel.bbclass
+++ b/meta/classes/kernel.bbclass
@@ -204,6 +204,9 @@ kernel_do_install() {
 	for entry in $bin_files; do
 		rm -f $kerneldir/$entry
 	done
+
+	# Fix SLANG_INC for slang.h
+	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
 }
 
 sysroot_stage_all_append() {

---

> >   }
> >
> >   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.
> 

No problem, rephrased commit message:

Subject: [PATCH] perf: add SLANG_INC for slang.h

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
like:

cc1: warning: '/usr/include/slang' doesn't exists.

or

cc1: warning: include location "/usr/include/slang" is unsafe for
cross-compilation [-Wpoison-system-directories]

Then in some cases warnings become errors if WERROR is enabled hence
build errors.

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. And add a '=' prefix to indicate better
compatibility with sysroot/cross compile cases.

Signed-off-by: Liang Li <liang.li@windriver.com>

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

Confirmed.

Thanks,
		Liang Li

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

Re: [discussion] perf: specify SLANG_INC dir for perf

[Bruce Ashfield]

On 12-08-21 11:01 PM, Liang Li wrote:
> On 2012-08-21 21:07, Bruce Ashfield<bruce.ashfield@windriver.com>  wrote:
>> 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.
>>
>
> Yes, the change for the master branch is almost the same, except line
> number context:
>
> diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
> index f34e632..fdef1be 100644
> --- a/meta/classes/kernel.bbclass
> +++ b/meta/classes/kernel.bbclass
> @@ -204,6 +204,9 @@ kernel_do_install() {
>   	for entry in $bin_files; do
>   		rm -f $kerneldir/$entry
>   	done
> +
> +	# Fix SLANG_INC for slang.h
> +	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
>   }
>
>   sysroot_stage_all_append() {
>
> ---
>
>>>    }
>>>
>>>    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.
>>
>
> No problem, rephrased commit message:
>
> Subject: [PATCH] perf: add SLANG_INC for slang.h
>
> 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
> like:
>
> cc1: warning: '/usr/include/slang' doesn't exists.
>
> or
>
> cc1: warning: include location "/usr/include/slang" is unsafe for
> cross-compilation [-Wpoison-system-directories]
>
> Then in some cases warnings become errors if WERROR is enabled hence
> build errors.
>
> 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. And add a '=' prefix to indicate better
> compatibility with sysroot/cross compile cases.
>
> Signed-off-by: Liang Li<liang.li@windriver.com>
>
>>>
>>>       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.
>>
>
> Confirmed.

Both look fine to me, if you send me a pull request for the kernel.bbclass
part, and a separate one for the kernel patch, I'll take care
of dropping the old slang patch, and getting all the changes
to Richard in a single pull request.

We can then take the kernel patch upstream to the perf
maintainers for their comments.

Cheers,

Bruce

>
> Thanks,
> 		Liang Li
>
>> 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.

Re: [discussion] perf: specify SLANG_INC dir for perf

[Liang Li]

On 2012-08-22 13:41, Bruce Ashfield <bruce.ashfield@windriver.com> wrote:
> On 12-08-21 11:01 PM, Liang Li wrote:
> > On 2012-08-21 21:07, Bruce Ashfield<bruce.ashfield@windriver.com>  wrote:
> >> 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.
> >>
> >
> > Yes, the change for the master branch is almost the same, except line
> > number context:
> >
> > diff --git a/meta/classes/kernel.bbclass b/meta/classes/kernel.bbclass
> > index f34e632..fdef1be 100644
> > --- a/meta/classes/kernel.bbclass
> > +++ b/meta/classes/kernel.bbclass
> > @@ -204,6 +204,9 @@ kernel_do_install() {
> >   	for entry in $bin_files; do
> >   		rm -f $kerneldir/$entry
> >   	done
> > +
> > +	# Fix SLANG_INC for slang.h
> > +	sed -i 's#-I/usr/include/slang#-I=/usr/include/slang#g' $kerneldir/tools/perf/Makefile
> >   }
> >
> >   sysroot_stage_all_append() {
> >
> > ---
> >
> >>>    }
> >>>
> >>>    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.
> >>
> >
> > No problem, rephrased commit message:
> >
> > Subject: [PATCH] perf: add SLANG_INC for slang.h
> >
> > 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
> > like:
> >
> > cc1: warning: '/usr/include/slang' doesn't exists.
> >
> > or
> >
> > cc1: warning: include location "/usr/include/slang" is unsafe for
> > cross-compilation [-Wpoison-system-directories]
> >
> > Then in some cases warnings become errors if WERROR is enabled hence
> > build errors.
> >
> > 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. And add a '=' prefix to indicate better
> > compatibility with sysroot/cross compile cases.
> >
> > Signed-off-by: Liang Li<liang.li@windriver.com>
> >
> >>>
> >>>       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.
> >>
> >
> > Confirmed.
> 
> Both look fine to me, if you send me a pull request for the kernel.bbclass
> part, and a separate one for the kernel patch, I'll take care
> of dropping the old slang patch, and getting all the changes
> to Richard in a single pull request.
> 

Done. Thanks.

Regards,
		Liang Li

Re: [discussion] perf: specify SLANG_INC dir for perf

[Darren Hart]

On 08/17/2012 06:01 AM, Liang Li 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
> to linux-yocto kernel first, and propose it to mainline kernel in the
> meantime. With proper kernel patch in linux-yocto kernel(s), we does
> not have to do the second above, right? :)

No, the kernel.bbclass supports more than the linux-yocto kernels. It
needs to be generally useful with 2.6+kernels. The sed patch for the
kernel.bbclass is still necessary.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel