diff mbox series

package/fakedate: fix finding the right date executable

Message ID 20211215173432.r75invoe3ff6mthg@zenon.in.qult.net
State Accepted
Headers show
Series package/fakedate: fix finding the right date executable | expand

Commit Message

Ignacy Gawędzki Dec. 15, 2021, 5:34 p.m. UTC
If the PATH initially contains host/bin, then the right date
executable is to be found after the *first* occurrence of fakedate in
normal (forward) PATH order.  Fix the wrapper so that that first
occurrence is found indeed.

Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
 package/fakedate/fakedate | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Arnout Vandecappelle (Essensium/Mind) Dec. 16, 2021, 8:03 p.m. UTC | #1
On 15/12/2021 18:34, Ignacy Gawędzki wrote:
> If the PATH initially contains host/bin, then the right date
> executable is to be found after the *first* occurrence of fakedate in
> normal (forward) PATH order.  Fix the wrapper so that that first
> occurrence is found indeed.

  Just to be clear: the problem is that if you do

PATH=$PATH:$PWD/output/host/bin make

that it doesn't find the real date program, right?

> 
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> ---
>   package/fakedate/fakedate | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate
> index 9bef113357..03a4f04079 100755
> --- a/package/fakedate/fakedate
> +++ b/package/fakedate/fakedate
> @@ -21,13 +21,18 @@
>   DATE_BIN=false
>   # Do not call any 'date' before us in the PATH, or that would create
>   # an infinite recursion.
> +last_date=false
>   for date in $(which -a date |tac); do
>       if [ "${date}" -ef "$0" ]; then
> -        break
> +	DATE_BIN=$last_date
>       fi
> -    DATE_BIN="${date}"
> +    last_date="${date}"
>   done

  Although this works, wouldn't it be simpler to throw away the tac invocation 
and instead just take the first one after ourselves? I.e., something like:

found=false
for date in $(which -a date); do
     if [ "${date}" -ef "$@" ]; then
         found=true
     elif [ "$found" = true ]; then
         DATE_BIN="${date}"
     fi
done

(as usual, untested).

  Yann? Remember what was the reason for the tac?

  Regards,
  Arnout


>   
> +if [ "$DATE_BIN" = false ]; then
> +    DATE_BIN=$last_date
> +fi

> +
>   if [ -n "$SOURCE_DATE_EPOCH" ]; then
>       FORCE_EPOCH=1
>       for i in "$@"; do
>
Yann E. MORIN Dec. 16, 2021, 9:08 p.m. UTC | #2
Arnout, All,

On 2021-12-16 21:03 +0100, Arnout Vandecappelle spake thusly:
> On 15/12/2021 18:34, Ignacy Gawędzki wrote:
> >If the PATH initially contains host/bin, then the right date
> >executable is to be found after the *first* occurrence of fakedate in
> >normal (forward) PATH order.  Fix the wrapper so that that first
> >occurrence is found indeed.
>  Just to be clear: the problem is that if you do
> PATH=$PATH:$PWD/output/host/bin make
> that it doesn't find the real date program, right?

