* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. @ 2017-02-21 9:04 Guo Ren 2017-02-21 9:26 ` Thomas Petazzoni 2017-02-21 10:05 ` Arnout Vandecappelle 0 siblings, 2 replies; 9+ messages in thread From: Guo Ren @ 2017-02-21 9:04 UTC (permalink / raw) To: buildroot some gcc --version ouput like this: $ csky-linux-gcc --version csky-linux-gcc (C-SKY Tools V2.8.08(Beta1)(Glibc,Linux 4.8.4), ABIV1, B20161118) 4.5.1 current script just find first ')', but we need find last ')' to get correct version number. Signed-off-by: Guo Ren <ren_guo@c-sky.com> --- toolchain/helpers.mk | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk index 72e7292..2d695ed 100644 --- a/toolchain/helpers.mk +++ b/toolchain/helpers.mk @@ -136,18 +136,26 @@ check_kernel_headers_version = \ # - 1!d # - delete if not line 1 # -# - s/^[^)]+\) ([^[:space:]]+).*/\1/ -# - eat all until the first ')' character followed by a space -# - match as many non-space chars as possible -# - eat all the remaining chars on the line +# - s/^.+\)[[:space:]]*([^[:space:]]+)/\1/ +# - eat all until the last ')' character +# - match the first non-space sequence # - replace by the matched expression # +# The GCC version string is built up as follows: +# GCC (PKGVERSION) BASEVER DATESTAMP DEVPHASE REVISION +# PKGVERSION can be anything, it's a string provided at configure time. +# BASEVER is what we need. +# DATESTAMP is YYYYMMDD or empty +# DEVPHASE is experimental, pre-release or empty +# REVISION is [svn revision info] or empty +# Therefore, we need the first non-space sequence after the last ')' character +# check_gcc_version = \ expected_version="$(strip $2)" ; \ if [ -z "$${expected_version}" ]; then \ exit 0 ; \ fi; \ - real_version=`$(1) --version | sed -r -e '1!d; s/^[^)]+\) ([^[:space:]]+).*/\1/;'` ; \ + real_version=`$(1) --version | sed -r -e '1!d; s/^.+\)[[:space:]]*([^[:space:]]+)/\1/;'` ; \ if [[ ! "$${real_version}" =~ ^$${expected_version}\. ]] ; then \ printf "Incorrect selection of gcc version: expected %s.x, got %s\n" \ "$${expected_version}" "$${real_version}" ; \ -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 9:04 [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version Guo Ren @ 2017-02-21 9:26 ` Thomas Petazzoni 2017-02-21 14:15 ` ren_guo 2017-02-21 14:33 ` Arnout Vandecappelle 2017-02-21 10:05 ` Arnout Vandecappelle 1 sibling, 2 replies; 9+ messages in thread From: Thomas Petazzoni @ 2017-02-21 9:26 UTC (permalink / raw) To: buildroot Hello, On Tue, 21 Feb 2017 17:04:36 +0800, Guo Ren wrote: > - real_version=`$(1) --version | sed -r -e '1!d; s/^[^)]+\) ([^[:space:]]+).*/\1/;'` ; \ > + real_version=`$(1) --version | sed -r -e '1!d; s/^.+\)[[:space:]]*([^[:space:]]+)/\1/;'` ; \ At some point, isn't something like: echo "__GNUC__.__GNUC_MINOR__" | gcc -P -E - | sed 's/ //g' simpler, more readable and probably more robust? Bonus points if you find a way to not have the spaces around the "." in the output generated by the preprocessor, which would avoid the need to call sed. Best regards, Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 9:26 ` Thomas Petazzoni @ 2017-02-21 14:15 ` ren_guo 2017-02-21 14:59 ` Peter Korsgaard 2017-02-21 14:33 ` Arnout Vandecappelle 1 sibling, 1 reply; 9+ messages in thread From: ren_guo @ 2017-02-21 14:15 UTC (permalink / raw) To: buildroot Thx Thomas, Cool Idea! but it need be retested. I couldn't find a proper way to avoid the spaces around the ".", and i think sed is ok :) Best Regards guoren On 2017?02?21? 17:26, Thomas Petazzoni wrote: > Hello, > > On Tue, 21 Feb 2017 17:04:36 +0800, Guo Ren wrote: > >> - real_version=`$(1) --version | sed -r -e '1!d; s/^[^)]+\) ([^[:space:]]+).*/\1/;'` ; \ >> + real_version=`$(1) --version | sed -r -e '1!d; s/^.+\)[[:space:]]*([^[:space:]]+)/\1/;'` ; \ > At some point, isn't something like: > > echo "__GNUC__.__GNUC_MINOR__" | gcc -P -E - | sed 's/ //g' > > simpler, more readable and probably more robust? > > Bonus points if you find a way to not have the spaces around the "." in > the output generated by the preprocessor, which would avoid the need to > call sed. > > Best regards, > > Thomas ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 14:15 ` ren_guo @ 2017-02-21 14:59 ` Peter Korsgaard 2017-02-22 2:16 ` ren_guo 0 siblings, 1 reply; 9+ messages in thread From: Peter Korsgaard @ 2017-02-21 14:59 UTC (permalink / raw) To: buildroot >>>>> "ren" == ren guo <ren_guo@c-sky.com> writes: > Thx Thomas, > Cool Idea! but it need be retested. > I couldn't find a proper way to avoid the spaces around the ".", and i > think sed is ok :) Alternatively we could drop the . and use the shell or awk or whatever to convert from space separated to .-separated: echo "__GNUC__ __GNUC_MINOR__" | gcc -P -E - | \ { read MAJOR MINOR; echo $MAJOR.$MINOR ; } Not that this is so much prettier. -- Bye, Peter Korsgaard ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 14:59 ` Peter Korsgaard @ 2017-02-22 2:16 ` ren_guo 0 siblings, 0 replies; 9+ messages in thread From: ren_guo @ 2017-02-22 2:16 UTC (permalink / raw) To: buildroot On 2017?02?21? 22:59, Peter Korsgaard wrote: > > Thx Thomas, > > Cool Idea! but it need be retested. > > > I couldn't find a proper way to avoid the spaces around the ".", and i > > think sed is ok :) > > Alternatively we could drop the . and use the shell or awk or whatever > to convert from space separated to .-separated: > > echo "__GNUC__ __GNUC_MINOR__" | gcc -P -E - | \ > { read MAJOR MINOR; echo $MAJOR.$MINOR ; } > > Not that this is so much prettier. > Thx Peter, "bash read" seems not bad :) ... But I like 'sed' a little more? Best Regards guoren ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 9:26 ` Thomas Petazzoni 2017-02-21 14:15 ` ren_guo @ 2017-02-21 14:33 ` Arnout Vandecappelle 2017-02-22 2:11 ` ren_guo 1 sibling, 1 reply; 9+ messages in thread From: Arnout Vandecappelle @ 2017-02-21 14:33 UTC (permalink / raw) To: buildroot On 21-02-17 10:26, Thomas Petazzoni wrote: > Hello, > > On Tue, 21 Feb 2017 17:04:36 +0800, Guo Ren wrote: > >> - real_version=`$(1) --version | sed -r -e '1!d; s/^[^)]+\) ([^[:space:]]+).*/\1/;'` ; \ >> + real_version=`$(1) --version | sed -r -e '1!d; s/^.+\)[[:space:]]*([^[:space:]]+)/\1/;'` ; \ > > At some point, isn't something like: > > echo "__GNUC__.__GNUC_MINOR__" | gcc -P -E - | sed 's/ //g' > > simpler, more readable and probably more robust? Sounds good to me. I ran that on my 300 toolchains and it seems to work. One thing however: some of them generate a few empty lines at the beginning, so the sed expression should also have /^$/d. Regards, Arnout > > Bonus points if you find a way to not have the spaces around the "." in > the output generated by the preprocessor, which would avoid the need to > call sed. > > Best regards, > > Thomas > -- Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 14:33 ` Arnout Vandecappelle @ 2017-02-22 2:11 ` ren_guo 0 siblings, 0 replies; 9+ messages in thread From: ren_guo @ 2017-02-22 2:11 UTC (permalink / raw) To: buildroot Hi Arnout, > Sounds good to me. I ran that on my 300 toolchains and it seems to work. One > thing however: some of them generate a few empty lines at the beginning, so the > sed expression should also have /^$/d. > > Regards, > Arnout > Good Test! Do you need I patch mail ? Or just fixup by yourself? Best Regards guoren ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 9:04 [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version Guo Ren 2017-02-21 9:26 ` Thomas Petazzoni @ 2017-02-21 10:05 ` Arnout Vandecappelle 2017-02-21 14:19 ` ren_guo 1 sibling, 1 reply; 9+ messages in thread From: Arnout Vandecappelle @ 2017-02-21 10:05 UTC (permalink / raw) To: buildroot Hi guoren, We like to have a version log in the patch. Under the Signed-off-by, add a line with --- and then a log of what changed compared to your first submission. Also put "[PATCH v2]" in the Subject (which you can do with "git format-patch -v2"). On 21-02-17 10:04, Guo Ren wrote: > some gcc --version ouput like this: > $ csky-linux-gcc --version > csky-linux-gcc (C-SKY Tools V2.8.08(Beta1)(Glibc,Linux 4.8.4), ABIV1, > B20161118) 4.5.1 > > current script just find first ')', but we need find last ')' to get > correct version number. > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> I've tested this on about 300 gcc binaries I have available, and it gives the same results as before on all of them (I don't have anything like the C-SKY version string). However, not exactly the same, see below. However, this made me realize that there is still a potential issue, see below. > --- > toolchain/helpers.mk | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk > index 72e7292..2d695ed 100644 > --- a/toolchain/helpers.mk > +++ b/toolchain/helpers.mk > @@ -136,18 +136,26 @@ check_kernel_headers_version = \ > # - 1!d > # - delete if not line 1 > # > -# - s/^[^)]+\) ([^[:space:]]+).*/\1/ > -# - eat all until the first ')' character followed by a space > -# - match as many non-space chars as possible > -# - eat all the remaining chars on the line Why did you remove this part of the regex? I think the regex should be: > +# - s/^.+\)[[:space:]]*([^[:space:]]+)/\1/ s/^.+\)[[:space:]]*([^[:space:]]+).*/\1/ so keep on eating the part that comes after BASEVER. > +# - eat all until the last ')' character > +# - match the first non-space sequence You removed the "eat spaces" part. So: # - eat all until the last ')' character # - eat any following spaces # - match the following non-space sequence # - replace by the matched expression > # - replace by the matched expression > # > +# The GCC version string is built up as follows: > +# GCC (PKGVERSION) BASEVER DATESTAMP DEVPHASE REVISION > +# PKGVERSION can be anything, it's a string provided at configure time. > +# BASEVER is what we need. > +# DATESTAMP is YYYYMMDD or empty > +# DEVPHASE is experimental, pre-release or empty Turns out that this is wrong. It should be: # DEVPHASE is (experimental), (prerelease) or empty Which brings us to the first potential problem that I found in these 300 toolchains: your new pattern will extract the wrong version when DEVPHASE is non-empty and REVISION is also non-empty. It worked for the toolchains I have, because they had an empty REVISION and the regex requires a non-space character after the last ). But it may not always work. So this patch fixes one practical case and breaks another theoretical case - still a net improvement I'd say, so I'm OK with leaving it as is. To improve this, we could probably require the match part to start with a digit. However, I don't have any toolchain with non-empty REVISION so I'm not sure if that one doesn't start with a digit either. From the code, it *looks* like the REVISION part always starts with [ but I can't be 100% sure. > +# REVISION is [svn revision info] or empty > +# Therefore, we need the first non-space sequence after the last ')' character > +# > check_gcc_version = \ > expected_version="$(strip $2)" ; \ > if [ -z "$${expected_version}" ]; then \ > exit 0 ; \ > fi; \ > - real_version=`$(1) --version | sed -r -e '1!d; s/^[^)]+\) ([^[:space:]]+).*/\1/;'` ; \ > + real_version=`$(1) --version | sed -r -e '1!d; s/^.+\)[[:space:]]*([^[:space:]]+)/\1/;'` ; \ > if [[ ! "$${real_version}" =~ ^$${expected_version}\. ]] ; then \ By not eating the DATESTAMP DEVPHASE REVISION part, nothing really changes because it will still match this pattern. Still, it's cleaner to have the actual BASEVER in real_version and not the entire version string. Here is the second problem I found while going through all my toolchains: I have some toolchains that report BASEVER as 4.8 instead of 4.8.x. This will NOT match the expected_version because of the missing . after it. Not sure how to fix that, and if we really need to fix it - it was an Android toolchain :-) But anyway, that would be for a separate patch. Regards, Arnout > printf "Incorrect selection of gcc version: expected %s.x, got %s\n" \ > "$${expected_version}" "$${real_version}" ; \ > -- Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version. 2017-02-21 10:05 ` Arnout Vandecappelle @ 2017-02-21 14:19 ` ren_guo 0 siblings, 0 replies; 9+ messages in thread From: ren_guo @ 2017-02-21 14:19 UTC (permalink / raw) To: buildroot Hi Arnout, Another path is done, pls have a check. my patch v2 is ok? Best Regards guoren On 2017?02?21? 18:05, Arnout Vandecappelle wrote: > Hi guoren, > > We like to have a version log in the patch. Under the Signed-off-by, add a line > with --- and then a log of what changed compared to your first submission. Also > put "[PATCH v2]" in the Subject (which you can do with > "git format-patch -v2"). > > On 21-02-17 10:04, Guo Ren wrote: >> some gcc --version ouput like this: >> $ csky-linux-gcc --version >> csky-linux-gcc (C-SKY Tools V2.8.08(Beta1)(Glibc,Linux 4.8.4), ABIV1, >> B20161118) 4.5.1 >> >> current script just find first ')', but we need find last ')' to get >> correct version number. >> >> Signed-off-by: Guo Ren <ren_guo@c-sky.com> > I've tested this on about 300 gcc binaries I have available, and it gives the > same results as before on all of them (I don't have anything like the C-SKY > version string). However, not exactly the same, see below. > > However, this made me realize that there is still a potential issue, see below. > >> --- >> toolchain/helpers.mk | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk >> index 72e7292..2d695ed 100644 >> --- a/toolchain/helpers.mk >> +++ b/toolchain/helpers.mk >> @@ -136,18 +136,26 @@ check_kernel_headers_version = \ >> # - 1!d >> # - delete if not line 1 >> # >> -# - s/^[^)]+\) ([^[:space:]]+).*/\1/ >> -# - eat all until the first ')' character followed by a space >> -# - match as many non-space chars as possible >> -# - eat all the remaining chars on the line > Why did you remove this part of the regex? I think the regex should be: > >> +# - s/^.+\)[[:space:]]*([^[:space:]]+)/\1/ > s/^.+\)[[:space:]]*([^[:space:]]+).*/\1/ > > so keep on eating the part that comes after BASEVER. > >> +# - eat all until the last ')' character >> +# - match the first non-space sequence > You removed the "eat spaces" part. So: > > # - eat all until the last ')' character > # - eat any following spaces > # - match the following non-space sequence > # - replace by the matched expression > >> # - replace by the matched expression >> # >> +# The GCC version string is built up as follows: >> +# GCC (PKGVERSION) BASEVER DATESTAMP DEVPHASE REVISION >> +# PKGVERSION can be anything, it's a string provided at configure time. >> +# BASEVER is what we need. >> +# DATESTAMP is YYYYMMDD or empty >> +# DEVPHASE is experimental, pre-release or empty > Turns out that this is wrong. It should be: > > # DEVPHASE is (experimental), (prerelease) or empty > > Which brings us to the first potential problem that I found in these 300 > toolchains: your new pattern will extract the wrong version when DEVPHASE is > non-empty and REVISION is also non-empty. It worked for the toolchains I have, > because they had an empty REVISION and the regex requires a non-space character > after the last ). But it may not always work. > > So this patch fixes one practical case and breaks another theoretical case - > still a net improvement I'd say, so I'm OK with leaving it as is. > > To improve this, we could probably require the match part to start with a > digit. However, I don't have any toolchain with non-empty REVISION so I'm not > sure if that one doesn't start with a digit either. From the code, it *looks* > like the REVISION part always starts with [ but I can't be 100% sure. > > >> +# REVISION is [svn revision info] or empty >> +# Therefore, we need the first non-space sequence after the last ')' character >> +# >> check_gcc_version = \ >> expected_version="$(strip $2)" ; \ >> if [ -z "$${expected_version}" ]; then \ >> exit 0 ; \ >> fi; \ >> - real_version=`$(1) --version | sed -r -e '1!d; s/^[^)]+\) ([^[:space:]]+).*/\1/;'` ; \ >> + real_version=`$(1) --version | sed -r -e '1!d; s/^.+\)[[:space:]]*([^[:space:]]+)/\1/;'` ; \ >> if [[ ! "$${real_version}" =~ ^$${expected_version}\. ]] ; then \ > By not eating the DATESTAMP DEVPHASE REVISION part, nothing really changes > because it will still match this pattern. Still, it's cleaner to have the actual > BASEVER in real_version and not the entire version string. > > Here is the second problem I found while going through all my toolchains: I > have some toolchains that report BASEVER as 4.8 instead of 4.8.x. This will NOT > match the expected_version because of the missing . after it. Not sure how to > fix that, and if we really need to fix it - it was an Android toolchain :-) > > But anyway, that would be for a separate patch. > > Regards, > Arnout > >> printf "Incorrect selection of gcc version: expected %s.x, got %s\n" \ >> "$${expected_version}" "$${real_version}" ; \ >> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-22 2:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-21 9:04 [Buildroot] [PATCH 1/1] toolchain/helpers.mk: fixup check_gcc_version Guo Ren 2017-02-21 9:26 ` Thomas Petazzoni 2017-02-21 14:15 ` ren_guo 2017-02-21 14:59 ` Peter Korsgaard 2017-02-22 2:16 ` ren_guo 2017-02-21 14:33 ` Arnout Vandecappelle 2017-02-22 2:11 ` ren_guo 2017-02-21 10:05 ` Arnout Vandecappelle 2017-02-21 14:19 ` ren_guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox