[U-Boot] v2015.01-rc4 REGRESSION: "scripts: fix binutils-version.sh" breaks things for non Linaro toolchains
Hi,
I noticed $subject while doing a MAKEALL.
It seems that this commit: http://git.denx.de/?p=u-boot.git;a=commit;h=73c25753060c58e4c339fba306ed0ded...
Breaks things with Fedora's arm toolchain:
[hans@shalem u-boot]$ scripts/binutils-version.sh arm-linux-gnu-as scripts/binutils-version.sh: line 22: printf: GNU: invalid number scripts/binutils-version.sh: line 22: printf: assembler: invalid number 0000 scripts/binutils-version.sh: line 22: printf: version: invalid number 0002 2400
And:
[hans@shalem ~]$ arm-linux-gnu-as --version GNU assembler version 2.24.0-6.fc21 20140613 Copyright 2013 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or later. This program has absolutely no warranty. This assembler was configured for a target of `arm-linux-gnueabi'.
Which makes $version_string: "GNU assembler version 2.24.0-6.fc21 20140613" and $MAJOR: "GNU assembler version 2"
Regards,
Hans
On Tue, Dec 30, 2014 at 11:55:27AM +0100, Hans de Goede wrote:
Hi,
I noticed $subject while doing a MAKEALL.
It seems that this commit: http://git.denx.de/?p=u-boot.git;a=commit;h=73c25753060c58e4c339fba306ed0ded...
Breaks things with Fedora's arm toolchain:
[hans@shalem u-boot]$ scripts/binutils-version.sh arm-linux-gnu-as scripts/binutils-version.sh: line 22: printf: GNU: invalid number scripts/binutils-version.sh: line 22: printf: assembler: invalid number 0000 scripts/binutils-version.sh: line 22: printf: version: invalid number 0002 2400
And:
[hans@shalem ~]$ arm-linux-gnu-as --version GNU assembler version 2.24.0-6.fc21 20140613 Copyright 2013 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or later. This program has absolutely no warranty. This assembler was configured for a target of `arm-linux-gnueabi'.
Which makes $version_string: "GNU assembler version 2.24.0-6.fc21 20140613" and $MAJOR: "GNU assembler version 2"
FWIW it works on "vanilla" toolchains too so it's something in the tweaking above that needs further tweaks to work for Fedora and Linaro: $ /opt/eldk-5.5.2/armv7a/sysroots/i686-eldk-linux/usr/bin/arm-linux-gnueabi/arm-linux-gnueabi-as --version GNU assembler (GNU Binutils) 2.23.2
Hi all,
ping ? current master still has this regression, it is not fatal, but it is not pretty either.
Regards,
Hans
On 30-12-14 11:55, Hans de Goede wrote:
Hi,
I noticed $subject while doing a MAKEALL.
It seems that this commit: http://git.denx.de/?p=u-boot.git;a=commit;h=73c25753060c58e4c339fba306ed0ded...
Breaks things with Fedora's arm toolchain:
[hans@shalem u-boot]$ scripts/binutils-version.sh arm-linux-gnu-as scripts/binutils-version.sh: line 22: printf: GNU: invalid number scripts/binutils-version.sh: line 22: printf: assembler: invalid number 0000 scripts/binutils-version.sh: line 22: printf: version: invalid number 0002 2400
And:
[hans@shalem ~]$ arm-linux-gnu-as --version GNU assembler version 2.24.0-6.fc21 20140613 Copyright 2013 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or later. This program has absolutely no warranty. This assembler was configured for a target of `arm-linux-gnueabi'.
Which makes $version_string: "GNU assembler version 2.24.0-6.fc21 20140613" and $MAJOR: "GNU assembler version 2"
Regards,
Hans
On Tue, Jan 06, 2015 at 11:27:43AM +0100, Hans de Goede wrote:
Hi all,
ping ? current master still has this regression, it is not fatal, but it is not pretty either.
Did you see my earlier reply? It's OK with vanilla toolchains (see ELDK) and Linaro ones, but breaking on how Fedora mangles the version string. I'm _not_ saying it's a Fedora problem, but can you poke at this a bit? If not, I'll play with echo and see if I can do it. Thanks!
Regards,
Hans
On 30-12-14 11:55, Hans de Goede wrote:
Hi,
I noticed $subject while doing a MAKEALL.
It seems that this commit: http://git.denx.de/?p=u-boot.git;a=commit;h=73c25753060c58e4c339fba306ed0ded...
Breaks things with Fedora's arm toolchain:
[hans@shalem u-boot]$ scripts/binutils-version.sh arm-linux-gnu-as scripts/binutils-version.sh: line 22: printf: GNU: invalid number scripts/binutils-version.sh: line 22: printf: assembler: invalid number 0000 scripts/binutils-version.sh: line 22: printf: version: invalid number 0002 2400
And:
[hans@shalem ~]$ arm-linux-gnu-as --version GNU assembler version 2.24.0-6.fc21 20140613 Copyright 2013 Free Software Foundation, Inc. This program is free software; you may redistribute it under the terms of the GNU General Public License version 3 or later. This program has absolutely no warranty. This assembler was configured for a target of `arm-linux-gnueabi'.
Which makes $version_string: "GNU assembler version 2.24.0-6.fc21 20140613" and $MAJOR: "GNU assembler version 2"
Regards,
Hans
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On 6 Jan 2015, trini@ti.com wrote:
On Tue, Jan 06, 2015 at 11:27:43AM +0100, Hans de Goede wrote:
Hi all,
ping ? current master still has this regression, it is not fatal, but it is not pretty either.
Did you see my earlier reply? It's OK with vanilla toolchains (see ELDK) and Linaro ones, but breaking on how Fedora mangles the version string. I'm _not_ saying it's a Fedora problem, but can you poke at this a bit? If not, I'll play with echo and see if I can do it.
I did a shell check on this script,
shellcheck -f gcc binutils-version.sh binutils-version.sh:13:9: warning: Don't use variables in the printf format string. Use printf "..%s.." "$foo". [SC2059] binutils-version.sh:19:14: note: Double quote to prevent globbing and word splitting. [SC2086] binutils-version.sh:20:14: note: Double quote to prevent globbing and word splitting. [SC2086] binutils-version.sh:22:22: note: Double quote to prevent globbing and word splitting. [SC2086] binutils-version.sh:22:29: note: Double quote to prevent globbing and word splitting. [SC2086]
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..955267d 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 # - gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -14,9 +13,9 @@ if [ ${#gas} -eq 0 ]; then exit 1 fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' )
-MAJOR=$(echo $version_string | cut -d . -f 1) -MINOR=$(echo $version_string | cut -d . -f 2) +MAJOR=$(echo "$version_string" | cut -d . -f 1) +MINOR=$(echo "$version_string" | cut -d . -f 2)
-printf "%02d%02d\n" $MAJOR $MINOR +printf "%02d%02d\n" "$MAJOR" "$MINOR"
The main issue is quoting of the 'sed' expression. We had regex [0-9.], but we want [0-9.] so that we match a literal '.' as opposed to anything. Or so I speculate. I made a script that output the version info Hans has and after the patch it is fine.
On Tue, Jan 06, 2015 at 11:27:43AM +0100, Hans de Goede wrote:
ping ? current master still has this regression, it is not fatal, but it is not pretty either.
On 6 Jan 2015, trini@ti.com wrote:
Did you see my earlier reply? It's OK with vanilla toolchains (see ELDK) and Linaro ones, but breaking on how Fedora mangles the version string. I'm _not_ saying it's a Fedora problem, but can you poke at this a bit? If not, I'll play with echo and see if I can do it.
On 6 Jan 2015, bpringlemeir@nbsps.com wrote:
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..955267d 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 # - gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -14,9 +13,9 @@ if [ ${#gas} -eq 0 ]; then exit 1 fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' )
-MAJOR=$(echo $version_string | cut -d . -f 1) -MINOR=$(echo $version_string | cut -d . -f 2) +MAJOR=$(echo "$version_string" | cut -d . -f 1) +MINOR=$(echo "$version_string" | cut -d . -f 2)
-printf "%02d%02d\n" $MAJOR $MINOR +printf "%02d%02d\n" "$MAJOR" "$MINOR"
The main issue is quoting of the 'sed' expression. We had regex [0-9.], but we want [0-9.] so that we match a literal '.' as opposed to anything. Or so I speculate. I made a script that output the version info Hans has and after the patch it is fine.
Err none of the above. The RedHat binutils doesnt have a 'package' string. With brackets. For example,
GNU assembler (GNU Binutils for Ubuntu) 2.24 ^^^^ GNU assembler (crosstool-NG 1.20.0 - nbsps) 2.24 ^^^^ It is just,
GNU assembler version 2.24.0-6.fc21 20140613 ----------------------------------- ++++++
Below gets the first ##.## after a bracket or the first ##.## No escaping of the . is needed inside the range of course.
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..b1afc98 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 # - gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -15,6 +14,7 @@ if [ ${#gas} -eq 0 ]; then fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$(echo "$version_string" | sed -e 's/[^0-9]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
On Tue, Jan 06, 2015 at 05:01:22PM -0500, Bill Pringlemeir wrote:
On Tue, Jan 06, 2015 at 11:27:43AM +0100, Hans de Goede wrote:
ping ? current master still has this regression, it is not fatal, but it is not pretty either.
On 6 Jan 2015, trini@ti.com wrote:
Did you see my earlier reply? It's OK with vanilla toolchains (see ELDK) and Linaro ones, but breaking on how Fedora mangles the version string. I'm _not_ saying it's a Fedora problem, but can you poke at this a bit? If not, I'll play with echo and see if I can do it.
On 6 Jan 2015, bpringlemeir@nbsps.com wrote:
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..955267d 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 # - gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -14,9 +13,9 @@ if [ ${#gas} -eq 0 ]; then exit 1 fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' )
-MAJOR=$(echo $version_string | cut -d . -f 1) -MINOR=$(echo $version_string | cut -d . -f 2) +MAJOR=$(echo "$version_string" | cut -d . -f 1) +MINOR=$(echo "$version_string" | cut -d . -f 2)
-printf "%02d%02d\n" $MAJOR $MINOR +printf "%02d%02d\n" "$MAJOR" "$MINOR"
The main issue is quoting of the 'sed' expression. We had regex [0-9.], but we want [0-9.] so that we match a literal '.' as opposed to anything. Or so I speculate. I made a script that output the version info Hans has and after the patch it is fine.
Err none of the above. The RedHat binutils doesnt have a 'package' string. With brackets. For example,
GNU assembler (GNU Binutils for Ubuntu) 2.24 ^^^^ GNU assembler (crosstool-NG 1.20.0 - nbsps) 2.24 ^^^^ It is just,
GNU assembler version 2.24.0-6.fc21 20140613
++++++Below gets the first ##.## after a bracket or the first ##.## No escaping of the . is needed inside the range of course.
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..b1afc98 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 #
gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -15,6 +14,7 @@ if [ ${#gas} -eq 0 ]; then fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$(echo "$version_string" | sed -e 's/[^0-9]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
Thanks! Can you post a proper version and Hans can Tested-by it as well?
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Run a 2nd pass on the 'version_string' to strip off any leading characters when a package is not provided in brackets.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com --- scripts/binutils-version.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..68cd0fe 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,7 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 # - +set -x gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -15,6 +15,7 @@ if [ ${#gas} -eq 0 ]; then fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$(echo "$version_string" | sed -e 's/[^0-9]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Run a 2nd pass on the 'version_string' to strip off any leading characters when a package is not provided in brackets.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com --- scripts/binutils-version.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..b1afc98 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 # - gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -15,6 +14,7 @@ if [ ${#gas} -eq 0 ]; then fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$(echo "$version_string" | sed -e 's/[^0-9]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
Hi Bill,
On Tue, 6 Jan 2015 17:38:19 -0500 Bill Pringlemeir bpringlemeir@nbsps.com wrote:
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Run a 2nd pass on the 'version_string' to strip off any leading characters when a package is not provided in brackets.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com
scripts/binutils-version.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..b1afc98 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -5,7 +5,6 @@ # Prints the binutils version of `gas-command' in a canonical 4-digit form # such as `0222' for binutils 2.22 #
gas="$*"
if [ ${#gas} -eq 0 ]; then @@ -15,6 +14,7 @@ if [ ${#gas} -eq 0 ]; then fi
version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$(echo "$version_string" | sed -e 's/[^0-9]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2) -- 1.8.0.2
Thanks for your patch.
I think this works but could it be more simplified?
In your commit-log, you mentioned only some of tools provide additional information surrounded by brackets.
If so, we can [1] remove blackets [2] and then take the first word that consists of numbers+period
Like this:
version_string=$($gas --version | head -1 | \ sed -e 's/(.*)//' -e 's/[^0-9.]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
printf "%02d%02d\n" $MAJOR $MINOR
Best Regards Masahiro Yamada
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Strip out the '(package version text)' and then look for a ##.## string.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com --- scripts/binutils-version.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..a343681 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -14,7 +14,8 @@ if [ ${#gas} -eq 0 ]; then exit 1 fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$($gas --version | head -1 | \ + sed -e 's/(.*)//; s/[^0-9.]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
On Wed, 7 Jan 2015 10:34:15 -0500 Bill Pringlemeir bpringlemeir@nbsps.com wrote:
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Strip out the '(package version text)' and then look for a ##.## string.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com
scripts/binutils-version.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/binutils-version.sh b/scripts/binutils-version.sh index 0bc26cf..a343681 100755 --- a/scripts/binutils-version.sh +++ b/scripts/binutils-version.sh @@ -14,7 +14,8 @@ if [ ${#gas} -eq 0 ]; then exit 1 fi
-version_string=$($gas --version | head -1 | sed -e 's/.*) *([0-9.]*).*/\1/' ) +version_string=$($gas --version | head -1 | \
- sed -e 's/(.*)//; s/[^0-9.]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2) -- 1.8.0.2
Tested-by: Masahiro Yamada yamada.m@jp.panasonic.com
Hi,
On 07-01-15 16:34, Bill Pringlemeir wrote:
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Strip out the '(package version text)' and then look for a ##.## string.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com
Thanks, this fixes the errors I was seeing:
Tested-by: Hans de Goede hdegoede@redhat.com
Regards,
Hans
On Wed, Jan 07, 2015 at 10:34:15AM -0500, Bill Pringlemeir wrote:
Commit 73c25753 fixed the common issue that binutil packages (tool/organization that packaged or built the bin-utils) are included in brackets and this may falsely be recognized as a version. However, some tools do not provide a 'package' and previously we add the 'Gnu assembler..' to the version.
Strip out the '(package version text)' and then look for a ##.## string.
Signed-off-by: Bill Pringlemeir bpringlemeir@nbsps.com Tested-by: Masahiro Yamada yamada.m@jp.panasonic.com Tested-by: Hans de Goede hdegoede@redhat.com
Applied to u-boot/master, thanks!
On 7 Jan 2015, yamada.m@jp.panasonic.com wrote:
Thanks for your patch.
I think this works but could it be more simplified?
In your commit-log, you mentioned only some of tools provide additional information surrounded by brackets.
If so, we can [1] remove blackets [2] and then take the first word that consists of numbers+period
Like this:
version_string=$($gas --version | head -1 | \ sed -e 's/(.*)//' -e 's/[^0-9.]*([0-9.]*).*/\1/')
MAJOR=$(echo $version_string | cut -d . -f 1) MINOR=$(echo $version_string | cut -d . -f 2)
printf "%02d%02d\n" $MAJOR $MINOR
I realized I didn't need to spawn sed twice, but removing the '(package)' stuff seems better. Thanks for giving me a reason to fix the whitespace.
Also the string,
GNU assembler version 2.24.0-6.fc21 20140613
Was originally reporting '2014061320140613' prior to your version. The new version reports '0224' like all the others.
Fwiw, Bill.
participants (4)
-
Bill Pringlemeir -
Hans de Goede -
Masahiro Yamada -
Tom Rini