<!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.&nbsp; Sometimes you indent by 4, sometimes by 3, sometimes you use tabs for 2-indents, ... it would be nice if that were all consistent.&nbsp; 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:
&gt; The srcipt runs 'make dist' to create the tarball. We don't have to prompt
&gt; 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:<BR>
- I had to draw the line between 'development/test' and 'releasing'<BR>
&nbsp;&nbsp;&nbsp; - The prereq for releasing is a fully built and tested module with version bump in the remote<BR>
&nbsp;&nbsp;&nbsp; - The person releasing may not be the developer.<BR>
&nbsp;&nbsp;&nbsp; - One may be asked to release a dozen apps by a release manager who got confirmation from<BR>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; the developers that they are ready to release.<BR>
- It is significantly longer, particularly xserver and modules with doc<BR>
- It would make --dry-run virtually unusable<BR>
- 'make dist' only requires running autoconf, not compiling<BR>
- You may to release from a branch where not all the build dependencies are met. The module<BR>
would configure but not build. But you know it works because you or someone else has tested it.<BR>
<BR>
There are many releasing scenarios other than a module maintainer releasing a single module.<BR>
<BR>
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>

&gt; Interface
&gt; ---------
&gt; Usage: publish.sh [options] section[/module]...
&gt; 
&gt; Section:
&gt; app|data|doc|driver|font|lib|mesa|proto|util|xcb|
&gt; 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 -&lt;version&gt; 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?<BR>
An entry such as lib/private_libX11 will work. I just need &quot;lib&quot; to get the section and there should be<BR>
either no subdir or only one subdir. I am basically following the module url:<BR>
<BLOCKQUOTE>
    <A HREF="git://anongit.freedesktop.org/xorg/lib/libX11">git://anongit.freedesktop.org/xorg/</A> ==&gt; <A HREF="git://anongit.freedesktop.org/xorg/lib/libX11">lib/libX11</A><BR>
    or <A HREF="git://anongit.freedesktop.org/xorg/lib/libX11">git://anongit.freedesktop.org/xorg/</A> ==&gt; xserver<BR>
</BLOCKQUOTE>
What would be the structure of your &quot;local git checkouts&quot;?<BR>
Also keep in&nbsp; mind the section[/module] is a format used by build.sh and it follows the module url
<BLOCKQUOTE TYPE=CITE>
<PRE>

&gt; ...
&gt; +check_local_changes() {
&gt; +    git diff --quiet HEAD &gt; /dev/null 2&gt;&amp;1
&gt; +    if [ $? -ne 0 ]; then
&gt; +        echo &quot;&quot;
&gt; +        echo &quot;Uncommitted changes found. Did you forget to commit? Aborting.&quot;
&gt; +        echo &quot;&quot;
&gt; +        echo &quot;You can perform a 'git stash' to save your local changes and&quot;
&gt; +        echo &quot;a 'git stash apply' to recover them after the tarball release.&quot;
&gt; +        echo &quot;Make sure to rebuild and run 'make distcheck' again.&quot;
&gt; +        echo &quot;&quot;
&gt; +        echo &quot;Alternatively, you can clone the module in another directory&quot;
&gt; +        echo &quot;and run ./configure. No need to build if testing was finished.&quot;
&gt; +        echo &quot;&quot;
&gt; +        return 1
&gt; +    fi
&gt; +    return 0
&gt; +}

There are whitespace inconsistencies above ^^^

&gt; ...
&gt; +#------------------------------------------------------------------------------
&gt; +#                        Function: generate_announce
&gt; +#------------------------------------------------------------------------------
&gt; +#
&gt; +generate_announce()
&gt; +{
&gt; +    cat &lt;&lt;RELEASE
&gt; +Subject: [ANNOUNCE] $pkg_name $pkg_version
&gt; +To: $list_to
&gt; +CC: $list_cc
&gt; +
&gt; +`git log --no-merges &quot;$tag_range&quot; | git shortlog`
&gt; +
&gt; +git tag: $tar_name
&gt; +
&gt; +<A HREF="http://">http://</A>$host_current/$section_path/$tarbz2
&gt; +MD5:  `$MD5SUM $tarbz2`
&gt; +SHA1: `$SHA1SUM $tarbz2`
&gt; +SHA256: `$SHA256SUM $tarbz2`
&gt; +
&gt; +<A HREF="http://">http://</A>$host_current/$section_path/$targz
&gt; +MD5:  `$MD5SUM $targz`
&gt; +SHA1: `$SHA1SUM $targz`
&gt; +SHA256: `$SHA256SUM $targz`
&gt; +
&gt; +RELEASE
&gt; +}

