[LEDE-DEV,v2,2/3] base-files: put board_name into separate file

Submitted by Roman Yeryomin on May 9, 2017, 9:17 a.m.

Details

Message ID 1494321420-7472-1-git-send-email-roman@advem.lv
State Not Applicable
Delegated to: Mathias Kresin
Headers show

Commit Message

Roman Yeryomin May 9, 2017, 9:17 a.m.
Signed-off-by: Roman Yeryomin <roman@advem.lv>
---
 package/base-files/files/lib/functions.sh       | 4 +---
 package/base-files/files/lib/functions/board.sh | 6 ++++++
 2 files changed, 7 insertions(+), 3 deletions(-)
 create mode 100644 package/base-files/files/lib/functions/board.sh

Comments

John Crispin May 16, 2017, 10:26 a.m.
Hi Roman,

what is the reasoning behind moving this function to its own file ?

     John


On 09/05/17 11:17, Roman Yeryomin wrote:
> Signed-off-by: Roman Yeryomin <roman@advem.lv>
> ---
>   package/base-files/files/lib/functions.sh       | 4 +---
>   package/base-files/files/lib/functions/board.sh | 6 ++++++
>   2 files changed, 7 insertions(+), 3 deletions(-)
>   create mode 100644 package/base-files/files/lib/functions/board.sh
>
> diff --git a/package/base-files/files/lib/functions.sh b/package/base-files/files/lib/functions.sh
> index 2b6415a..2aaafcf 100755
> --- a/package/base-files/files/lib/functions.sh
> +++ b/package/base-files/files/lib/functions.sh
> @@ -353,8 +353,6 @@ user_exists() {
>   	grep -qs "^${1}:" ${IPKG_INSTROOT}/etc/passwd
>   }
>   
> -board_name() {
> -	[ -e /tmp/sysinfo/board_name ] && cat /tmp/sysinfo/board_name || echo "generic"
> -}
> +. /lib/functions/board.sh
>   
>   [ -z "$IPKG_INSTROOT" -a -f /lib/config/uci.sh ] && . /lib/config/uci.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..d33edc8
> --- /dev/null
> +++ b/package/base-files/files/lib/functions/board.sh
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +
> +board_name()
> +{
> +	[ -e /tmp/sysinfo/board_name ] && cat /tmp/sysinfo/board_name || echo "generic"
> +}
Roman Yeryomin May 17, 2017, 11:49 a.m.
Hi John,

the reasoning is that most scripts which need board_name, don't need
anything else, and those which do need more, they need only a fraction
of functions.sh
The other aspect is that, e.g. including board.sh is 50-60 times
faster than including functions.sh
I'm going to send another RFC email about other scripts rework but the
main idea (as described in v1 of this patch set) is to separate
functions.sh by usage and reconsider it's functions usage.

Regards,
Roman

On 16 May 2017 at 13:26, John Crispin <john@phrozen.org> wrote:
> Hi Roman,
>
> what is the reasoning behind moving this function to its own file ?
>
>     John
>
>
>
> On 09/05/17 11:17, Roman Yeryomin wrote:
>>
>> Signed-off-by: Roman Yeryomin <roman@advem.lv>
>> ---
>>   package/base-files/files/lib/functions.sh       | 4 +---
>>   package/base-files/files/lib/functions/board.sh | 6 ++++++
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>   create mode 100644 package/base-files/files/lib/functions/board.sh
>>
>> diff --git a/package/base-files/files/lib/functions.sh
>> b/package/base-files/files/lib/functions.sh
>> index 2b6415a..2aaafcf 100755
>> --- a/package/base-files/files/lib/functions.sh
>> +++ b/package/base-files/files/lib/functions.sh
>> @@ -353,8 +353,6 @@ user_exists() {
>>         grep -qs "^${1}:" ${IPKG_INSTROOT}/etc/passwd
>>   }
>>   -board_name() {
>> -       [ -e /tmp/sysinfo/board_name ] && cat /tmp/sysinfo/board_name ||
>> echo "generic"
>> -}
>> +. /lib/functions/board.sh
>>     [ -z "$IPKG_INSTROOT" -a -f /lib/config/uci.sh ] && .
>> /lib/config/uci.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..d33edc8
>> --- /dev/null
>> +++ b/package/base-files/files/lib/functions/board.sh
>> @@ -0,0 +1,6 @@
>> +#!/bin/sh
>> +
>> +board_name()
>> +{
>> +       [ -e /tmp/sysinfo/board_name ] && cat /tmp/sysinfo/board_name ||
>> echo "generic"
>> +}
>
>
Mathias Kresin May 17, 2017, 1:19 p.m.
Hey Roman.,

independent of the work done by you, I worked on exactly the same
topic. But I've pushed it a bit further by switching all targets to
the generic boardname function and a few targets to the generic board
detection in basefiles/preinit.

Furthermore I've converted some targets to get the boardname from the
device tree compatible string as suggested by John. Till now I wasn't
comfortable to push it somewhere, but with my latest changes it should
work in theory. You can find the changes at
https://git.lede-project.org/?p=lede/mkresin/staging.git;a=shortlog;h=refs/heads/boarddetection.
What is missing: testing!

