| Message ID | 20170207220817.5781-1-peter@korsgaard.com |
|---|---|
| State | Accepted |
| Commit | e7548edb5f930362e361b14eb67cda2b16c8846c |
| Headers | show |
Hello Peter, On Tuesday 07 February 2017 23:08:17 Peter Korsgaard wrote: [...] > diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate > index 4a9b9b5e6..abe6b38f9 100755 > --- a/package/fakedate/fakedate > +++ b/package/fakedate/fakedate > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # vim: set sw=4 expandtab: > # > # This program is free software; you can redistribute it and/or modify > @@ -18,18 +18,12 @@ > # Copyright (C) 2016 Jérôme Pouiller <jezz@sysmic.org> > # > > -# Sanity check > -if ! readlink -f "$0" | grep -q fakedate; then > - echo "fakedate: Please name this script \`fakedate'" > - exit 1 > -fi > - > DATE_BIN=false > # Do not call `date' directly since it will produce an infinite recursion. > # Instead, find path of true `date' binary. > -for P in `echo $PATH | tr ':' ' '`; do > +IFS=':'; for P in $PATH; do I think you have to reset $IFS to its previous value after used it. Globally, I dislike use of $IFS. I think it may produce unattended side effects. > if [ -x "$P/date" ]; then > - if readlink -f "$P/date" | grep -qv fakedate; then > + if ! [ "$P/date" -ef "$0" ]; then Nice! I didn't know '-ef'. > DATE_BIN="$P/date" > break; > fi > @@ -50,8 +44,8 @@ if [ -n "$SOURCE_DATE_EPOCH" ]; then > done > if [ $FORCE_EPOCH -eq 1 ]; then > echo "date: Warning: using \$SOURCE_DATE_EPOCH instead of true time" >&2 > - exec $DATE_BIN -d "@$SOURCE_DATE_EPOCH" "$@" > + ARGS="-d @$SOURCE_DATE_EPOCH" Indentation seems incorrect: s/\t/ / > fi > fi > > -exec $DATE_BIN "$@" > +exec $DATE_BIN $ARGS "$@" It's matter of taste. > diff --git a/package/fakedate/fakedate.mk b/package/fakedate/fakedate.mk > index 61d4bd702..e81ce5dac 100644 > --- a/package/fakedate/fakedate.mk > +++ b/package/fakedate/fakedate.mk > @@ -8,8 +8,7 @@ > HOST_FAKEDATE_LICENSE = GPLv2+ > > define HOST_FAKEDATE_INSTALL_CMDS > - $(INSTALL) -D -m 755 package/fakedate/fakedate $(HOST_DIR)/usr/bin/fakedate > - ln -sfn fakedate $(HOST_DIR)/usr/bin/date > + $(INSTALL) -D -m 755 package/fakedate/fakedate $(HOST_DIR)/usr/bin/date Hmm... In order to not confuse user, I prefer to keep fakedate as original name and adding a symlink beside. > endef > > $(eval $(host-generic-package)) >
On 08-02-17 15:50, Jérôme Pouiller wrote: > Hello Peter, > > On Tuesday 07 February 2017 23:08:17 Peter Korsgaard wrote: > [...] >> diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate >> index 4a9b9b5e6..abe6b38f9 100755 >> --- a/package/fakedate/fakedate >> +++ b/package/fakedate/fakedate >> @@ -1,4 +1,4 @@ >> -#!/bin/sh >> +#!/bin/bash >> # vim: set sw=4 expandtab: >> # >> # This program is free software; you can redistribute it and/or > modify >> @@ -18,18 +18,12 @@ >> # Copyright (C) 2016 Jérôme Pouiller <jezz@sysmic.org> >> # >> >> -# Sanity check >> -if ! readlink -f "$0" | grep -q fakedate; then >> - echo "fakedate: Please name this script \`fakedate'" >> - exit 1 >> -fi >> - >> DATE_BIN=false >> # Do not call `date' directly since it will produce an infinite > recursion. >> # Instead, find path of true `date' binary. >> -for P in `echo $PATH | tr ':' ' '`; do >> +IFS=':'; for P in $PATH; do > > I think you have to reset $IFS to its previous value after used > it. That's on principle, though in this script there is no more word splitting after the loop. > > Globally, I dislike use of $IFS. I think it may produce unattended side > effects. Yeah, I find it particularly confusing when you need a different word splitting inside the loop... So indeed, I'd drop the IFS part from this patch. I don't think it makes things simpler. > > >> if [ -x "$P/date" ]; then >> - if readlink -f "$P/date" | grep -qv fakedate; then >> + if ! [ "$P/date" -ef "$0" ]; then > > Nice! I didn't know '-ef'. If you want to avoid the dependency on bash, you can also use if [ "$(readlink -f "$P/date")" != "$(readlink -f "$0")" ]; then but it's not exactly more readable :-) > >> DATE_BIN="$P/date" >> break; >> fi >> @@ -50,8 +44,8 @@ if [ -n "$SOURCE_DATE_EPOCH" ]; then >> done >> if [ $FORCE_EPOCH -eq 1 ]; then >> echo "date: Warning: using \$SOURCE_DATE_EPOCH instead of > true time" >&2 >> - exec $DATE_BIN -d "@$SOURCE_DATE_EPOCH" "$@" >> + ARGS="-d @$SOURCE_DATE_EPOCH" > > Indentation seems incorrect: s/\t/ / Very true. > >> fi >> fi >> >> -exec $DATE_BIN "$@" >> +exec $DATE_BIN $ARGS "$@" > > It's matter of taste. I'm fine either way. > > >> diff --git a/package/fakedate/fakedate.mk > b/package/fakedate/fakedate.mk >> index 61d4bd702..e81ce5dac 100644 >> --- a/package/fakedate/fakedate.mk >> +++ b/package/fakedate/fakedate.mk >> @@ -8,8 +8,7 @@ >> HOST_FAKEDATE_LICENSE = GPLv2+ >> >> define HOST_FAKEDATE_INSTALL_CMDS >> - $(INSTALL) -D -m 755 package/fakedate/fakedate > $(HOST_DIR)/usr/bin/fakedate >> - ln -sfn fakedate $(HOST_DIR)/usr/bin/date >> + $(INSTALL) -D -m 755 package/fakedate/fakedate > $(HOST_DIR)/usr/bin/date > > Hmm... In order to not confuse user, I prefer to keep fakedate as > original name and adding a symlink beside. I don't, I think it's clearer as a script than as a symlink. I don't mind much about the IFS part (it really should have been a separate patch, though), and the tab-space mixup can be fixed up while committing, so Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Regards, Arnout > >> endef >> >> $(eval $(host-generic-package)) >> >
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, >> Hmm... In order to not confuse user, I prefer to keep fakedate as >> original name and adding a symlink beside. > I don't, I think it's clearer as a script than as a symlink. > I don't mind much about the IFS part (it really should have been a separate > patch, though), and the tab-space mixup can be fixed up while committing, so > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Thanks, committed after dropping the IFS part.
diff --git a/package/fakedate/fakedate b/package/fakedate/fakedate index 4a9b9b5e6..abe6b38f9 100755 --- a/package/fakedate/fakedate +++ b/package/fakedate/fakedate @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # vim: set sw=4 expandtab: # # This program is free software; you can redistribute it and/or modify @@ -18,18 +18,12 @@ # Copyright (C) 2016 Jérôme Pouiller <jezz@sysmic.org> # -# Sanity check -if ! readlink -f "$0" | grep -q fakedate; then - echo "fakedate: Please name this script \`fakedate'" - exit 1 -fi - DATE_BIN=false # Do not call `date' directly since it will produce an infinite recursion. # Instead, find path of true `date' binary. -for P in `echo $PATH | tr ':' ' '`; do +IFS=':'; for P in $PATH; do if [ -x "$P/date" ]; then - if readlink -f "$P/date" | grep -qv fakedate; then + if ! [ "$P/date" -ef "$0" ]; then DATE_BIN="$P/date" break; fi @@ -50,8 +44,8 @@ if [ -n "$SOURCE_DATE_EPOCH" ]; then done if [ $FORCE_EPOCH -eq 1 ]; then echo "date: Warning: using \$SOURCE_DATE_EPOCH instead of true time" >&2 - exec $DATE_BIN -d "@$SOURCE_DATE_EPOCH" "$@" + ARGS="-d @$SOURCE_DATE_EPOCH" fi fi -exec $DATE_BIN "$@" +exec $DATE_BIN $ARGS "$@" diff --git a/package/fakedate/fakedate.mk b/package/fakedate/fakedate.mk index 61d4bd702..e81ce5dac 100644 --- a/package/fakedate/fakedate.mk +++ b/package/fakedate/fakedate.mk @@ -8,8 +8,7 @@ HOST_FAKEDATE_LICENSE = GPLv2+ define HOST_FAKEDATE_INSTALL_CMDS - $(INSTALL) -D -m 755 package/fakedate/fakedate $(HOST_DIR)/usr/bin/fakedate - ln -sfn fakedate $(HOST_DIR)/usr/bin/date + $(INSTALL) -D -m 755 package/fakedate/fakedate $(HOST_DIR)/usr/bin/date endef $(eval $(host-generic-package))
We can use IFS=':' to get the shell to split on colons instead of running tr, and using -ef to check for the same file is nicer than relying on magic symlink-to-fakedate. Notice that -ef isn't stricly posix (but supported by bash/dash/zsh), so I've changed the shebang to /bin/bash. While we are at it, restructure the logic to do a single exec at the end instead of handling the epoch/!epoch cases differently for simplicity. With that out of the way we can directly install it as $HOST/usr/bin/date instead of the fakedate / date symlink. Signed-off-by: Peter Korsgaard <peter@korsgaard.com> --- package/fakedate/fakedate | 16 +++++----------- package/fakedate/fakedate.mk | 3 +-- 2 files changed, 6 insertions(+), 13 deletions(-)