Why are we duplicating code between release.sh and publish.sh?&nbsp; 
</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&nbsp; tests. The odds of a change done in release.sh and be tested from publish.sh are next to nil.<BR>
Returning information, sharing variables, error checking, user messages style and consistency are all more difficult.<BR>
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.<BR>
<BR>
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 &quot;intelligent&quot; aspects but for the current module.
&nbsp; I would like to either see both of these exist together or update publish.sh to support &quot;.&quot;
</PRE>
</BLOCKQUOTE>
The thought had crossed my mind, given the existing working habits. I was waiting for comments on that.<BR>
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>
<BR>
<BLOCKQUOTE TYPE=CITE>
<PRE>

&gt; ...
&gt; +            # skip comment lines
&gt; +            echo &quot;$line&quot; | grep &quot;^#&quot; &gt; /dev/null
&gt; +            if [ $? -eq 0 ]; then
&gt; +                continue
&gt; +            fi

A tad cleaner:
if echo &quot;$line&quot; | grep -q &quot;^#&quot; ; then
    continue;
fi
</PRE>
</BLOCKQUOTE>
Ok, copied code from build.sh.
<BLOCKQUOTE TYPE=CITE>
<PRE>

&gt; +#------------------------------------------------------------------------------
&gt; +#                        Function: print_epilog
&gt; +#------------------------------------------------------------------------------
&gt; +#
&gt; +print_epilog() {
&gt; +
&gt; +epilog=&quot;========  Successful Completion&quot;
&gt; +if [ x&quot;$NO_QUIT&quot; != x ]; then
&gt; +    if [ x&quot;$failed_modules&quot; != x ]; then
&gt; +        epilog=&quot;========  Partial Completion&quot;
&gt; +    fi
&gt; +elif [ x&quot;$failed_modules&quot; != x ]; then
&gt; +        epilog=&quot;========  Stopped on Error&quot;
&gt; +fi
&gt; +
&gt; +echo &quot;&quot;
&gt; +echo &quot;$epilog `date`&quot;
&gt; +
&gt; +# Report about modules that failed for one reason or another
&gt; +if [ x&quot;$failed_modules&quot; != x ]; then
&gt; +    echo &quot;        List of failed modules:&quot;
&gt; +    for mod in $failed_modules; do
&gt; +        echo &quot;        $mod&quot;
&gt; +    done
&gt; +    echo &quot;========&quot;
&gt; +    echo &quot;&quot;
&gt; +fi
&gt; +
&gt; +}

I'm confused why the contents of the above function are not indented...
</PRE>
</BLOCKQUOTE>
Sore eyeballs.
<BLOCKQUOTE TYPE=CITE>
<PRE>

&gt; +#------------------------------------------------------------------------------
&gt; +#                        Function: process_modules
&gt; +#------------------------------------------------------------------------------
&gt; +#
&gt; +# Loop through each module to publish
&gt; +# Exit on error if --no-quit no specified

^^^ &quot;if --no-quit was not specified&quot;

