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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 8/5/19 11:57 AM, Ian Jackson wrote:
> Hi.  Thanks for looking into this.  Sorry about the delay to the
> review.  I found it, unsent, while recovering a crashed mailreader
> session...

Thanks for the review -- I was going to come back and ping this at some
point; it would be nice to have it in tree before we actually need it
again. :-)

So just in general: I'm willing to make fixes and small tweaks, but I'm
not going to invest a lot of time rewriting this script.  It was useful
for me; if you don't want it in tree, then I'll just use it myself when
I find it useful.

> 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 ?
I've never run with `set -e` before, but let me give it a try and see
how it goes.

>> +###
>> +# George's bash library core
>> +###
> 
> You've cloned (and presumably not hacked) some personal library of
> your own ?

I C&P my personal library, yes.  (Not sure what "hacked" means in this
context.)  I've got a reasonably extensive set of personal scripts that
use this calling convention as their core.  It's also the basis for a
bunch of scripting I did around the CentOS package maintenanace:

https://github.com/CentOS-virt7/xen/tree/xen-412/lib

(I've C&P'd here a subset of 'core.sh' in that directory.)

>  What's wrong with getopt(1) eg
>    eval "$(getopt -s bash ... "$@")"

* It's ugly and hard to read naturally

* You have to think about short names for every option you want; if you
want more than 26, then you start to run out of meaningful letters.

* You have to encode everything in a single line of runes.  "$arg_parse"
sets local variables for all your arguments; "$requireargs" is a more
natual way of saying what's required, and "$default" allows line-by-line
specification of defaults if something isn't specified.

* You don't get "inheritance".  One of the explicit reasons I developed
this type of framework was so that I could do something like:

```
tgt=c6_01;

vm-start

tgt-ssh "<some command>"

vm-shutdown
```

(In case it isn't clear, `vm-start`, `tgt-ssh`, and `vm-shutdown` all
take `tgt` as an argument.)

Obviously this makes some things easier by making other things more
dangerous; but bash already has inherited variables anyway.  If I wanted
real scoping I'd switch to a locally-scoped programming language.

In any case, in line with what I said above -- I'm used to writing bash
scripts in this way; I find it useful.  And although the core
"arg_parse" machinery is somewhat ugly, it's been tested and refined
over several years.  I'm not going to spend a lot of time rewriting this
script to do something else.

>> +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.)

What would you prefer instead?

> 
> +    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.

You mean namespace the internal "library" functions like `fail`?  Or
helper functions like `tag`?

Either way, I don't see a big issue from allowing them to be called from
the command-line: sometimes it's helpful (if only for unit testing); and
although calling `release fail "blah"` seems a bit pointless, it's not
harmful, and I'd prefer to keep names of the "library" functions short.

>> +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:~>

It looks like only the first one of those does anything?  Or am I
misunderstanding the syntax?

At any rate, yes, that's probably a cleaner solution; we can add that to
the instructions for the script.  (I didn't know about `insteadof`.)

>> +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.

Some of it is boilerplate to be able to make functions; a lot of it is
encoding error and exception handling which is implicit in a checklist
(because expert humans know what an error looks like and how to fix
things up).

>> +    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.

Oh, this should have an `--ff-only`.  (I've got --ff-only enabled by
default in my .gitconfig).  So for me this will just update to the
newest commit on the origin/stable-$x branch (and fail if an actual
merge commit would be needed).

If we don't want to auto-ff, then we should check to see how many
commits we are away from upstream and fail out if that number is 0.

>> +    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.

Sure.

>> +    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 &&).

Can you 'set -e' in an ssh command like this?

In any case -- before I invest a lot of time cleaning this up further,
are you willing to take it with my quirky calling conventions rather
than getopt?  If not I'll probably just keep this locally.

 -George

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