2017-05-17 13:49 GMT+02:00 Roman Yeryomin <leroi.lists@gmail.com>:
> Hi John,
>
> the reasoning is that most scripts which need board_name, don't need
> anything else, and those which do need more, they need only a fraction
> of functions.sh

Due to the fact that I had to touch all scripts using the board name,
I feel confident enough to reply here. I don't see any benefit in
moving the board_name function into an extra script. Most of the
scripts include (and require) functions.sh anyway.

I would be happy I've you can review and runtime test the boardname
branch from my staging tree.

Mathias
Roman Yeryomin May 17, 2017, 6:39 p.m.
On 17 May 2017 at 16:19, Mathias Kresin <dev@kresin.me> wrote:
> Hey Roman.,
>
> independent of the work done by you, I worked on exactly the same
> topic. But I've pushed it a bit further by switching all targets to
> the generic boardname function and a few targets to the generic board
> detection in basefiles/preinit.
>
> Furthermore I've converted some targets to get the boardname from the
> device tree compatible string as suggested by John. Till now I wasn't
> comfortable to push it somewhere, but with my latest changes it should
> work in theory. You can find the changes at
> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=shortlog;h=refs/heads/boarddetection.
> What is missing: testing!
>
> 2017-05-17 13:49 GMT+02:00 Roman Yeryomin <leroi.lists@gmail.com>:
>> Hi John,
>>
>> the reasoning is that most scripts which need board_name, don't need
>> anything else, and those which do need more, they need only a fraction
>> of functions.sh
>
> Due to the fact that I had to touch all scripts using the board name,
> I feel confident enough to reply here. I don't see any benefit in
> moving the board_name function into an extra script. Most of the
> scripts include (and require) functions.sh anyway.

I already explained that.
functions.sh inclusion in many cases is a mistake
and in other 90% of cases only one or two functions are needed
other 10% need 3-5 functions
so all that is very far from optimal both from maintenance/development
and resource usage perspective

> I would be happy I've you can review and runtime test the boardname
> branch from my staging tree.
>
> Mathias
Roman Yeryomin May 17, 2017, 7:08 p.m.
On 17 May 2017 at 21:39, Roman Yeryomin <leroi.lists@gmail.com> wrote:
> On 17 May 2017 at 16:19, Mathias Kresin <dev@kresin.me> wrote:
>> Hey Roman.,
>>
>> independent of the work done by you, I worked on exactly the same
>> topic. But I've pushed it a bit further by switching all targets to
>> the generic boardname function and a few targets to the generic board
>> detection in basefiles/preinit.
>>
>> Furthermore I've converted some targets to get the boardname from the
>> device tree compatible string as suggested by John. Till now I wasn't
>> comfortable to push it somewhere, but with my latest changes it should
>> work in theory. You can find the changes at
>> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=shortlog;h=refs/heads/boarddetection.
>> What is missing: testing!

Some of your changes would be ok to me if:
- board.sh would be used instead of functions.sh (which I would like
to split into several files anyway) for $(board_name)
- related changes would be squashed into one commit, otherwise it's
hard to perceive/track the changes, IMO more commits is not always
better

also this one is a fail:
https://git.lede-project.org/?p=lede/mkresin/staging.git;a=commitdiff;h=af3ef20591e6e9e4ee2cf1b1fc58012df68445ba

and changes like this is overkill:
+. /lib/functions.sh
 . /lib/functions/leds.sh
-. /lib/ar71xx.sh

>>
>> 2017-05-17 13:49 GMT+02:00 Roman Yeryomin <leroi.lists@gmail.com>:
>>> Hi John,
>>>
>>> the reasoning is that most scripts which need board_name, don't need
>>> anything else, and those which do need more, they need only a fraction
>>> of functions.sh
>>
>> Due to the fact that I had to touch all scripts using the board name,
>> I feel confident enough to reply here. I don't see any benefit in
>> moving the board_name function into an extra script. Most of the
>> scripts include (and require) functions.sh anyway.
>
> I already explained that.
> functions.sh inclusion in many cases is a mistake
> and in other 90% of cases only one or two functions are needed
> other 10% need 3-5 functions
> so all that is very far from optimal both from maintenance/development
> and resource usage perspective
>
>> I would be happy I've you can review and runtime test the boardname
>> branch from my staging tree.
>>
>> Mathias
Mathias Kresin May 17, 2017, 10:05 p.m.
17.05.2017 21:08, Roman Yeryomin:
> On 17 May 2017 at 21:39, Roman Yeryomin <leroi.lists@gmail.com> wrote:
>> On 17 May 2017 at 16:19, Mathias Kresin <dev@kresin.me> wrote:
>>> Hey Roman.,
>>>
>>> independent of the work done by you, I worked on exactly the same
>>> topic. But I've pushed it a bit further by switching all targets to
>>> the generic boardname function and a few targets to the generic board
>>> detection in basefiles/preinit.
>>>
>>> Furthermore I've converted some targets to get the boardname from the
>>> device tree compatible string as suggested by John. Till now I wasn't
>>> comfortable to push it somewhere, but with my latest changes it should
>>> work in theory. You can find the changes at
>>> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=shortlog;h=refs/heads/boarddetection.
>>> What is missing: testing!
>
> Some of your changes would be ok to me if:
> - board.sh would be used instead of functions.sh (which I would like
> to split into several files anyway) for $(board_name)

