RE: [OE-core] [PATCH] Revert "featimage: refactor style"

[Peter Kjellerstedt <peter.kjellerstedt@axis.com>]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: den 29 januari 2022 02:39
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> core@lists.openembedded.org
> Cc: Andrej Valek <andrej.valek@siemens.com>; Richard Purdie
> <richard.purdie@linuxfoundation.org>
> Subject: Re: [OE-core] [PATCH] Revert "featimage: refactor style"
> 
> On 1/29/22 02:06, Peter Kjellerstedt wrote:
> >> -----Original Message-----
> >> From: openembedded-core@lists.openembedded.org <openembedded-
> >> core@lists.openembedded.org> On Behalf Of Marek Vasut
> >> Sent: den 29 januari 2022 01:29
> >> To: openembedded-core@lists.openembedded.org
> >> Cc: Marek Vasut <marex@denx.de>; Andrej Valek
> <andrej.valek@siemens.com>;
> >> Richard Purdie <richard.purdie@linuxfoundation.org>
> >> Subject: [OE-core] [PATCH] Revert "featimage: refactor style"
> >>
> >> This reverts commit f44bb458884da64356ee188917094b5515d3b159.
> >>
> >> The reverted patch attempted to perform some sort of clean up, however
> >> it only brought in style inconsistencies like this:
> >>
> >> ```
> >> conf_desc="$conf_desc${sep}setup"
> >> ```
> >>
> >> The curly brackets around variables were placed in the kernel-fitimage
> >> bbclass deliberately, since when assembling the fitimage ITS there are
> >> multiple variables where it is difficult to identify where the variable
> >> ends and some sort of follow up string starts.
> >
> > There is actually a technical reason to not use ${foo} for shell
> > variables unless necessary in bitbake files and it is because
> > bitbake will treat them all as potential bitbake variables. This
> > means they are unnecessarily included in the taskhashes that
> > bitbake calculates.
> 
> Yikes. (it would be good to include this gem in the commit message)

Well there was:

    - use bash variable notation without {} where possible
      - just to make sure it looks like bash variable not bitbake variable one

though I guess my explanation has a bit more technical merit 
than that it improves the looks. ;)

> So are we stuck with this inconsistent coding style change 
> or is there a

Personally I do not see it as inconsistent, it is just the way 
shell handles variables. It is just something to get used to (I 
also had a colleague who would review any shell code changes we 
made and comment on every single unnecessary character so one 
quickly learned to use $foo everywhere possible rather than 
${foo}...) That said, I have never looked at this code so I 
have no real idea of how bad it is or isn't.

> third alternative ? I mean, besides rewriting the fitimage 
> generation into python, which might make it more flexible too.

Replacing shell code that has grown beyond a couple of hundred 
(tens?) lines with something written in a better language is 
almost always a good idea.

//Peter