[LEDE-DEV] [PATCH 1/5] base-files: introduce /lib/functions/board.sh

[Roman Yeryomin]

On 3 May 2017 at 13:38, Felix Fietkau <nbd at nbd.name> wrote:
> On 2017-05-03 12:15, Roman Yeryomin wrote:
>>>> Simply including a big
>>>> functions.sh doesn't say anything and IMO can only be a workaround,
>>>> when you don't know what you need. But even now this doesn't work
>>>> because there are separate files in /lib/functions/
>>> In this case we're talking about *one* *single* *one-line* function (if
>>> you leave out the unused board_model function, which you really should).
>>> How does having a separate include file make sense?
>>
>> As I said, most target scripts need either board_name alone or a
>> fraction of functions.sh
>> Those which don't need board_name also need only a fraction of functions.sh
> At the moment, almost all of them include functions.sh either directly
> or indirectly.

There are 31 scripts across 12 targets which use functions.sh
directly. Out of those some include it by mistake (t.i. they either
don't need it at all or they are not aware that uci-defaults already
has it). And others include it because system.sh needs 2 functions
which are used only together with functions from system.sh (so this
will be optimized out). Some use config related functions which is
slightly bigger subset of functions.sh (7-8 functions) but which, IMO,
should be a separate include too.
47 scrips across 32 targets (out of 47 targets) use uci-defaults,
which needs only ONE function from functions.sh (so this will be
optimized out too).
And finally 114 scripts across 28 targets use board_name either from
target provided functions or manually reading it.
All this means that separate board.sh does make sense.

>>> As I mentioned earlier, if there are significant cases where you could
>>> avoid including functions.sh entirely, you could put this one-line
>>> function into a different one if you like, e.g. system.sh
>>
>> I see such solution only increasing the clutter.
>> Many scrips now include functions.sh because authors simply didn't
>> want to look at the source and see that they don't need it. Even that
>> bad.
>> Some functions are completely unused.
>>
>> Maybe it wasn't clear -- I'm volunteering to clean this clutter up.
>> But since it's spread all over the tree, I would do it step by step,
>> so it will take some time.
> I do agree with the goal of having a better split and smaller files, I
> just don't want to have tons of .sh files with just 1 or 2 one-liner
> functions.

As I see it only board.sh will be like that.
Others will have sane names so that one could understand what it has inside.

> I fully support you going ahead and cleaning it up, but it would be nice
> if we could find some agreement first on what the cleaned up version
> should look like. We should definitely discuss this further.

Ok, I can describe my vision in a separate thread so we can discuss it.

> Would you be okay with leaving this function where it is until we have a
> better plan on how to re-organize .sh include files?

As I described above I don't see why not having board.sh already. I
don't see a better alternative anyway.

> I think your board detection and ipq806x script cleanup is good and it
> shouldn't be held up by the generic shell script include discussion.

This generic script is a part of cleanup. And I'm sure it is an
improvement, not making any more cluttering.
I will make a rework you proposed on another patch but this one I'm
convinced should stay.


Regards,
Roman