| Message ID | 1493384195-7864-1-git-send-email-roman@advem.lv |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2017-04-28 14:56, Roman Yeryomin wrote: > Signed-off-by: Roman Yeryomin <roman@advem.lv> > --- > package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > create mode 100644 package/base-files/files/lib/functions/board.sh > > diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh > new file mode 100644 > index 0000000..28f3bad > --- /dev/null > +++ b/package/base-files/files/lib/functions/board.sh > @@ -0,0 +1,17 @@ > +#!/bin/sh > + > +sysinfo() > +{ > + local file=$1 > + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" > +} > + > +board_name() > +{ > + sysinfo board_name > +} > + > +board_model() > +{ > + sysinfo model > +} Do we really need board_model() at all? I think it's better to leave the board_name() function where it is instead of adding this extra include file. - Felix
On 28 April 2017 at 20:12, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-04-28 14:56, Roman Yeryomin wrote: >> Signed-off-by: Roman Yeryomin <roman@advem.lv> >> --- >> package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> create mode 100644 package/base-files/files/lib/functions/board.sh >> >> diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh >> new file mode 100644 >> index 0000000..28f3bad >> --- /dev/null >> +++ b/package/base-files/files/lib/functions/board.sh >> @@ -0,0 +1,17 @@ >> +#!/bin/sh >> + >> +sysinfo() >> +{ >> + local file=$1 >> + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" >> +} >> + >> +board_name() >> +{ >> + sysinfo board_name >> +} >> + >> +board_model() >> +{ >> + sysinfo model >> +} > Do we really need board_model() at all? I think it's better to leave the > board_name() function where it is instead of adding this extra include file. > If we don't need it why it exists then? I think it's better to leave as separate file because it's mostly used separately, sourcing platform file, but for functions.sh it's negligible change. Regards, Roman
* Felix Fietkau <nbd@nbd.name> [30.04.2017 09:30]: > > +board_name() > > +{ > > + sysinfo board_name > > +} > > + > > +board_model() > > +{ > > + sysinfo model > > +} > Do we really need board_model() at all? I think it's better to leave the > board_name() function where it is instead of adding this extra include file. the difference is that: # root@box:~ cat /tmp/sysinfo/model # TP-Link TL-WDR4300 v1 # root@box:~ cat /tmp/sysinfo/board_name # tl-wdr4300 we use 'model' e.g. for our monitoring. bye, bastian
On 2017-04-30 11:23, Bastian Bittorf wrote: > * Felix Fietkau <nbd@nbd.name> [30.04.2017 09:30]: >> > +board_name() >> > +{ >> > + sysinfo board_name >> > +} >> > + >> > +board_model() >> > +{ >> > + sysinfo model >> > +} > >> Do we really need board_model() at all? I think it's better to leave the >> board_name() function where it is instead of adding this extra include file. > > the difference is that: > > # root@box:~ cat /tmp/sysinfo/model > # TP-Link TL-WDR4300 v1 > # root@box:~ cat /tmp/sysinfo/board_name > # tl-wdr4300 > > we use 'model' e.g. for our monitoring. Right, but you don't need that shell function in a separate include file. For your purposes, you can simply use 'cat /tmp/sysinfo/board_name' in your script. - Felix
On 2017-04-28 22:51, Roman Yeryomin wrote: > On 28 April 2017 at 20:12, Felix Fietkau <nbd@nbd.name> wrote: >> On 2017-04-28 14:56, Roman Yeryomin wrote: >>> Signed-off-by: Roman Yeryomin <roman@advem.lv> >>> --- >>> package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> create mode 100644 package/base-files/files/lib/functions/board.sh >>> >>> diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh >>> new file mode 100644 >>> index 0000000..28f3bad >>> --- /dev/null >>> +++ b/package/base-files/files/lib/functions/board.sh >>> @@ -0,0 +1,17 @@ >>> +#!/bin/sh >>> + >>> +sysinfo() >>> +{ >>> + local file=$1 >>> + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" >>> +} >>> + >>> +board_name() >>> +{ >>> + sysinfo board_name >>> +} >>> + >>> +board_model() >>> +{ >>> + sysinfo model >>> +} >> Do we really need board_model() at all? I think it's better to leave the >> board_name() function where it is instead of adding this extra include file. >> > > If we don't need it why it exists then? It exists in various files because of random copy&paste madness. It hasn't been used in years, so it does not make any sense to carry it forward. > I think it's better to leave as separate file because it's mostly used > separately, sourcing platform file, but for functions.sh it's > negligible change. The function is so small that a separate file does not make any sense. If you don't want it in functions.sh, you could try to find a different one where it fits better. But please don't add a new file for this tiny piece of code. - Felix
On 30 April 2017 at 12:44, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-04-30 11:23, Bastian Bittorf wrote: >> * Felix Fietkau <nbd@nbd.name> [30.04.2017 09:30]: >>> > +board_name() >>> > +{ >>> > + sysinfo board_name >>> > +} >>> > + >>> > +board_model() >>> > +{ >>> > + sysinfo model >>> > +} >> >>> Do we really need board_model() at all? I think it's better to leave the >>> board_name() function where it is instead of adding this extra include file. >> >> the difference is that: >> >> # root@box:~ cat /tmp/sysinfo/model >> # TP-Link TL-WDR4300 v1 >> # root@box:~ cat /tmp/sysinfo/board_name >> # tl-wdr4300 >> >> we use 'model' e.g. for our monitoring. > Right, but you don't need that shell function in a separate include > file. For your purposes, you can simply use 'cat > /tmp/sysinfo/board_name' in your script. > simply `cat` (and check for file and NULL) doesn't look good to me for a generic "api"
On 30 April 2017 at 12:46, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-04-28 22:51, Roman Yeryomin wrote: >> On 28 April 2017 at 20:12, Felix Fietkau <nbd@nbd.name> wrote: >>> On 2017-04-28 14:56, Roman Yeryomin wrote: >>>> Signed-off-by: Roman Yeryomin <roman@advem.lv> >>>> --- >>>> package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> create mode 100644 package/base-files/files/lib/functions/board.sh >>>> >>>> diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh >>>> new file mode 100644 >>>> index 0000000..28f3bad >>>> --- /dev/null >>>> +++ b/package/base-files/files/lib/functions/board.sh >>>> @@ -0,0 +1,17 @@ >>>> +#!/bin/sh >>>> + >>>> +sysinfo() >>>> +{ >>>> + local file=$1 >>>> + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" >>>> +} >>>> + >>>> +board_name() >>>> +{ >>>> + sysinfo board_name >>>> +} >>>> + >>>> +board_model() >>>> +{ >>>> + sysinfo model >>>> +} >>> Do we really need board_model() at all? I think it's better to leave the >>> board_name() function where it is instead of adding this extra include file. >>> >> >> If we don't need it why it exists then? > It exists in various files because of random copy&paste madness. It > hasn't been used in years, so it does not make any sense to carry it > forward. This is another reason why it should be kept generic and changed in one place if needed. >> I think it's better to leave as separate file because it's mostly used >> separately, sourcing platform file, but for functions.sh it's >> negligible change. > The function is so small that a separate file does not make any sense. > If you don't want it in functions.sh, you could try to find a different > one where it fits better. But please don't add a new file for this tiny > piece of code. This tiny file will nuke a lot of lines from target scripts. So it's about optimizing the code actually. I don't see a better place for it because, like I've noticed before, the users mostly only need board name when they include platform files. Also having single separate place for board detection function eases the maintenance and allows changing/improving the way it is stored. Could you explain why do you think it's bad to have a separate file for it? Regards, Roman
On 2017-05-02 00:50, Roman Yeryomin wrote: > On 30 April 2017 at 12:46, Felix Fietkau <nbd@nbd.name> wrote: >> On 2017-04-28 22:51, Roman Yeryomin wrote: >>> On 28 April 2017 at 20:12, Felix Fietkau <nbd@nbd.name> wrote: >>>> On 2017-04-28 14:56, Roman Yeryomin wrote: >>>>> Signed-off-by: Roman Yeryomin <roman@advem.lv> >>>>> --- >>>>> package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ >>>>> 1 file changed, 17 insertions(+) >>>>> create mode 100644 package/base-files/files/lib/functions/board.sh >>>>> >>>>> diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh >>>>> new file mode 100644 >>>>> index 0000000..28f3bad >>>>> --- /dev/null >>>>> +++ b/package/base-files/files/lib/functions/board.sh >>>>> @@ -0,0 +1,17 @@ >>>>> +#!/bin/sh >>>>> + >>>>> +sysinfo() >>>>> +{ >>>>> + local file=$1 >>>>> + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" >>>>> +} >>>>> + >>>>> +board_name() >>>>> +{ >>>>> + sysinfo board_name >>>>> +} >>>>> + >>>>> +board_model() >>>>> +{ >>>>> + sysinfo model >>>>> +} >>>> Do we really need board_model() at all? I think it's better to leave the >>>> board_name() function where it is instead of adding this extra include file. >>>> >>> >>> If we don't need it why it exists then? >> It exists in various files because of random copy&paste madness. It >> hasn't been used in years, so it does not make any sense to carry it >> forward. > > This is another reason why it should be kept generic and changed in > one place if needed. Something not having been used in years and simply carried forward by mindless copy&paste is a good reason to keep it generic? I'm sorry, this makes no sense at all. Please just delete this nonsense. >>> I think it's better to leave as separate file because it's mostly used >>> separately, sourcing platform file, but for functions.sh it's >>> negligible change. >> The function is so small that a separate file does not make any sense. >> If you don't want it in functions.sh, you could try to find a different >> one where it fits better. But please don't add a new file for this tiny >> piece of code. > > This tiny file will nuke a lot of lines from target scripts. So it's > about optimizing the code actually. > I don't see a better place for it because, like I've noticed before, > the users mostly only need board name when they include platform > files. board_name already exists, and I've already converted e.g. the lantiq target to use it directly from functions.sh. board_model is not used, neither is the model function from any of the messy copy&paste target files. Which means you're arguing for having a separate source file for a function that in its current form only has one single line of code. In my opinion the best course of action is to nuke your patches 1-3 completely, rework patch 4 to adjust the generic code as proposed, and adjust patch 5 to simply use the board_name() from /lib/functions.sh Just ignore the board_model nonsense, it does not need to be part of the shell 'API' at all. - Felix
On 2 May 2017 at 10:25, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-05-02 00:50, Roman Yeryomin wrote: >> On 30 April 2017 at 12:46, Felix Fietkau <nbd@nbd.name> wrote: >>> On 2017-04-28 22:51, Roman Yeryomin wrote: >>>> On 28 April 2017 at 20:12, Felix Fietkau <nbd@nbd.name> wrote: >>>>> On 2017-04-28 14:56, Roman Yeryomin wrote: >>>>>> Signed-off-by: Roman Yeryomin <roman@advem.lv> >>>>>> --- >>>>>> package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ >>>>>> 1 file changed, 17 insertions(+) >>>>>> create mode 100644 package/base-files/files/lib/functions/board.sh >>>>>> >>>>>> diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh >>>>>> new file mode 100644 >>>>>> index 0000000..28f3bad >>>>>> --- /dev/null >>>>>> +++ b/package/base-files/files/lib/functions/board.sh >>>>>> @@ -0,0 +1,17 @@ >>>>>> +#!/bin/sh >>>>>> + >>>>>> +sysinfo() >>>>>> +{ >>>>>> + local file=$1 >>>>>> + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" >>>>>> +} >>>>>> + >>>>>> +board_name() >>>>>> +{ >>>>>> + sysinfo board_name >>>>>> +} >>>>>> + >>>>>> +board_model() >>>>>> +{ >>>>>> + sysinfo model >>>>>> +} >>>>> Do we really need board_model() at all? I think it's better to leave the >>>>> board_name() function where it is instead of adding this extra include file. >>>>> >>>> >>>> If we don't need it why it exists then? >>> It exists in various files because of random copy&paste madness. It >>> hasn't been used in years, so it does not make any sense to carry it >>> forward. >> >> This is another reason why it should be kept generic and changed in >> one place if needed. > Something not having been used in years and simply carried forward by > mindless copy&paste is a good reason to keep it generic? > I'm sorry, this makes no sense at all. Please just delete this nonsense. > >>>> I think it's better to leave as separate file because it's mostly used >>>> separately, sourcing platform file, but for functions.sh it's >>>> negligible change. >>> The function is so small that a separate file does not make any sense. >>> If you don't want it in functions.sh, you could try to find a different >>> one where it fits better. But please don't add a new file for this tiny >>> piece of code. >> >> This tiny file will nuke a lot of lines from target scripts. So it's >> about optimizing the code actually. >> I don't see a better place for it because, like I've noticed before, >> the users mostly only need board name when they include platform >> files. > board_name already exists, and I've already converted e.g. the lantiq > target to use it directly from functions.sh. > board_model is not used, neither is the model function from any of the > messy copy&paste target files. Which means you're arguing for having a > separate source file for a function that in its current form only has > one single line of code. > > In my opinion the best course of action is to nuke your patches 1-3 > completely, rework patch 4 to adjust the generic code as proposed, and > adjust patch 5 to simply use the board_name() from /lib/functions.sh > > Just ignore the board_model nonsense, it does not need to be part of the > shell 'API' at all. I'm talking about board_name, not model. I've added model to the file only because /tmp/sysinfo/model exists. You say that including 365 (non-functional) lines is better than 15 (functional) when you only need board_name? I cannot understand that... And if you need functions.sh also then it includes board.sh, which is nothing, compared to functions.sh itself, and nothing is broken. Regards, Roman
On 2017-05-02 17:53, Roman Yeryomin wrote: > On 2 May 2017 at 10:25, Felix Fietkau <nbd@nbd.name> wrote: >> board_name already exists, and I've already converted e.g. the lantiq >> target to use it directly from functions.sh. >> board_model is not used, neither is the model function from any of the >> messy copy&paste target files. Which means you're arguing for having a >> separate source file for a function that in its current form only has >> one single line of code. >> >> In my opinion the best course of action is to nuke your patches 1-3 >> completely, rework patch 4 to adjust the generic code as proposed, and >> adjust patch 5 to simply use the board_name() from /lib/functions.sh >> >> Just ignore the board_model nonsense, it does not need to be part of the >> shell 'API' at all. > > I'm talking about board_name, not model. I've added model to the file > only because /tmp/sysinfo/model exists. > You say that including 365 (non-functional) lines is better than 15 > (functional) when you only need board_name? I cannot understand > that... I'd say almost all places that will use board_name() already need /lib/functions.sh. /lib/functions/uci-defaults.sh already includes it, so do many other core include files. Among the files touched by your rework, I think there is only one file that does not already include functions.sh. In functions.sh we already do have quite a few functions that are used only in some places and not in others. Putting every small piece into a separate include file would make things messy very quickly. This may seem like a minor detail to you, but I want to make sure that we don't add more unnecessary clutter here. - Felix
On 2 May 2017 at 19:18, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-05-02 17:53, Roman Yeryomin wrote: >> On 2 May 2017 at 10:25, Felix Fietkau <nbd@nbd.name> wrote: >>> board_name already exists, and I've already converted e.g. the lantiq >>> target to use it directly from functions.sh. >>> board_model is not used, neither is the model function from any of the >>> messy copy&paste target files. Which means you're arguing for having a >>> separate source file for a function that in its current form only has >>> one single line of code. >>> >>> In my opinion the best course of action is to nuke your patches 1-3 >>> completely, rework patch 4 to adjust the generic code as proposed, and >>> adjust patch 5 to simply use the board_name() from /lib/functions.sh >>> >>> Just ignore the board_model nonsense, it does not need to be part of the >>> shell 'API' at all. >> >> I'm talking about board_name, not model. I've added model to the file >> only because /tmp/sysinfo/model exists. >> You say that including 365 (non-functional) lines is better than 15 >> (functional) when you only need board_name? I cannot understand >> that... > I'd say almost all places that will use board_name() already need > /lib/functions.sh. > /lib/functions/uci-defaults.sh already includes it, so do many other > core include files. > Among the files touched by your rework, I think there is only one file > that does not already include functions.sh. > > In functions.sh we already do have quite a few functions that are used > only in some places and not in others. Putting every small piece into a > separate include file would make things messy very quickly. > > This may seem like a minor detail to you, but I want to make sure that > we don't add more unnecessary clutter here. I just want to save maintenance time. As you admit it's already a clutter. I'm trying to introduce some order. Actually uci-defaults is another one which doesn't need 90% of functions.sh (it only needs list_contains). I would propose breaking down functions.sh into smaller files by functionality, e.g. config/user/base/etc., together with optimizing it's usage in other scripts. Then /lib/functions.sh could become a meta include which includes everything under /lib/functions/. It would be more logical and structurally more right than including functions.sh from a script from /lib/functions/. Regards, Roman
On 2017-05-02 19:32, Roman Yeryomin wrote: > I just want to save maintenance time. As you admit it's already a > clutter. I'm trying to introduce some order. > Actually uci-defaults is another one which doesn't need 90% of > functions.sh (it only needs list_contains). > I would propose breaking down functions.sh into smaller files by > functionality, e.g. config/user/base/etc., together with optimizing > it's usage in other scripts. > Then /lib/functions.sh could become a meta include which includes > everything under /lib/functions/. It would be more logical and > structurally more right than including functions.sh from a script from > /lib/functions/. I think effectively you're making the whole thing slower and bigger at the same time. Processing the extra includes certainly results in extra syscalls and extra overhead, maybe even extra memory overhead. Additionally I'd say it does nothing to make the maintenance any easier. Please don't go that route, it really doesn't help. - Felix
On 2 May 2017 at 20:46, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-05-02 19:32, Roman Yeryomin wrote: >> I just want to save maintenance time. As you admit it's already a >> clutter. I'm trying to introduce some order. >> Actually uci-defaults is another one which doesn't need 90% of >> functions.sh (it only needs list_contains). >> I would propose breaking down functions.sh into smaller files by >> functionality, e.g. config/user/base/etc., together with optimizing >> it's usage in other scripts. >> Then /lib/functions.sh could become a meta include which includes >> everything under /lib/functions/. It would be more logical and >> structurally more right than including functions.sh from a script from >> /lib/functions/. > I think effectively you're making the whole thing slower and bigger at > the same time. Processing the extra includes certainly results in extra > syscalls and extra overhead, maybe even extra memory overhead. > Additionally I'd say it does nothing to make the maintenance any easier. > Please don't go that route, it really doesn't help. > Why do you think so? I've done tests, including few smaller files with only needed functions actually is better than including one big file like functions.sh. Performance hit for bigger includes is more than from several small ones. It's 10-20ms vs. 700-800ms over 100 iterations on ipq8065. And why do you say it will not make maintenance easier? Right now one cannot understand what is what. If you name things it would become more clear and easier to understand. 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/ Regards, Roman
On 2017-05-02 20:31, Roman Yeryomin wrote: > On 2 May 2017 at 20:46, Felix Fietkau <nbd@nbd.name> wrote: >> On 2017-05-02 19:32, Roman Yeryomin wrote: >>> I just want to save maintenance time. As you admit it's already a >>> clutter. I'm trying to introduce some order. >>> Actually uci-defaults is another one which doesn't need 90% of >>> functions.sh (it only needs list_contains). >>> I would propose breaking down functions.sh into smaller files by >>> functionality, e.g. config/user/base/etc., together with optimizing >>> it's usage in other scripts. >>> Then /lib/functions.sh could become a meta include which includes >>> everything under /lib/functions/. It would be more logical and >>> structurally more right than including functions.sh from a script from >>> /lib/functions/. >> I think effectively you're making the whole thing slower and bigger at >> the same time. Processing the extra includes certainly results in extra >> syscalls and extra overhead, maybe even extra memory overhead. >> Additionally I'd say it does nothing to make the maintenance any easier. >> Please don't go that route, it really doesn't help. >> > > Why do you think so? > I've done tests, including few smaller files with only needed > functions actually is better than including one big file like > functions.sh. > Performance hit for bigger includes is more than from several small > ones. It's 10-20ms vs. 700-800ms over 100 iterations on ipq8065. Did you compare the case of including functions.sh with the function included vs the case of functions.sh + a separate file with the extra function, included by functions.sh? > And why do you say it will not make maintenance easier? Right now one > cannot understand what is what. If you name things it would become > more clear and easier to understand. In many cases there are dependencies between functions. If you split things up and need to include one file from the other, you could later end up processing the same files over and over again if you include multiple files (or even the top-level functions.sh). If you avoid those include statements in the include files themselves, the caller would have to know about the dependencies, which is not exactly better either. > 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 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 - Felix
On 2 May 2017 at 21:47, Felix Fietkau <nbd@nbd.name> wrote: > On 2017-05-02 20:31, Roman Yeryomin wrote: >> On 2 May 2017 at 20:46, Felix Fietkau <nbd@nbd.name> wrote: >>> On 2017-05-02 19:32, Roman Yeryomin wrote: >>>> I just want to save maintenance time. As you admit it's already a >>>> clutter. I'm trying to introduce some order. >>>> Actually uci-defaults is another one which doesn't need 90% of >>>> functions.sh (it only needs list_contains). >>>> I would propose breaking down functions.sh into smaller files by >>>> functionality, e.g. config/user/base/etc., together with optimizing >>>> it's usage in other scripts. >>>> Then /lib/functions.sh could become a meta include which includes >>>> everything under /lib/functions/. It would be more logical and >>>> structurally more right than including functions.sh from a script from >>>> /lib/functions/. >>> I think effectively you're making the whole thing slower and bigger at >>> the same time. Processing the extra includes certainly results in extra >>> syscalls and extra overhead, maybe even extra memory overhead. >>> Additionally I'd say it does nothing to make the maintenance any easier. >>> Please don't go that route, it really doesn't help. >>> >> >> Why do you think so? >> I've done tests, including few smaller files with only needed >> functions actually is better than including one big file like >> functions.sh. >> Performance hit for bigger includes is more than from several small >> ones. It's 10-20ms vs. 700-800ms over 100 iterations on ipq8065. > Did you compare the case of including functions.sh with the function > included vs the case of functions.sh + a separate file with the extra > function, included by functions.sh? Different scenarios, including that. All they show that what makes the difference is the size of include, not count (up to sane numbers). Last test I did was including 4 smaller files from /lib/functions (summed lines are 310, close to functions.sh) vs. including single functions.sh Which I would say is pretty pessimistic scenario, most use cases will do 1-3 includes. And even then there is 500-600ms difference over 500 iterations. If I take including board.sh vs. functions.sh (which is NOT including anything further), then difference is 1500-1600ms. >> And why do you say it will not make maintenance easier? Right now one >> cannot understand what is what. If you name things it would become >> more clear and easier to understand. > In many cases there are dependencies between functions. If you split > things up and need to include one file from the other, you could later > end up processing the same files over and over again if you include > multiple files (or even the top-level functions.sh). > If you avoid those include statements in the include files themselves, > the caller would have to know about the dependencies, which is not > exactly better either. It's just a question of clearing that up. And clear naming will help a lot. >> 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 > 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. Regards, Roman
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. >> 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. 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. 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? 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. Thanks, - Felix
On 3 May 2017 at 13:38, Felix Fietkau <nbd@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
diff --git a/package/base-files/files/lib/functions/board.sh b/package/base-files/files/lib/functions/board.sh new file mode 100644 index 0000000..28f3bad --- /dev/null +++ b/package/base-files/files/lib/functions/board.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +sysinfo() +{ + local file=$1 + [ -e /tmp/sysinfo/$file ] && cat /tmp/sysinfo/$file || echo "generic" +} + +board_name() +{ + sysinfo board_name +} + +board_model() +{ + sysinfo model +}
Signed-off-by: Roman Yeryomin <roman@advem.lv> --- package/base-files/files/lib/functions/board.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 package/base-files/files/lib/functions/board.sh