On Fri, May 16, 2014 at 12:30:45AM +0200, Lennart Poettering wrote:
> On Wed, 07.05.14 08:54, Emil Sjölin (emil.sjo...@axis.com) wrote:
>
> > This fix makes sure that the package installation will work
> > on systems using versions of 'GNU coreutils' older than 8.16.
> >
> > Please see tools/lnr.sh for limitations for this fix.
> > ---
> > configure.ac | 16 ++++++++++
> > tools/lnr.sh | 93
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 109 insertions(+)
> > create mode 100755 tools/lnr.sh
> >
> > diff --git a/configure.ac b/configure.ac
> > index ead697b..399a52f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -315,6 +315,22 @@ fi
> > AM_CONDITIONAL(ENABLE_COVERAGE, [test "$have_coverage" = "yes"])
> >
> > #
> > ------------------------------------------------------------------------------
> > +ln_relative_support=yes
> > +AC_CHECK_PROG(ln_found, [ln], [yes], [no])
> > +if test "x$ln_found" = xno ; then
> > + AC_MSG_ERROR([*** ln support requested but the program was not
> > found])
This error message is wrong: there was no request. It should be something
like "ln is required".
> > +else
> > + ln_version_major="`ln --version | head -1 | cut -d ' ' -f 4 | cut
> > -d '.' -f 1`"
> > + ln_version_minor="`ln --version | head -1 | cut -d ' ' -f 4 |
> > cut -d '.' -f 2`"
>
> Isn't "head -n 1" more correct?
>
>
> > + if test "$ln_version_major" -lt 8 || test "$ln_version_major" -eq
> > 8 -a "$ln_version_minor" -lt 16; then
> > + ln_relative_support=no
> > + fi
> > + if test "x$ln_relative_support" = "xno"; then
> > + LN_S=$(echo "$LN_S" | sed
> > s:"ln":""$srcdir"\/tools\/lnr.sh":)
The quoting here is ... complicated. If you count the quotes, what is
effectively
quoted are strings "ln", "\/tools\/lnr.sh". They don't contain shell special
characters,
so there's no need to quote them. OTOH, the parts which are _not_ quoted, might.
And why are slashes escaped?
LN_S="$(echo "$LN_S" | sed -E "s|^[^ ]+|${srcdir}/tools/lnr.sh|")"
This form has the advantage that it'll work if $LN_S e.g. has /bin/ln not ln,
and also if ${srcdir} contains spaces.
>
> Shouldn't this be "sed -e"?
It's fine here, because no file is given.
>
> > + fi
> > +fi
>
> Hmm, if we ship this anyway, why not always use it? Otherwise it would
> too easily bitrot...
>
> THis would also allow removing much of the shell pipeline in the
> configure script for this. I mean, these commands have changed all the
> time in the past, for example sed quite a bit...
Yeah, probably it's better to use it unconditionally. This will rid us of
the ugly version checks.
> > +#
> > ------------------------------------------------------------------------------
> > have_kmod=no
> > AC_ARG_ENABLE(kmod, AS_HELP_STRING([--disable-kmod], [disable loadable
> > modules support]))
> > if test "x$enable_kmod" != "xno"; then
> > diff --git a/tools/lnr.sh b/tools/lnr.sh
> > new file mode 100755
> > index 0000000..74e1644
> > --- /dev/null
> > +++ b/tools/lnr.sh
> > @@ -0,0 +1,93 @@
>
> No shebang?
>
> > +# This script makes the 'ln --relative' command work as expected on a
> > system where the
> > +# 'relative' option of 'ln' is not supported.
> > +#
> > +# NOTE:
> > +# The script assumes that the 'relative' option of 'ln' is used with any
> > +# of the following syntaxes:
> > +# '--relative'
> > +# '-r'
> > +#
> > +# The script will NOT handle combined options e.g. '-rf', '-ir' etc.
> > +# The script will also only handle the 1st form of the 'ln' command (see
> > man page
> > +# for the 'ln' command for the different forms).
> > +#
> > +
> > +
> > +while [ $# -gt 2 ]; do
> > + string="$1"
> > + if [ "${string#-*}" != "$string" ]; then
> > + # argument is an option
> > + if [ "$string" = "$relop_1" ] || [ "$string" =
> > "$relop_2" ]; then
>
> Why not "-o" instead of "] || ["?
>
> I'd really prefer if somebody who's a true shell guru could look over
> this. Harald? (Or Zbigniew?)
If I review it, do I get to become a true shell guru? ;)
Zbyszek
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