I guess we all got your point. But I'm sorry to say, that isn't subject 
of my changes.

> - related changes would be squashed into one commit, otherwise it's
> hard to perceive/track the changes, IMO more commits is not always
> better

The commits will be most likely squashed before I commit them. But at 
least during review I prefer smaller changes instead of a single patch 
with >100 lines.

>
> also this one is a fail:
> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=commitdiff;h=af3ef20591e6e9e4ee2cf1b1fc58012df68445ba

Hopefully someone beats me with a stick if I every reply during a review 
with "fail" and without any explanation what is wrong. This isn't 
helpful in any kind and just a waste of your and my time.

But I can give you a quick summary why the commit is required:

No target_board_name() => target_board_detect() isn't called => 
/tmp/sysinfo/board_name isn't populated => board_name() has nothing to 
return.

And as the commit message states, it is also meant to unify the way the 
board detection is done between targets (to not fool more people like I 
was fooled because of these target specifics).

Mathias
Roman Yeryomin May 18, 2017, 8:51 a.m.
On 18 May 2017 at 01:05, Mathias Kresin <dev@kresin.me> wrote:
> 17.05.2017 21:08, Roman Yeryomin:
>>
>> On 17 May 2017 at 21:39, Roman Yeryomin <leroi.lists@gmail.com> wrote:
>>>
>>> On 17 May 2017 at 16:19, Mathias Kresin <dev@kresin.me> wrote:
>>>>
>>>> Hey Roman.,
>>>>
>>>> independent of the work done by you, I worked on exactly the same
>>>> topic. But I've pushed it a bit further by switching all targets to
>>>> the generic boardname function and a few targets to the generic board
>>>> detection in basefiles/preinit.
>>>>
>>>> Furthermore I've converted some targets to get the boardname from the
>>>> device tree compatible string as suggested by John. Till now I wasn't
>>>> comfortable to push it somewhere, but with my latest changes it should
>>>> work in theory. You can find the changes at
>>>>
>>>> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=shortlog;h=refs/heads/boarddetection.
>>>> What is missing: testing!
>>
>>
>> Some of your changes would be ok to me if:
>> - board.sh would be used instead of functions.sh (which I would like
>> to split into several files anyway) for $(board_name)
>
>
> I guess we all got your point. But I'm sorry to say, that isn't subject of
> my changes.

I just wanted to say that the way you did it doesn't make sense to me
and IMO does more harm than good.

>> - related changes would be squashed into one commit, otherwise it's
>> hard to perceive/track the changes, IMO more commits is not always
>> better
>
>
> The commits will be most likely squashed before I commit them. But at least
> during review I prefer smaller changes instead of a single patch with >100
> lines.
>
>>
>> also this one is a fail:
>>
>> https://git.lede-project.org/?p=lede/mkresin/staging.git;a=commitdiff;h=af3ef20591e6e9e4ee2cf1b1fc58012df68445ba
>
>
> Hopefully someone beats me with a stick if I every reply during a review
> with "fail" and without any explanation what is wrong. This isn't helpful in
> any kind and just a waste of your and my time.

Sorry, didn't mean to be rude.
But, again, the way you did it doesn't make sense to me. You replaced
one platform specific code with other, it's not unifying anything,
like you wrote in commit message.
Instead here is what I propose: https://patchwork.ozlabs.org/patch/759976/

> But I can give you a quick summary why the commit is required:
>
> No target_board_name() => target_board_detect() isn't called =>
> /tmp/sysinfo/board_name isn't populated => board_name() has nothing to
> return.
>
> And as the commit message states, it is also meant to unify the way the
> board detection is done between targets (to not fool more people like I was
> fooled because of these target specifics).
>
> Mathias

Patch hide | download patch | download mbox

diff --git a/package/base-files/files/lib/functions.sh b/package/base-files/files/lib/functions.sh
index 2b6415a..2aaafcf 100755
--- a/package/base-files/files/lib/functions.sh
+++ b/package/base-files/files/lib/functions.sh
@@ -353,8 +353,6 @@  user_exists() {
 	grep -qs "^${1}:" ${IPKG_INSTROOT}/etc/passwd
 }
 
-board_name() {
-	[ -e /tmp/sysinfo/board_name ] && cat /tmp/sysinfo/board_name || echo "generic"
-}
+. /lib/functions/board.sh
 
 [ -z "$IPKG_INSTROOT" -a -f /lib/config/uci.sh ] && . /lib/config/uci.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..d33edc8
--- /dev/null
+++ b/package/base-files/files/lib/functions/board.sh
@@ -0,0 +1,6 @@ 
+#!/bin/sh
+
+board_name()
+{
+	[ -e /tmp/sysinfo/board_name ] && cat /tmp/sysinfo/board_name || echo "generic"
+}