Re: [Xen-devel] [PATCH RFC 1/2] scripts: Add script to do the repetitive bits of the release process

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Hi.  Thanks for looking into this.  Sorry about the delay to the
review.  I found it, unsent, while recovering a crashed mailreader
session...

George Dunlap writes ("[PATCH RFC 1/2] scripts: Add script to do the repetitive 
bits of the release process"):
> With this script, once the main checks are out of the way, doing a
> release (either an RC or the final release) should mostly be a matter
> of executing a sequence of 4 commands given by the `help` function in
> this script.

Script is missing set -e.  I'm generally not happy with the || fail
pattern, except for improving error messages.  It makes it far too
easy to accidentally miss an error check.  Your script has at least a
few missing error checks and I don't want to read it and try to spot
them all.  Can you change this ?

> +###
> +# George's bash library core
> +###

You've cloned (and presumably not hacked) some personal library of
your own ?  What's wrong with getopt(1) eg
   eval "$(getopt -s bash ... "$@")"
See /usr/share/doc/util-linux/examples/getopt-parse.bash.

Although, it's not clear to me that a full-blown option parser is
a better approach than
  var1=DEFAULT1
  var2=DEFAULT2
  for x in "$@"; do eval "$x"; done
for this script, since it will have few users.

> +function cmdline()

I think the keyword `function' here is unidiomatic shell.  (I think the
brace place is too but I don't want to quibble about style.)

+    info Running "${args[0]}"
+    "${args[0]}" "${args[@]:1}" || exit 1

You should probably namespace these with an appropriate prefix, rather
than exposing every internal function as a toplevel verb.

> +function xen-make-prefix-config() {
> +    $arg_parse
> +
> +    # TODO: Ping drall.uk.xensource.com to see if we can reach it?
> +    
> +    default cache_prefix "git://drall.uk.xensource.com:9419/" ; $default_post
> +
> +    perl -ne "if(/^([A-Z_]+_URL) \?= (git.*)/) { print \"\$1 ?= 
> ${cache_prefix}\$2\n\"; }" Config.mk >> .config || fail "Generating .config"
> +    cat .config
> +}

Maybe we can expect the caller to have this in their global git
config.  I do this:

mariner:~> git-config -l | grep instead
url.git://git-cache.xs.citrite.net:9419/git://.insteadof=git://
url.git://git-cache.xs.citrite.net:9419/.insteadof=git://git-cache.xs.citrite.net:9419/
url.git://drall:9419/http://.insteadof=git://drall:9419/http://
url.git://drall:9419/https://.insteadof=git://drall:9419/https://
url.git://drall:9419/git://.insteadof=git://drall:9419/git://
mariner:~>

> +function set-tdir() {
> +    if [[ -z "$tdir" || ! -e "$tdir" ]] ; then
> +     info "$tdir doesn't exist, using /tmp"
> +     tdir="/tmp"
> +    fi

It is not OK to use /tmp/$v, because of tmpfile races.  If you don't
want to use somewhere in ~ then you need to mess with mktemp.

> +function tag() {
> +    $arg_parse

Overall I am surprised at how much code there is in this script.  It
seems much longer than the current runes in the release checklist.

> +    if [[ -n "$c" ]] ; then
> +     info "Checking out commit $c"
> +     git checkout $c || fail
> +    else
> +     local q
> +     git checkout stable-$x || fail "Couldn't check out stable branch"
> +     git merge || fail "Merge"

Surely we rarely want to do this, and never automatically.

> +     git log -n 10
> +     read -p "Enter to continue, anything else to quit: " q

Better to ask for a "y".  read might return "" due to eof.

> +    cvs ci -m $v || fail "cvs checkin"
> +
> +    ssh mail.xenproject.org "cd /data/downloads.xenproject.org/xen.org && 
> cvs -q up -d" || fail "Deploying tarball"

Should surely read   ssh downloads.xenproject.org   and then it should
be a variable.  Also the scriptlet could be formatted across
multiple lines (and start with set -e, rather than using &&).

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel