| Message ID | 1494321420-7472-1-git-send-email-roman@advem.lv |
|---|---|
| State | Not Applicable |
| Delegated to: | Mathias Kresin |
| Headers | show |
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"
> +}
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" >> +} > >
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
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
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
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
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
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" +}
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