This is very not nice to do sto, indeed... :-(
Unless this is again this SDK thing that is bitting us back?

> >Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> >---
> >  package/fakedate/fakedate | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> >diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate
> >index 9bef113357..03a4f04079 100755
> >--- a/package/fakedate/fakedate
> >+++ b/package/fakedate/fakedate
> >@@ -21,13 +21,18 @@
> >  DATE_BIN=false
> >  # Do not call any 'date' before us in the PATH, or that would create
> >  # an infinite recursion.
> >+last_date=false
> >  for date in $(which -a date |tac); do
> >      if [ "${date}" -ef "$0" ]; then
> >-        break
> >+	DATE_BIN=$last_date
> >      fi
> >-    DATE_BIN="${date}"
> >+    last_date="${date}"
> >  done
> 
>  Although this works, wouldn't it be simpler to throw away the tac
> invocation and instead just take the first one after ourselves? I.e.,
> something like:
> 
> found=false
> for date in $(which -a date); do
>     if [ "${date}" -ef "$@" ]; then
>         found=true
>     elif [ "$found" = true ]; then

You can simplify the elif test with (after all, that's the whole point
of using false/true instead of 0/1):

    elif ${found}; then

>         DATE_BIN="${date}"

And then, you can break the loop because we did find it.

>     fi
> done

This is usually what I would have suggested... However, see below...

> (as usual, untested).

>  Yann? Remember what was the reason for the tac?

0fe536b797c (package/fakedate: avoid recursion if not first in the PATH)

I used tac because it avoided using an intermediate variable like you
suggest above, with "found=true":

    We fix that by iterating, in reverse order, over all the 'date' we can
    find in PATH, and when we find ourselves, then we know the one we found
    in the iteration just before is the one that we should call.

This was a hack, and in restrospect, not that smart as it seemed to be...

So this means that we now have the reverse problem: we recurse if we are
also the last...

Your (Arnout's) solution is the best I can think of: it is obvious,
straightforward, and it is not trying to be smart, so I like it.

Ignacy, when you respin with Arnout's proposal, do not forget to add a
line like that to your commit log, please:

    Fixes: 0fe536b797cc0ae11bd54ed7046cd39b73780c18

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> 
> >+if [ "$DATE_BIN" = false ]; then
> >+    DATE_BIN=$last_date
> >+fi
> 
> >+
> >  if [ -n "$SOURCE_DATE_EPOCH" ]; then
> >      FORCE_EPOCH=1
> >      for i in "$@"; do
> >
Ignacy Gawędzki Dec. 17, 2021, 10:28 p.m. UTC | #3
On Thu, Dec 16, 2021 at 10:08:08PM +0100, thus spake Yann E. MORIN:
> Arnout, All,
> 
> On 2021-12-16 21:03 +0100, Arnout Vandecappelle spake thusly:
> > On 15/12/2021 18:34, Ignacy Gawędzki wrote:
> > >If the PATH initially contains host/bin, then the right date
> > >executable is to be found after the *first* occurrence of fakedate in
> > >normal (forward) PATH order.  Fix the wrapper so that that first
> > >occurrence is found indeed.
> >  Just to be clear: the problem is that if you do
> > PATH=$PATH:$PWD/output/host/bin make
> > that it doesn't find the real date program, right?
> 
> This is very not nice to do sto, indeed... :-(
> Unless this is again this SDK thing that is bitting us back?

In a nutshell, I have host/bin appended to my PATH because I often use
the buildroot-built cross-compiler chain.

> > >Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> > >---
> > >  package/fakedate/fakedate | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate
> > >index 9bef113357..03a4f04079 100755
> > >--- a/package/fakedate/fakedate
> > >+++ b/package/fakedate/fakedate
> > >@@ -21,13 +21,18 @@
> > >  DATE_BIN=false
> > >  # Do not call any 'date' before us in the PATH, or that would create
> > >  # an infinite recursion.
> > >+last_date=false
> > >  for date in $(which -a date |tac); do
> > >      if [ "${date}" -ef "$0" ]; then
> > >-        break
> > >+	DATE_BIN=$last_date
> > >      fi
> > >-    DATE_BIN="${date}"
> > >+    last_date="${date}"
> > >  done
> > 
> >  Although this works, wouldn't it be simpler to throw away the tac
> > invocation and instead just take the first one after ourselves? I.e.,
> > something like:
> > 
> > found=false
> > for date in $(which -a date); do
> >     if [ "${date}" -ef "$@" ]; then
> >         found=true
> >     elif [ "$found" = true ]; then
> 
> You can simplify the elif test with (after all, that's the whole point
> of using false/true instead of 0/1):
> 
>     elif ${found}; then
> 
> >         DATE_BIN="${date}"
> 
> And then, you can break the loop because we did find it.
> 
> >     fi
> > done

The problem with this version is that it won't choose the first
non-fakedate date following fakedate in the PATH, but the last.
Moveover, if, for any reason, fakedate is not in the PATH, it won't
find any date.

This is mostly why I kept the reverse iteration, to find the first
non-fakedate date that follows the first fakedate in the PATH, and to
fall back on the first date in the PATH if fakedate is not found at
all.

Regards,

Ignacy
Yann E. MORIN Jan. 5, 2022, 8:37 p.m. UTC | #4
Ignacy, All,

On 2021-12-15 18:34 +0100, Ignacy Gawędzki spake thusly:
> If the PATH initially contains host/bin, then the right date
> executable is to be found after the *first* occurrence of fakedate in
> normal (forward) PATH order.  Fix the wrapper so that that first
> occurrence is found indeed.
> 
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>

After discussing this with Arnout and Thomas, we concluded that Arnout's
proposal, with my ltile amendment, was the proper solution. Except that
the loop had to be broken on first hit, of course, so I added the proper
break.

I also added a test that we actually did find a date executable.

Of course, if we missed something, then feel free to yel back! ;-)

Applied to master, thanks.

Regards,
Yann E. MORIN.

> ---
>  package/fakedate/fakedate | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate
> index 9bef113357..03a4f04079 100755
> --- a/package/fakedate/fakedate
> +++ b/package/fakedate/fakedate
> @@ -21,13 +21,18 @@
>  DATE_BIN=false
>  # Do not call any 'date' before us in the PATH, or that would create
>  # an infinite recursion.
> +last_date=false
>  for date in $(which -a date |tac); do
>      if [ "${date}" -ef "$0" ]; then
> -        break
> +	DATE_BIN=$last_date
>      fi
> -    DATE_BIN="${date}"
> +    last_date="${date}"
>  done
>  
> +if [ "$DATE_BIN" = false ]; then
> +    DATE_BIN=$last_date
> +fi
> +
>  if [ -n "$SOURCE_DATE_EPOCH" ]; then
>      FORCE_EPOCH=1
>      for i in "$@"; do
> -- 
> 2.32.0
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
Ignacy Gawędzki Jan. 6, 2022, 10:32 a.m. UTC | #5
On Wed, Jan 05, 2022 at 09:37:27PM +0100, thus spake Yann E. MORIN:
> Ignacy, All,
> 
> On 2021-12-15 18:34 +0100, Ignacy Gawędzki spake thusly:
> > If the PATH initially contains host/bin, then the right date
> > executable is to be found after the *first* occurrence of fakedate in
> > normal (forward) PATH order.  Fix the wrapper so that that first
> > occurrence is found indeed.
> > 
> > Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> 
> After discussing this with Arnout and Thomas, we concluded that Arnout's
> proposal, with my ltile amendment, was the proper solution. Except that
> the loop had to be broken on first hit, of course, so I added the proper
> break.
> 
> I also added a test that we actually did find a date executable.
> 
> Of course, if we missed something, then feel free to yel back! ;-)

With this code, fakedate is not usable without having its path in
PATH.  If that will never be a problem, then I have no reason to yell. =)

