| Message ID | 1473772858-30881-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/13/2016 08:20 AM, Thomas Huth wrote: > Basic idea of this script is to check the git log for URLs > to the QEMU bugtracker at launchpad.net and to figure out > whether the related bug has been marked there as "Fix released" > (i.e. closed) already. So this script can e.g. be used after > each public release of QEMU to check whether there are any > bug tickets that could be moved from "Fix committed" (or another > state if the author of the patch forgot to update the bug ticket) > to "Fix released". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v2: > - Use xdg-open and friends to open the URLs in a browser > - Some cosmetics > > scripts/show-fixed-bugs.sh | 91 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > create mode 100755 scripts/show-fixed-bugs.sh > > diff --git a/scripts/show-fixed-bugs.sh b/scripts/show-fixed-bugs.sh > new file mode 100755 > index 0000000..89847bd > --- /dev/null > +++ b/scripts/show-fixed-bugs.sh > @@ -0,0 +1,91 @@ > +#!/bin/sh > + > +# This script checks the git log for URLs to the QEMU launchpad bugtracker > +# and optionally checks whether the corresponding bugs are not closed yet. > + > +function show_help { > + echo "Usage:" > + echo " -s <commit> : Start searching at this commit" > + echo " -e <commit> : End searching at this commit" > + echo " -c : Check if bugs are still open" > + echo " -b : Open bugs in browser" > +} > + > +while [ $# -ge 1 ]; do > + case "$1" in > + -s) START="$2" ; shift ;; POSIX recommends that short options with arguments be parseable both as '-s foo' and '-sfoo'. I don't care that you aren't POSIX compliant, but using getopt(1) or getopts(1) may make it easier to comply. Using ALL_CAPS variables in a shell script risks collisions with environment variables that might have other uses. It may be better to use lower case variables to make it obvious that they are for internal use. > + -e) END="$2" ; shift ;; > + -c) CHECK_IF_OPEN=1 ;; > + -b) SHOW_IN_BROWSER=1 ;; > + -h) show_help ; exit 0 ;; > + *) echo "Unkown option $1 ... use -h for help." ; exit 1 ;; s/Unkown/Unknown/ > + esac > + shift > +done > + > +if [ "x$START" = "x" ]; then > + START=`git tag | grep 'v[0-9]*\.[0-9]*.0$' | tail -n 2 | head -n 1` You can shorten 'git tag | grep' via: git tag -l 'v[0-9]*.[0-9]*.0' > +fi > +if [ "x$END" = "x" ]; then > + END=`git tag | grep 'v[0-9]*\.[0-9]*.0$' | tail -n 1` and again > +fi > + > +if [ "x$BROWSER" != "x" ]; then > + BUGBROWSER = "$BROWSER" > +elif which xdg-open > /dev/null; then 'which' is not portable; it may not exist. It looks like you are trying to redirect to /dev/null to avoid shell messages when it does not exist, but to properly do that, you'd need: (which xdg-open) >/dev/null 2>&1 (the subshell is necessary, since some shells don't shut up without it). > + BUGBROWSER=xdg-open > +elif which gnome-open > /dev/null; then > + BUGBROWSER=gnome-open and again > +elif [ `uname` = "Darwin" ]; then > + BUGBROWSER=open Improperly quoted. You want "`uname`" to make sure that random spaces don't cause the wrong number of arguments to '['. > +elif which sensible-browser > /dev/null; then > + BUGBROWSER=sensible-browser another nonportable which. > +else > + echo "Please set the BROWSER variable to the browser of your choice." > + exit 1 > +fi > + > +if [ "x$START" = "x" -o "x$END" = "x" ]; then [ ... -o ... ] is not portable. POSIX itself recommends that you use: [ "x$START" = "x" ] || [ "x$END" = "x" ] > + echo "Could not determine start or end revision ... Please note that this" > + echo "script must be run from a checked out git repository of QEMU!" > + exit 1 > +fi > + > +echo "Searching git log for bugs in the range $START..$END" > + > +BUG_URLS=`git log $START..$END \ > + | grep 'https://bugs.launchpad.net/\(bugs\|qemu/+bug\)/' \ > + | sed 's,\(.*\)\(https://bugs.launchpad.net/\(bugs\|qemu/+bug\)/\)\([0-9]*\).*,\2\4,' \ 'grep | sed' can usually be shortened to just 'sed'. Here, it could be: str='https://bugs.launchpad.net,\(bugs\|qemu/+bug\)' sed -n '\,'"$str"', s,\(.*\)\('"$str"'/\)\([0-9]*\).*,\2\4,p' > + | sort -u` > + > +echo Found bug URLs: > +for i in $BUG_URLS ; do echo " $i" ; done > + > +if [ "x$CHECK_IF_OPEN" = "x1" ]; then > + echo > + echo "Checking which ones are still opened..." > + for i in $BUG_URLS ; do > + if ! curl -s -L "$i" | grep "value status" | grep -q "Fix Released" ; then > + echo " $i" > + FINAL_BUG_URLS="$FINAL_BUG_URLS $i" > + fi > + done > +else > + FINAL_BUG_URLS=$BUG_URLS > +fi > + > +if [ "x$FINAL_BUG_URLS" = "x" ]; then > + echo "No open bugs found." > +elif [ "x$SHOW_IN_BROWSER" = "x1" ]; then > + FIRST=1 > + for i in $FINAL_BUG_URLS; do > + "$BUGBROWSER" "$i" > + if [ $FIRST = 1 ]; then > + # if it is the first entry, give the browser some time to start > + # (to avoid messages like "Firefox is already running, but is > + # not responding...") > + sleep 4 > + FIRST=0 > + fi > + done > +fi >
On Tue, Sep 13, 2016 at 10:48:46AM -0500, Eric Blake wrote: > On 09/13/2016 08:20 AM, Thomas Huth wrote: > > Basic idea of this script is to check the git log for URLs > > to the QEMU bugtracker at launchpad.net and to figure out > > whether the related bug has been marked there as "Fix released" > > (i.e. closed) already. So this script can e.g. be used after > > each public release of QEMU to check whether there are any > > bug tickets that could be moved from "Fix committed" (or another > > state if the author of the patch forgot to update the bug ticket) > > to "Fix released". > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > v2: > > - Use xdg-open and friends to open the URLs in a browser > > - Some cosmetics > > > > scripts/show-fixed-bugs.sh | 91 ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > create mode 100755 scripts/show-fixed-bugs.sh > > > > diff --git a/scripts/show-fixed-bugs.sh b/scripts/show-fixed-bugs.sh > > new file mode 100755 > > index 0000000..89847bd > > --- /dev/null > > +++ b/scripts/show-fixed-bugs.sh > > @@ -0,0 +1,91 @@ > > +#!/bin/sh > > + > > +# This script checks the git log for URLs to the QEMU launchpad bugtracker > > +# and optionally checks whether the corresponding bugs are not closed yet. > > + > > +function show_help { > > + echo "Usage:" > > + echo " -s <commit> : Start searching at this commit" > > + echo " -e <commit> : End searching at this commit" > > + echo " -c : Check if bugs are still open" > > + echo " -b : Open bugs in browser" > > +} > > + > > +while [ $# -ge 1 ]; do > > + case "$1" in > > + -s) START="$2" ; shift ;; > > POSIX recommends that short options with arguments be parseable both as > '-s foo' and '-sfoo'. I don't care that you aren't POSIX compliant, but > using getopt(1) or getopts(1) may make it easier to comply. > > Using ALL_CAPS variables in a shell script risks collisions with > environment variables that might have other uses. It may be better to > use lower case variables to make it obvious that they are for internal use. > > > + -e) END="$2" ; shift ;; > > + -c) CHECK_IF_OPEN=1 ;; > > + -b) SHOW_IN_BROWSER=1 ;; > > + -h) show_help ; exit 0 ;; > > + *) echo "Unkown option $1 ... use -h for help." ; exit 1 ;; > > s/Unkown/Unknown/ > > > + esac > > + shift > > +done > > + > > +if [ "x$START" = "x" ]; then > > + START=`git tag | grep 'v[0-9]*\.[0-9]*.0$' | tail -n 2 | head -n 1` > > You can shorten 'git tag | grep' via: > > git tag -l 'v[0-9]*.[0-9]*.0' > > > +fi > > +if [ "x$END" = "x" ]; then > > + END=`git tag | grep 'v[0-9]*\.[0-9]*.0$' | tail -n 1` > > and again > > > +fi > > + > > +if [ "x$BROWSER" != "x" ]; then > > + BUGBROWSER = "$BROWSER" > > +elif which xdg-open > /dev/null; then > > 'which' is not portable; it may not exist. It looks like you are trying > to redirect to /dev/null to avoid shell messages when it does not exist, > but to properly do that, you'd need: > > (which xdg-open) >/dev/null 2>&1 > > (the subshell is necessary, since some shells don't shut up without it). > > > + BUGBROWSER=xdg-open > > +elif which gnome-open > /dev/null; then > > + BUGBROWSER=gnome-open > > and again > > > +elif [ `uname` = "Darwin" ]; then > > + BUGBROWSER=open > > Improperly quoted. You want "`uname`" to make sure that random spaces > don't cause the wrong number of arguments to '['. > > > +elif which sensible-browser > /dev/null; then > > + BUGBROWSER=sensible-browser > > another nonportable which. > > > +else > > + echo "Please set the BROWSER variable to the browser of your choice." > > + exit 1 > > +fi > > + > > +if [ "x$START" = "x" -o "x$END" = "x" ]; then > > [ ... -o ... ] is not portable. POSIX itself recommends that you use: > > [ "x$START" = "x" ] || [ "x$END" = "x" ] > > > + echo "Could not determine start or end revision ... Please note that this" > > + echo "script must be run from a checked out git repository of QEMU!" > > + exit 1 > > +fi > > + > > +echo "Searching git log for bugs in the range $START..$END" > > + > > +BUG_URLS=`git log $START..$END \ > > + | grep 'https://bugs.launchpad.net/\(bugs\|qemu/+bug\)/' \ > > + | sed 's,\(.*\)\(https://bugs.launchpad.net/\(bugs\|qemu/+bug\)/\)\([0-9]*\).*,\2\4,' \ > > 'grep | sed' can usually be shortened to just 'sed'. Here, it could be: > > str='https://bugs.launchpad.net,\(bugs\|qemu/+bug\)' > sed -n '\,'"$str"', s,\(.*\)\('"$str"'/\)\([0-9]*\).*,\2\4,p' Your various comments above are a great example of why IMHO any script that is longer than 1 line shouldn't be written in shell. Instead pick python or any other scripting language which isn't rammed full of portability problems and shockingly awful error handling facilities as shell is. Regards, Daniel
Erik, thanks a lot for your detailed review and comments ... I somewhat suspected that my shell-script-fu is crap, but now I also have the confirmation ;-) Anyway, some comments below... On 13.09.2016 18:06, Daniel P. Berrange wrote: > On Tue, Sep 13, 2016 at 10:48:46AM -0500, Eric Blake wrote: >> On 09/13/2016 08:20 AM, Thomas Huth wrote: >>> Basic idea of this script is to check the git log for URLs >>> to the QEMU bugtracker at launchpad.net and to figure out >>> whether the related bug has been marked there as "Fix released" >>> (i.e. closed) already. So this script can e.g. be used after >>> each public release of QEMU to check whether there are any >>> bug tickets that could be moved from "Fix committed" (or another >>> state if the author of the patch forgot to update the bug ticket) >>> to "Fix released". >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> v2: >>> - Use xdg-open and friends to open the URLs in a browser >>> - Some cosmetics >>> >>> scripts/show-fixed-bugs.sh | 91 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 91 insertions(+) >>> create mode 100755 scripts/show-fixed-bugs.sh >>> >>> diff --git a/scripts/show-fixed-bugs.sh b/scripts/show-fixed-bugs.sh >>> new file mode 100755 >>> index 0000000..89847bd >>> --- /dev/null >>> +++ b/scripts/show-fixed-bugs.sh >>> @@ -0,0 +1,91 @@ >>> +#!/bin/sh >>> + >>> +# This script checks the git log for URLs to the QEMU launchpad bugtracker >>> +# and optionally checks whether the corresponding bugs are not closed yet. >>> + >>> +function show_help { >>> + echo "Usage:" >>> + echo " -s <commit> : Start searching at this commit" >>> + echo " -e <commit> : End searching at this commit" >>> + echo " -c : Check if bugs are still open" >>> + echo " -b : Open bugs in browser" >>> +} >>> + >>> +while [ $# -ge 1 ]; do >>> + case "$1" in >>> + -s) START="$2" ; shift ;; >> >> POSIX recommends that short options with arguments be parseable both as >> '-s foo' and '-sfoo'. I don't care that you aren't POSIX compliant, but >> using getopt(1) or getopts(1) may make it easier to comply. OK. After googling a little bit, it sounds like getopts is the way to go, e.g. http://mywiki.wooledge.org/BashFAQ/035#getopts says that getopt should not be used. [...] >>> +if [ "x$BROWSER" != "x" ]; then >>> + BUGBROWSER = "$BROWSER" >>> +elif which xdg-open > /dev/null; then >> >> 'which' is not portable; it may not exist. It looks like you are trying >> to redirect to /dev/null to avoid shell messages when it does not exist, >> but to properly do that, you'd need: I rather wanted to simply suppress the output of which. Is there a POSIX compliant way to check whether a program is available in $PATH ? > Your various comments above are a great example of why IMHO any script that > is longer than 1 line shouldn't be written in shell. Instead pick python > or any other scripting language which isn't rammed full of portability > problems and shockingly awful error handling facilities as shell is. I fully agree with you that writing portable shell-scripts is a pain, but unfortunately my python-fu is even worse than my shell-script-fu ... well, maybe it's now time to improve it ;-) Thomas
On 09/14/2016 05:35 AM, Thomas Huth wrote: > Erik, It's Eric, but don't sweat it (you're not the first, and probably not the last, to typo names) >>>> +while [ $# -ge 1 ]; do >>>> + case "$1" in >>>> + -s) START="$2" ; shift ;; >>> >>> POSIX recommends that short options with arguments be parseable both as >>> '-s foo' and '-sfoo'. I don't care that you aren't POSIX compliant, but >>> using getopt(1) or getopts(1) may make it easier to comply. > > OK. After googling a little bit, it sounds like getopts is the way to > go, e.g. http://mywiki.wooledge.org/BashFAQ/035#getopts says that getopt > should not be used. getopts(1) is POSIX, but not universally present. I _think_ your script was portable to /bin/sh, but it may be easier to write by relying on bashisms and changing line one to /bin/bash, at which point we know getopts is present. > > [...] >>>> +if [ "x$BROWSER" != "x" ]; then >>>> + BUGBROWSER = "$BROWSER" >>>> +elif which xdg-open > /dev/null; then >>> >>> 'which' is not portable; it may not exist. It looks like you are trying >>> to redirect to /dev/null to avoid shell messages when it does not exist, >>> but to properly do that, you'd need: > > I rather wanted to simply suppress the output of which. > > Is there a POSIX compliant way to check whether a program is available > in $PATH ? 'type' and 'command' are both supposed to be able to do it according to PSOIX, although there are probably portability pitfalls with both of them; again, using /bin/bash can avoid some of the concerns about portabilities. Sadly, both of those names are hard to grep for, so I can't quickly determine if we have existing shell scripts in qemu.git that have an example to copy from. > >> Your various comments above are a great example of why IMHO any script that >> is longer than 1 line shouldn't be written in shell. Instead pick python >> or any other scripting language which isn't rammed full of portability >> problems and shockingly awful error handling facilities as shell is. > > I fully agree with you that writing portable shell-scripts is a pain, > but unfortunately my python-fu is even worse than my shell-script-fu ... > well, maybe it's now time to improve it ;-) > > Thomas > >
On 14.09.2016 16:44, Eric Blake wrote: > On 09/14/2016 05:35 AM, Thomas Huth wrote: >> Erik, > > It's Eric, but don't sweat it (you're not the first, and probably not > the last, to typo names) Oops, big sorry! I must have been confused by the German spelling of that name :-( >>>>> +while [ $# -ge 1 ]; do >>>>> + case "$1" in >>>>> + -s) START="$2" ; shift ;; >>>> >>>> POSIX recommends that short options with arguments be parseable both as >>>> '-s foo' and '-sfoo'. I don't care that you aren't POSIX compliant, but >>>> using getopt(1) or getopts(1) may make it easier to comply. >> >> OK. After googling a little bit, it sounds like getopts is the way to >> go, e.g. http://mywiki.wooledge.org/BashFAQ/035#getopts says that getopt >> should not be used. > > getopts(1) is POSIX, but not universally present. I _think_ your script > was portable to /bin/sh, but it may be easier to write by relying on > bashisms and changing line one to /bin/bash, at which point we know > getopts is present. Well, /bin/bash is also not really portable ... I've seen systems in the past where bash was installed in another directory or not at all... Anyway, FYI, I've found two more nice ways to check for POSIX compliance: - There is a program called checkbashisms which reports bash related style - "posh" is a very minimalistic POSIX compliant shell which hardly supports any of the bash extras And indeed, both pointed me to another bashism in my script: The "function" keyword is not portable and should be avoided... oh well. Not sure whether I really should do a v3 of my patch, convert it to python or just give up the idea of releasing such a script to the public... Thomas
On 09/14/2016 04:02 PM, Thomas Huth wrote: > Well, /bin/bash is also not really portable ... I've seen systems in the > past where bash was installed in another directory or not at all... True, but we already liberally use /bin/bash scripts elsewhere in qemu.git, so at least you wouldn't be the first, and if someone wants to build qemu on a platform where /bin/bash doesn't exist, they'd do a search-and-replace change to all affected scripts. > > Anyway, FYI, I've found two more nice ways to check for POSIX compliance: > > - There is a program called checkbashisms which reports bash related > style > > - "posh" is a very minimalistic POSIX compliant shell which hardly > supports any of the bash extras Yep, both of those are nice. > > And indeed, both pointed me to another bashism in my script: The > "function" keyword is not portable and should be avoided... oh well. In fact, even bash users discourage the use of the 'function' keyword. It exists because ksh has it, but ksh gives it different semantics than bash (what's worse, 'local' variables in bash functions always have dynamic scope; while in ksh, 'local' variables in POSIX-style functions have static scope and the only way to get dynamic scope is to use the 'function' keyword, except that the ksh maintainer says that he hates dynamic scope and wishes he hadn't done it). > Not > sure whether I really should do a v3 of my patch, convert it to python > or just give up the idea of releasing such a script to the public... I think keeping it as a shell script is probably okay (certainly easier than trying to convert it to python, at this point in the review process). And it does seem like a useful script. Sometimes, it's hard to see the forest (improving the ecosystem by accepting useful scripts into the project, even if only a handful of people ever run the script) for the trees (nitpicking on portability details that won't impact anyone who never runs the script).
diff --git a/scripts/show-fixed-bugs.sh b/scripts/show-fixed-bugs.sh new file mode 100755 index 0000000..89847bd --- /dev/null +++ b/scripts/show-fixed-bugs.sh @@ -0,0 +1,91 @@ +#!/bin/sh + +# This script checks the git log for URLs to the QEMU launchpad bugtracker +# and optionally checks whether the corresponding bugs are not closed yet. + +function show_help { + echo "Usage:" + echo " -s <commit> : Start searching at this commit" + echo " -e <commit> : End searching at this commit" + echo " -c : Check if bugs are still open" + echo " -b : Open bugs in browser" +} + +while [ $# -ge 1 ]; do + case "$1" in + -s) START="$2" ; shift ;; + -e) END="$2" ; shift ;; + -c) CHECK_IF_OPEN=1 ;; + -b) SHOW_IN_BROWSER=1 ;; + -h) show_help ; exit 0 ;; + *) echo "Unkown option $1 ... use -h for help." ; exit 1 ;; + esac + shift +done + +if [ "x$START" = "x" ]; then + START=`git tag | grep 'v[0-9]*\.[0-9]*.0$' | tail -n 2 | head -n 1` +fi +if [ "x$END" = "x" ]; then + END=`git tag | grep 'v[0-9]*\.[0-9]*.0$' | tail -n 1` +fi + +if [ "x$BROWSER" != "x" ]; then + BUGBROWSER = "$BROWSER" +elif which xdg-open > /dev/null; then + BUGBROWSER=xdg-open +elif which gnome-open > /dev/null; then + BUGBROWSER=gnome-open +elif [ `uname` = "Darwin" ]; then + BUGBROWSER=open +elif which sensible-browser > /dev/null; then + BUGBROWSER=sensible-browser +else + echo "Please set the BROWSER variable to the browser of your choice." + exit 1 +fi + +if [ "x$START" = "x" -o "x$END" = "x" ]; then + echo "Could not determine start or end revision ... Please note that this" + echo "script must be run from a checked out git repository of QEMU!" + exit 1 +fi + +echo "Searching git log for bugs in the range $START..$END" + +BUG_URLS=`git log $START..$END \ + | grep 'https://bugs.launchpad.net/\(bugs\|qemu/+bug\)/' \ + | sed 's,\(.*\)\(https://bugs.launchpad.net/\(bugs\|qemu/+bug\)/\)\([0-9]*\).*,\2\4,' \ + | sort -u` + +echo Found bug URLs: +for i in $BUG_URLS ; do echo " $i" ; done + +if [ "x$CHECK_IF_OPEN" = "x1" ]; then + echo + echo "Checking which ones are still opened..." + for i in $BUG_URLS ; do + if ! curl -s -L "$i" | grep "value status" | grep -q "Fix Released" ; then + echo " $i" + FINAL_BUG_URLS="$FINAL_BUG_URLS $i" + fi + done +else + FINAL_BUG_URLS=$BUG_URLS +fi + +if [ "x$FINAL_BUG_URLS" = "x" ]; then + echo "No open bugs found." +elif [ "x$SHOW_IN_BROWSER" = "x1" ]; then + FIRST=1 + for i in $FINAL_BUG_URLS; do + "$BUGBROWSER" "$i" + if [ $FIRST = 1 ]; then + # if it is the first entry, give the browser some time to start + # (to avoid messages like "Firefox is already running, but is + # not responding...") + sleep 4 + FIRST=0 + fi + done +fi
Basic idea of this script is to check the git log for URLs to the QEMU bugtracker at launchpad.net and to figure out whether the related bug has been marked there as "Fix released" (i.e. closed) already. So this script can e.g. be used after each public release of QEMU to check whether there are any bug tickets that could be moved from "Fix committed" (or another state if the author of the patch forgot to update the bug ticket) to "Fix released". Signed-off-by: Thomas Huth <thuth@redhat.com> --- v2: - Use xdg-open and friends to open the URLs in a browser - Some cosmetics scripts/show-fixed-bugs.sh | 91 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100755 scripts/show-fixed-bugs.sh