Re: [PATCH] arago.conf: Correct TOOLCHAIN_BRAND for ARM9 SOCs

[Denys Dmytriyenko <denys@ti.com>]

On Mon, May 06, 2013 at 08:07:19PM -0400, Cooper Jr., Franklin wrote:
> > > > > > > diff --git a/meta-arago-distro/conf/distro/arago.conf
> > > > > > > b/meta-arago-distro/conf/distro/arago.conf
> > > > > > > index 3954360..048c480 100644
> > > > > > > --- a/meta-arago-distro/conf/distro/arago.conf
> > > > > > > +++ b/meta-arago-distro/conf/distro/arago.conf
> > > > > > > @@ -21,7 +21,9 @@ DISTRO_FEATURES = "alsa argp bluetooth ext2
> > > > > > > irda largefile pcmcia usbgadget usbh
> > > > > > >
> > > > > > >  # Toolchain should be selected by setting TOOLCHAIN_BRAND in
> > > > > > > local.conf  # Set some sane defaults, in case someone forgets
> > > > > > > to set them in local.conf -TOOLCHAIN_BRAND ?= "linaro"
> > > > > > > +# ARM9 is not supported by the Linaro toolchain so default
> > > > > > > +back to the Arago # toolchain for ARM9 based SOCs.
> > > > > > > +TOOLCHAIN_BRAND ?=
> > > > > > "${@bb.utils.contains('SOC_FAMILY','omapl138','arago','linaro',d)}"
> > > > > >
> > > > > > I don't feel this belongs in the arago.conf - as the comment
> > > > > > above says, this is supposed to be set in the local.conf
> > > > > > anyways, here it just offers a default fall back...
> > > > >
> > > > > I don't see what harm is done by tweaking the variable to insure
> > > > > an optimal and safe default fall back is provided for all
> > > > > platforms we support. This patch changes nothing except fixes a broken
> > use case.
> > > >
> > > > Overhead - it calls that Python function every time you evaluate
> > > > TOOLCHAIN_BRAND variable...
> > > >
> > > > I might be more lenient with something like this though:
> > > >
> > > > TC_SANEDEFAULT := "${@...}"
> > > > TOOLCHAIN_BRAND ?= "${TC_SANEDEFAULT}"
> > > >
> > > > Although, that would no longer work in local.conf, only arago.conf...
> > >
> > > If you have no objection with me putting my suggested change to
> > > TOOLCHAIN_BRAND in oe-layersetup local.conf.sample then I am fine with
> > > dropping this patch.
> > 
> > Either way is fine. You didn't like to split it in 2 lines like above?
> 
> I did like your suggestion but I was confused by the statement "Although, 
> that would no longer work in local.conf, only arago.conf...". 
> TOOLCHAIN_BRAND could still be overridden in local.conf but TC_SANEDEFAULT 
> couldn't. Is that really a problem since originally the expectation was that 
> someone should be overriding TOOLCHAIN_BRAND anyway?

Correct - I meant setting TC_SANEDEFAULT like that (immediate assignment with 
Python code) won't work in local.conf. Which is fine, as TOOLCHAIN_BRAND can 
be overriden from local.conf anyway.


> If your fine with the below then I'll send a v2
> TC_SANEDEFAULT := "${@bb.utils.contains('SOC_FAMILY','omapl138','arago','linaro',d)}"
> TOOLCHAIN_BRAND ?= "${TC_SANEDEFAULT}"

Yes, please.

-- 
Denys