> 
> Applied to master, thanks.
> 
> Regards,
> Yann E. MORIN.
> 
> > ---
> >  package/fakedate/fakedate | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate
> > index 9bef113357..03a4f04079 100755
> > --- a/package/fakedate/fakedate
> > +++ b/package/fakedate/fakedate
> > @@ -21,13 +21,18 @@
> >  DATE_BIN=false
> >  # Do not call any 'date' before us in the PATH, or that would create
> >  # an infinite recursion.
> > +last_date=false
> >  for date in $(which -a date |tac); do
> >      if [ "${date}" -ef "$0" ]; then
> > -        break
> > +	DATE_BIN=$last_date
> >      fi
> > -    DATE_BIN="${date}"
> > +    last_date="${date}"
> >  done
> >  
> > +if [ "$DATE_BIN" = false ]; then
> > +    DATE_BIN=$last_date
> > +fi
> > +
> >  if [ -n "$SOURCE_DATE_EPOCH" ]; then
> >      FORCE_EPOCH=1
> >      for i in "$@"; do
> > -- 
> > 2.32.0
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
> 
> -- 
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
> 
>
diff mbox series

Patch

diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate
index 9bef113357..03a4f04079 100755
--- a/package/fakedate/fakedate
+++ b/package/fakedate/fakedate
@@ -21,13 +21,18 @@ 
 DATE_BIN=false
 # Do not call any 'date' before us in the PATH, or that would create
 # an infinite recursion.
+last_date=false
 for date in $(which -a date |tac); do
     if [ "${date}" -ef "$0" ]; then
-        break
+	DATE_BIN=$last_date
     fi
-    DATE_BIN="${date}"
+    last_date="${date}"
 done
 
+if [ "$DATE_BIN" = false ]; then
+    DATE_BIN=$last_date
+fi
+
 if [ -n "$SOURCE_DATE_EPOCH" ]; then
     FORCE_EPOCH=1
     for i in "$@"; do