&gt; +#
&gt; +process_modules() {
&gt; +    for MODULE_RPATH in ${INPUT_MODULES}; do
&gt; +        process_module
&gt; +        if [ $? -ne 0 ]; then

again, I would do:

if ! process_module ; then
...
fi

&gt; +#------------------------------------------------------------------------------
&gt; +#                        Function: process_module
&gt; +#------------------------------------------------------------------------------
&gt; +# Code 'return 0' on success to process the next module
&gt; +# Code 'return 1' on error to process next module if invoked with --no-quit 
&gt; +#
&gt; +process_module() {
&gt; +
&gt; +    # Skip already processed modules when resuming from a previous failure
&gt; +    if [ x&quot;$RESUME&quot; != x ]; then
&gt; +        if [ x&quot;$RESUME&quot; = x&quot;$MODULE_RPATH&quot; ]; then
&gt; +            # The current module is the one that failed last time
&gt; +            echo &quot;Info: resuming from $RESUME module&quot;
&gt; +            unset RESUME
&gt; +        else
&gt; +            # The current module has already been processed successfully last time
&gt; +            echo &quot;Info: skipping $MODULE_RPATH in autoresume mode.&quot;
&gt; +            return 0
&gt; +        fi
&gt; +    fi
&gt; +
&gt; +    echo &quot;&quot;
&gt; +    echo &quot;========  Processing \&quot;$MODULE_RPATH\&quot;&quot;
&gt; +
&gt; +    top_src=`pwd`
&gt; +    if [ ! -d $MODULE_RPATH ] ; then
&gt; +        echo &quot;Error: $MODULE_RPATH cannot be found under $top_src.&quot;
&gt; +        failed_modules=&quot;$failed_modules $MODULE_RPATH&quot;
&gt; +        return 1

^^^ whitespace

&gt; ...
&gt; +    # Mailing lists to be CC according to the project (xorg|dri|xkb)
&gt; +    list_xorg_user=&quot;<A HREF="mailto:xorg@lists.freedesktop.org">xorg@lists.freedesktop.org</A>&quot;
&gt; +    list_dri_devel=&quot;<A HREF="mailto:dri-devel@lists.sourceforge.net">dri-devel@lists.sourceforge.net</A>&quot;
&gt; +    list_xkb=&quot;<A HREF="mailto:xkb@listserv.bat.ru">xkb@listserv.bat.ru</A>&quot;

Add in the xcb list?
</PRE>
</BLOCKQUOTE>
Sure, it wasn't in release.sh
<BLOCKQUOTE TYPE=CITE>
<PRE>

...

&gt; +    # The jhbuild moduleset to update with relase info
&gt; +    --moduleset)
&gt; +        check_option_args $1 $2
&gt; +        shift
&gt; +        JH_MODULESET=$1
&gt; +        ;;

^^^ Whitespace inconsistency


&gt; +    --*)
&gt; +        echo &quot;&quot;
&gt; +        echo &quot;Error: unknown option: $1&quot;
&gt; +        echo &quot;&quot;
&gt; +        usage
&gt; +        exit 1

^^^ Whitespace inconsistency

&gt; +        ;;
&gt; +    -*)
&gt; +        echo &quot;&quot;
&gt; +        echo &quot;Error: unknown option: $1&quot;
&gt; +        echo &quot;&quot;
&gt; +        usage
&gt; +        exit 1
&gt; +        ;;

^^^ Whitespace inconsistency

&gt; +    *)
&gt; +        if [ x&quot;${MODFILE}&quot; != x ]; then
&gt; +            echo &quot;&quot;
&gt; +            echo &quot;Error: specifying both modules and --modfile is not permitted&quot;
&gt; +            echo &quot;&quot;
&gt; +            usage
&gt; +            exit 1

^^^ Whitespace inconsistency


---
Jeremy Huddleston

</PRE>
</BLOCKQUOTE>
Many thanks, will submit a version 2.<BR>
<BR>
Outstanding points:<BR>
&nbsp;&nbsp;&nbsp; - how to specify modules on the command line and in --modfile (local git checkouts, what are the popular schemes?)<BR>
&nbsp;&nbsp;&nbsp; - one script (which name) or two scripts? Think wiki and maintenance. <BR>
<A HREF="http://wiki.x.org/wiki/Development/Documentation/ReleaseHOWTO">http://wiki.x.org/wiki/Development/Documentation/ReleaseHOWTO</A><BR>
<BR>
<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>
<BR>
</BODY>
</HTML>