RE: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
From: Masahiro Yamada
> Sent: 21 December 2020 14:29
>
> On Sun, Dec 13, 2020 at 6:47 AM David Laight wrote:
> >
> > From: Masahiro Yamada
> > > Sent: 12 December 2020 16:55
> > >
> > > This script was written in awk in spite of the file extension '.sh'.
> > > Rewrite it as a shell script.
> > ...
> > > +#
> > > +# Usage: $ ./scripts/ld-version.sh ld
> > > +#
> > > +# Print the linker version of `ld' in a 5 or 6-digit form
> > > +# such as `23501' for GNU ld 2.35.1 etc.
> > > +
> > > +first_line="$($* --version | head -n 1)"
> > > +
> > > +if ! ( echo $first_line | grep -q "GNU ld"); then
> > > + echo 0
> > > + exit 1
> > > +fi
> > > +
> > > +# Distributions may append an extra string like 2.35-15.fc33
> > > +# Take the part that consists of numbers and dots.
> > > +VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed
> > > 's/^\(^[0-9.]*\).*/\1/')
> > > +MAJOR=$(echo $VERSION | cut -d . -f 1)
> > > +MINOR=$(echo $VERSION | cut -d . -f 2)
> > > +PATCHLEVEL=$(echo $VERSION | cut -d . -f 3)
> > > +printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
> >
> >
> > H.
> > You've managed to convert an awk script into something that requires
> > sh, head, grep, sed (twice), and cut (thrice).
> > Plus (probably) a few sub-shells.
> >
> > It is quite ease to do it all in all in the shell.
> >
> > David
> >
>
> OK, please rewrite the code.
I've posted a couple of versions before, how about this one.
I've added a few comments - which don't need to be in the final version.
# Get the first line of the 'ld --version' output
if input_from_from_stdin; then
read line
else
IFS='
'
set -- $("$@")
line="$1"
fi
# Split the line on 'space' and get the last word
IFS=' '
set -- $line
shift $(($# - 1))
version="$1"
# Split on '.' and '-'
IFS='.-'
set -- $version
# The three version components are now $1 $2 and $3
# so you can do either
printf "%d%02d%02d\\n" $1 $2 $3
# or
echo $(($1 * 1 + $2 * 100 + $3))
# both are builtins on recent shells (printf is most likely not to be).
So this should be the same as the script above:
#! /bin/sh
IFS='
'
set -- $("$@")
# The last word contains the version
IFS=' '
set -- $1
shift $(($# - 1))
# Get the first 3 parts of the version
IFS='.-'
set -- $1
printf "%d%02d%02d\\n" $1 $2 $3
Seems to work for me.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
On Sun, Dec 13, 2020 at 6:47 AM David Laight wrote: > > From: Masahiro Yamada > > Sent: 12 December 2020 16:55 > > > > This script was written in awk in spite of the file extension '.sh'. > > Rewrite it as a shell script. > ... > > +# > > +# Usage: $ ./scripts/ld-version.sh ld > > +# > > +# Print the linker version of `ld' in a 5 or 6-digit form > > +# such as `23501' for GNU ld 2.35.1 etc. > > + > > +first_line="$($* --version | head -n 1)" > > + > > +if ! ( echo $first_line | grep -q "GNU ld"); then > > + echo 0 > > + exit 1 > > +fi > > + > > +# Distributions may append an extra string like 2.35-15.fc33 > > +# Take the part that consists of numbers and dots. > > +VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed > > 's/^\(^[0-9.]*\).*/\1/') > > +MAJOR=$(echo $VERSION | cut -d . -f 1) > > +MINOR=$(echo $VERSION | cut -d . -f 2) > > +PATCHLEVEL=$(echo $VERSION | cut -d . -f 3) > > +printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL > > > H. > You've managed to convert an awk script into something that requires > sh, head, grep, sed (twice), and cut (thrice). > Plus (probably) a few sub-shells. > > It is quite ease to do it all in all in the shell. > > David > OK, please rewrite the code. -- Best Regards Masahiro Yamada
Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
On Sun, Dec 13, 2020 at 2:48 AM Dominique Martinet
wrote:
>
> Masahiro Yamada wrote on Sun, Dec 13, 2020:
> > This script was written in awk in spite of the file extension '.sh'.
> > Rewrite it as a shell script.
>
> Wow! I wasn't expecting so much, would have sent some rework after the
> upcoming merge window.
> Thank you.
>
> Some comments below that you can probably ignore, this works for me.
>
>
> > diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> > index 0f8a2c0f9502..c214aeb3200d 100755
> > --- a/scripts/ld-version.sh
> > +++ b/scripts/ld-version.sh
> > @@ -1,11 +1,22 @@
> > -#!/usr/bin/awk -f
> > +#!/bin/sh
> > # SPDX-License-Identifier: GPL-2.0
> > -# extract linker version number from stdin and turn into single number
> > - {
> > - gsub(".*\\)", "");
> > - gsub(".*version ", "");
> > - gsub("-.*", "");
> > - split($1,a, ".");
> > - print a[1]*1 + a[2]*100 + a[3];
> > - exit
> > - }
> > +#
> > +# Usage: $ ./scripts/ld-version.sh ld
> > +#
> > +# Print the linker version of `ld' in a 5 or 6-digit form
> > +# such as `23501' for GNU ld 2.35.1 etc.
> > +
> > +first_line="$($* --version | head -n 1)"
>
> Just nitpicking: this ($*) would fail if the argument contains spaces,
> it's generally better to use "$@" or "$1" (with quotes)
This is just a copy-paste work based on scripts/lld-version.sh.
"$@" is better, I agree.
> Probably doesn't matter here as gcc/clang-version scripts have the same
> problem, so if someone had a problem with that they probably would have
> reported it there.
Talking about gcc/clang-version, "$1" is not acceptable because the first
word of the arguments may not be the compiler.
For example, when CC="ccache gcc" is passed in,
scripts/gcc-version.sh ccache gcc
must return the gcc version.
Difference between "$@" and "$*" matters only
when it is directly passed to some command.
masahiro@grover:~$ set "a b" c d
masahiro@grover:~$ ls "$*"
ls: cannot access 'a b c d': No such file or directory
masahiro@grover:~$ ls "$@"
ls: cannot access 'a b': No such file or directory
ls: cannot access 'c': No such file or directory
ls: cannot access 'd': No such file or directory
"$*" was expanded into a single string, 'a b c d'.
It forgot which spaces were the part of the argument,
and which spaces were argument delimiters.
In contrast, "$@" was expanded into three arguments,
'a b', 'c', and 'd'.
It correctly preserved the original arguments.
Let me continue some more experiments...
masahiro@grover:~$ set "a b" c d
masahiro@grover:~$ compiler="$*"
masahiro@grover:~$ ls "$compiler"
ls: cannot access 'a b c d': No such file or directory
masahiro@grover:~$ ls $compiler
ls: cannot access 'a': No such file or directory
ls: cannot access 'b': No such file or directory
ls: cannot access 'c': No such file or directory
ls: cannot access 'd': No such file or directory
masahiro@grover:~$ set "a b" c d
masahiro@grover:~$ compiler="$@"
masahiro@grover:~$ ls "$compiler"
ls: cannot access 'a b c d': No such file or directory
masahiro@grover:~$ ls $compiler
ls: cannot access 'a': No such file or directory
ls: cannot access 'b': No such file or directory
ls: cannot access 'c': No such file or directory
ls: cannot access 'd': No such file or directory
The result is the same.
So, whichever we use, after it is assigned to the variable "compiler",
it forgets the origin of the spaces.
If we really want to preserve the passed arguments,
we need to use "$@" directly in scripts/gcc-version.sh
like this:
MAJOR=$(echo __GNUC__ | "$@" -E -x c - | tail -n 1)
If we do this,
scripts/gcc-version.sh ccache gcc
will succeed, and
scripts/gcc-version.sh "ccache gcc"
will fail.
> > +
> > +if ! ( echo $first_line | grep -q "GNU ld"); then
> > + echo 0
> > + exit 1
> > +fi
> > +
> > +# Distributions may append an extra string like 2.35-15.fc33
> > +# Take the part that consists of numbers and dots.
> > +VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed
> > 's/^\(^[0-9.]*\).*/\1/')
> > +MAJOR=$(echo $VERSION | cut -d . -f 1)
> > +MINOR=$(echo $VERSION | cut -d . -f 2)
> > +PATCHLEVEL=$(echo $VERSION | cut -d . -f 3)
> > +printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
>
> There is a bug if there is no dot at all (e.g. if binutils ever releases
> a version 3 and call it version 3 and not 3.0, the script would print
> 30303 because cut when no delimiter is found always returns the whole
> string)
> This can be fixed by artificially appending a dot to VERSION:
> VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed
> 's/^\(^[0-9.]*\).*/\1./')
>
> I'm not sure it's worth worrying about either.
Ah, good catch.
--
Best Regards
Masahiro Yamada
RE: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
From: Masahiro Yamada > Sent: 12 December 2020 16:55 > > This script was written in awk in spite of the file extension '.sh'. > Rewrite it as a shell script. ... > +# > +# Usage: $ ./scripts/ld-version.sh ld > +# > +# Print the linker version of `ld' in a 5 or 6-digit form > +# such as `23501' for GNU ld 2.35.1 etc. > + > +first_line="$($* --version | head -n 1)" > + > +if ! ( echo $first_line | grep -q "GNU ld"); then > + echo 0 > + exit 1 > +fi > + > +# Distributions may append an extra string like 2.35-15.fc33 > +# Take the part that consists of numbers and dots. > +VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed > 's/^\(^[0-9.]*\).*/\1/') > +MAJOR=$(echo $VERSION | cut -d . -f 1) > +MINOR=$(echo $VERSION | cut -d . -f 2) > +PATCHLEVEL=$(echo $VERSION | cut -d . -f 3) > +printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL H. You've managed to convert an awk script into something that requires sh, head, grep, sed (twice), and cut (thrice). Plus (probably) a few sub-shells. It is quite ease to do it all in all in the shell. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
Hi Masahiro, I love your patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on kbuild/for-next arm64/for-next/core linus/master v5.10-rc7 next-20201211] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Masahiro-Yamada/kbuild-do-not-use-scripts-ld-version-sh-for-checking-spatch-version/20201213-010621 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-amigaone_defconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/0eab60f5e1f804528a4d0dd9566cb30f62242d22 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Masahiro-Yamada/kbuild-do-not-use-scripts-ld-version-sh-for-checking-spatch-version/20201213-010621 git checkout 0eab60f5e1f804528a4d0dd9566cb30f62242d22 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): /bin/sh: 1: [: -ge: unexpected operator >> /bin/sh: 1: [: -lt: unexpected operator --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
Hi Masahiro, I love your patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on kbuild/for-next arm64/for-next/core linus/master v5.10-rc7 next-20201211] [cannot apply to mmarek/misc] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Masahiro-Yamada/kbuild-do-not-use-scripts-ld-version-sh-for-checking-spatch-version/20201213-010621 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc64-randconfig-r014-20201213 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 84c09ab44599ece409e4e19761288ddf796fceec) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install powerpc64 cross compiling tool for clang build # apt-get install binutils-powerpc64-linux-gnu # https://github.com/0day-ci/linux/commit/0eab60f5e1f804528a4d0dd9566cb30f62242d22 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Masahiro-Yamada/kbuild-do-not-use-scripts-ld-version-sh-for-checking-spatch-version/20201213-010621 git checkout 0eab60f5e1f804528a4d0dd9566cb30f62242d22 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): >> /bin/sh: 1: [: -ge: unexpected operator /bin/sh: 1: [: -lt: unexpected operator -- >> /bin/sh: 1: [: -ge: unexpected operator -- >> /bin/sh: 1: [: -ge: unexpected operator --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
Hi Masahiro, I love your patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on kbuild/for-next arm64/for-next/core linus/master v5.10-rc7 next-20201211] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Masahiro-Yamada/kbuild-do-not-use-scripts-ld-version-sh-for-checking-spatch-version/20201213-010621 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-tqm8xx_defconfig (attached as .config) compiler: powerpc-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/0eab60f5e1f804528a4d0dd9566cb30f62242d22 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Masahiro-Yamada/kbuild-do-not-use-scripts-ld-version-sh-for-checking-spatch-version/20201213-010621 git checkout 0eab60f5e1f804528a4d0dd9566cb30f62242d22 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): >> /bin/sh: 1: [: -ge: unexpected operator /bin/sh: 1: [: -lt: unexpected operator -- >> /bin/sh: 1: [: -ge: unexpected operator -- >> /bin/sh: 1: [: -ge: unexpected operator --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH 3/3] kbuild: rewrite ld-version.sh in shell script
Masahiro Yamada wrote on Sun, Dec 13, 2020:
> This script was written in awk in spite of the file extension '.sh'.
> Rewrite it as a shell script.
Wow! I wasn't expecting so much, would have sent some rework after the
upcoming merge window.
Thank you.
Some comments below that you can probably ignore, this works for me.
> diff --git a/scripts/ld-version.sh b/scripts/ld-version.sh
> index 0f8a2c0f9502..c214aeb3200d 100755
> --- a/scripts/ld-version.sh
> +++ b/scripts/ld-version.sh
> @@ -1,11 +1,22 @@
> -#!/usr/bin/awk -f
> +#!/bin/sh
> # SPDX-License-Identifier: GPL-2.0
> -# extract linker version number from stdin and turn into single number
> - {
> - gsub(".*\\)", "");
> - gsub(".*version ", "");
> - gsub("-.*", "");
> - split($1,a, ".");
> - print a[1]*1 + a[2]*100 + a[3];
> - exit
> - }
> +#
> +# Usage: $ ./scripts/ld-version.sh ld
> +#
> +# Print the linker version of `ld' in a 5 or 6-digit form
> +# such as `23501' for GNU ld 2.35.1 etc.
> +
> +first_line="$($* --version | head -n 1)"
Just nitpicking: this ($*) would fail if the argument contains spaces,
it's generally better to use "$@" or "$1" (with quotes)
Probably doesn't matter here as gcc/clang-version scripts have the same
problem, so if someone had a problem with that they probably would have
reported it there.
> +
> +if ! ( echo $first_line | grep -q "GNU ld"); then
> + echo 0
> + exit 1
> +fi
> +
> +# Distributions may append an extra string like 2.35-15.fc33
> +# Take the part that consists of numbers and dots.
> +VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed
> 's/^\(^[0-9.]*\).*/\1/')
> +MAJOR=$(echo $VERSION | cut -d . -f 1)
> +MINOR=$(echo $VERSION | cut -d . -f 2)
> +PATCHLEVEL=$(echo $VERSION | cut -d . -f 3)
> +printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
There is a bug if there is no dot at all (e.g. if binutils ever releases
a version 3 and call it version 3 and not 3.0, the script would print
30303 because cut when no delimiter is found always returns the whole
string)
This can be fixed by artificially appending a dot to VERSION:
VERSION=$(echo $first_line | sed 's/.* \([^ ]*\)$/\1/' | sed
's/^\(^[0-9.]*\).*/\1./')
I'm not sure it's worth worrying about either.
Thanks again,
--
Dominique
