<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN"> <HTML> <HEAD> <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8"> <META NAME="GENERATOR" CONTENT="GtkHTML/3.32.2"> </HEAD> <BODY> On Fri, 2011-10-07 at 10:46 -0700, Jeremy Huddleston wrote: <BLOCKQUOTE TYPE=CITE> <PRE> Here's a first pass review. I'm sure I missed a few things, but this is certainly on the right track. There is a lot of whitespace inconsistency throughout.  Sometimes you indent by 4, sometimes by 3, sometimes you use tabs for 2-indents, ... it would be nice if that were all consistent.  I point out the obvious cases where it stands out in the review below. </PRE> </BLOCKQUOTE> Sure, just forgot about it. <BLOCKQUOTE TYPE=CITE> <PRE> On Oct 6, 2011, at 1:09 PM, Gaetan Nadon wrote: > The srcipt runs 'make dist' to create the tarball. We don't have to prompt > the user for the tar name, the version number or the section name. Should we 'make distcheck' instead? </PRE> </BLOCKQUOTE> Excellent question. I have considered that. My reasons were not very strong: - I had to draw the line between 'development/test' and 'releasing'     - The prereq for releasing is a fully built and tested module with version bump in the remote     - The person releasing may not be the developer.     - One may be asked to release a dozen apps by a release manager who got confirmation from       the developers that they are ready to release. - It is significantly longer, particularly xserver and modules with doc - It would make --dry-run virtually unusable - 'make dist' only requires running autoconf, not compiling - You may to release from a branch where not all the build dependencies are met. The module would configure but not build. But you know it works because you or someone else has tested it. There are many releasing scenarios other than a module maintainer releasing a single module. Let's see how it goes with other. I have never tried it. I am not really against it. It could be optional too. <BLOCKQUOTE TYPE=CITE> <PRE> > Interface > --------- > Usage: publish.sh [options] section[/module]... > > Section: > app|data|doc|driver|font|lib|mesa|proto|util|xcb| > xkeyboard-config|xserver I would prefer passing in paths to my local git checkouts over section/module. You should still be able to do the same -<version> foo by checking if the argument is a dir, if not, chop off the -* and check again. </PRE> </BLOCKQUOTE> Sorry I don't get it. Can you give me an example? An entry such as lib/private_libX11 will work. I just need "lib" to get the section and there should be either no subdir or only one subdir. I am basically following the module url: <BLOCKQUOTE> <A HREF="git://anongit.freedesktop.org/xorg/lib/libX11">git://anongit.freedesktop.org/xorg/</A> ==> <A HREF="git://anongit.freedesktop.org/xorg/lib/libX11">lib/libX11</A> or <A HREF="git://anongit.freedesktop.org/xorg/lib/libX11">git://anongit.freedesktop.org/xorg/</A> ==> xserver </BLOCKQUOTE> What would be the structure of your "local git checkouts"? Also keep in  mind the section[/module] is a format used by build.sh and it follows the module url <BLOCKQUOTE TYPE=CITE> <PRE> > ... > +check_local_changes() { > + git diff --quiet HEAD > /dev/null 2>&1 > + if [ $? -ne 0 ]; then > + echo "" > + echo "Uncommitted changes found. Did you forget to commit? Aborting." > + echo "" > + echo "You can perform a 'git stash' to save your local changes and" > + echo "a 'git stash apply' to recover them after the tarball release." > + echo "Make sure to rebuild and run 'make distcheck' again." > + echo "" > + echo "Alternatively, you can clone the module in another directory" > + echo "and run ./configure. No need to build if testing was finished." > + echo "" > + return 1 > + fi > + return 0 > +} There are whitespace inconsistencies above ^^^ > ... > +#------------------------------------------------------------------------------ > +# Function: generate_announce > +#------------------------------------------------------------------------------ > +# > +generate_announce() > +{ > + cat <<RELEASE > +Subject: [ANNOUNCE] $pkg_name $pkg_version > +To: $list_to > +CC: $list_cc > + > +`git log --no-merges "$tag_range" | git shortlog` > + > +git tag: $tar_name > + > +<A HREF="http://">http://</A>$host_current/$section_path/$tarbz2 > +MD5: `$MD5SUM $tarbz2` > +SHA1: `$SHA1SUM $tarbz2` > +SHA256: `$SHA256SUM $tarbz2` > + > +<A HREF="http://">http://</A>$host_current/$section_path/$targz > +MD5: `$MD5SUM $targz` > +SHA1: `$SHA1SUM $targz` > +SHA256: `$SHA256SUM $targz` > + > +RELEASE > +} Why are we duplicating code between release.sh and publish.sh?  </PRE> </BLOCKQUOTE> That is something to decide. I aimed to have publish.sh be a superset of release.sh in order to have only a single release script. <BLOCKQUOTE TYPE=CITE> <PRE> Can we instead have publish.sh be a smart wrapper around release.sh. </PRE> </BLOCKQUOTE> I used release.sh as a functional spec and used some of the code. Calling a script within a script introduces multiple points of failure and a lot of additional  tests. The odds of a change done in release.sh and be tested from publish.sh are next to nil. Returning information, sharing variables, error checking, user messages style and consistency are all more difficult. With the assumption that we have a single script (need to revamp the wiki), the internal implementation does not matter much, so as long as it is maintainable. I also copied the structure of build.sh (which early on I thought I could call). The only script being called is update_moduleset.sh which has already given trouble with the readlink issue (still to be solved). <BLOCKQUOTE TYPE=CITE> <PRE> Alan also sent out a release.sh patch (which I reviewed and provided a followup for) which does some of the same "intelligent" aspects but for the current module.   I would like to either see both of these exist together or update publish.sh to support "." </PRE> </BLOCKQUOTE> The thought had crossed my mind, given the existing working habits. I was waiting for comments on that. I don't have any objection. <BLOCKQUOTE TYPE=CITE> <PRE> as an argument (which would possibly fall out naturally from my suggestion above about taking paths) </PRE> </BLOCKQUOTE> <BLOCKQUOTE TYPE=CITE> <PRE> > ... > + # skip comment lines > + echo "$line" | grep "^#" > /dev/null > + if [ $? -eq 0 ]; then > + continue > + fi A tad cleaner: if echo "$line" | grep -q "^#" ; then continue; fi </PRE> </BLOCKQUOTE> Ok, copied code from build.sh. <BLOCKQUOTE TYPE=CITE> <PRE> > +#------------------------------------------------------------------------------ > +# Function: print_epilog > +#------------------------------------------------------------------------------ > +# > +print_epilog() { > + > +epilog="======== Successful Completion" > +if [ x"$NO_QUIT" != x ]; then > + if [ x"$failed_modules" != x ]; then > + epilog="======== Partial Completion" > + fi > +elif [ x"$failed_modules" != x ]; then > + epilog="======== Stopped on Error" > +fi > + > +echo "" > +echo "$epilog `date`" > + > +# Report about modules that failed for one reason or another > +if [ x"$failed_modules" != x ]; then > + echo " List of failed modules:" > + for mod in $failed_modules; do > + echo " $mod" > + done > + echo "========" > + echo "" > +fi > + > +} I'm confused why the contents of the above function are not indented... </PRE> </BLOCKQUOTE> Sore eyeballs. <BLOCKQUOTE TYPE=CITE> <PRE> > +#------------------------------------------------------------------------------ > +# Function: process_modules > +#------------------------------------------------------------------------------ > +# > +# Loop through each module to publish > +# Exit on error if --no-quit no specified ^^^ "if --no-quit was not specified" > +# > +process_modules() { > + for MODULE_RPATH in ${INPUT_MODULES}; do > + process_module > + if [ $? -ne 0 ]; then again, I would do: if ! process_module ; then ... fi > +#------------------------------------------------------------------------------ > +# Function: process_module > +#------------------------------------------------------------------------------ > +# Code 'return 0' on success to process the next module > +# Code 'return 1' on error to process next module if invoked with --no-quit > +# > +process_module() { > + > + # Skip already processed modules when resuming from a previous failure > + if [ x"$RESUME" != x ]; then > + if [ x"$RESUME" = x"$MODULE_RPATH" ]; then > + # The current module is the one that failed last time > + echo "Info: resuming from $RESUME module" > + unset RESUME > + else > + # The current module has already been processed successfully last time > + echo "Info: skipping $MODULE_RPATH in autoresume mode." > + return 0 > + fi > + fi > + > + echo "" > + echo "======== Processing \"$MODULE_RPATH\"" > + > + top_src=`pwd` > + if [ ! -d $MODULE_RPATH ] ; then > + echo "Error: $MODULE_RPATH cannot be found under $top_src." > + failed_modules="$failed_modules $MODULE_RPATH" > + return 1 ^^^ whitespace > ... > + # Mailing lists to be CC according to the project (xorg|dri|xkb) > + list_xorg_user="<A HREF="mailto:xorg@lists.freedesktop.org">xorg@lists.freedesktop.org</A>" > + list_dri_devel="<A HREF="mailto:dri-devel@lists.sourceforge.net">dri-devel@lists.sourceforge.net</A>" > + list_xkb="<A HREF="mailto:xkb@listserv.bat.ru">xkb@listserv.bat.ru</A>" Add in the xcb list? </PRE> </BLOCKQUOTE> Sure, it wasn't in release.sh <BLOCKQUOTE TYPE=CITE> <PRE> ... > + # The jhbuild moduleset to update with relase info > + --moduleset) > + check_option_args $1 $2 > + shift > + JH_MODULESET=$1 > + ;; ^^^ Whitespace inconsistency > + --*) > + echo "" > + echo "Error: unknown option: $1" > + echo "" > + usage > + exit 1 ^^^ Whitespace inconsistency > + ;; > + -*) > + echo "" > + echo "Error: unknown option: $1" > + echo "" > + usage > + exit 1 > + ;; ^^^ Whitespace inconsistency > + *) > + if [ x"${MODFILE}" != x ]; then > + echo "" > + echo "Error: specifying both modules and --modfile is not permitted" > + echo "" > + usage > + exit 1 ^^^ Whitespace inconsistency --- Jeremy Huddleston </PRE> </BLOCKQUOTE> Many thanks, will submit a version 2. Outstanding points:     - how to specify modules on the command line and in --modfile (local git checkouts, what are the popular schemes?)     - one script (which name) or two scripts? Think wiki and maintenance. <A HREF="http://wiki.x.org/wiki/Development/Documentation/ReleaseHOWTO">http://wiki.x.org/wiki/Development/Documentation/ReleaseHOWTO</A> <BLOCKQUOTE TYPE=CITE> <PRE> Rebuild Sudan - Board of Directors - <A HREF="http://www.rebuildsudan.org">http://www.rebuildsudan.org</A> Berkeley Foundation for Opportunities in Information Technology - Advisory Board - <A HREF="http://www.bfoit.org">http://www.bfoit.org</A> </PRE> </BLOCKQUOTE> </BODY> </HTML>