[PATCH modular] Publish.sh: batch release and autotagging of modules
Gaetan Nadon
memsize at videotron.ca
Fri Oct 7 12:20:45 PDT 2011
On Fri, 2011-10-07 at 10:46 -0700, Jeremy Huddleston wrote:
> 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.
Sure, just forgot about it.
>
>
> 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?
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.
>
> > 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.
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:
git://anongit.freedesktop.org/xorg/ ==> lib/libX11
or git://anongit.freedesktop.org/xorg/ ==> xserver
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
>
> > ...
> > +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
> > +
> > +http://$host_current/$section_path/$tarbz2
> > +MD5: `$MD5SUM $tarbz2`
> > +SHA1: `$SHA1SUM $tarbz2`
> > +SHA256: `$SHA256SUM $tarbz2`
> > +
> > +http://$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?
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.
> Can we instead have publish.sh be a smart wrapper around release.sh.
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).
> 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 "."
The thought had crossed my mind, given the existing working habits. I
was waiting for comments on that.
I don't have any objection.
> as an argument (which would possibly fall out naturally from my suggestion above about taking paths)
>
> > ...
> > + # 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
Ok, copied code from build.sh.
>
> > +#------------------------------------------------------------------------------
> > +# 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...
Sore eyeballs.
>
> > +#------------------------------------------------------------------------------
> > +# 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="xorg at lists.freedesktop.org"
> > + list_dri_devel="dri-devel at lists.sourceforge.net"
> > + list_xkb="xkb at listserv.bat.ru"
>
> Add in the xcb list?
Sure, it wasn't in release.sh
>
> ...
>
> > + # 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
>
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.
http://wiki.x.org/wiki/Development/Documentation/ReleaseHOWTO
> Rebuild Sudan
> - Board of Directors
> - http://www.rebuildsudan.org
>
> Berkeley Foundation for Opportunities in Information Technology
> - Advisory Board
> - http://www.bfoit.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111007/48cdbd80/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111007/48cdbd80/attachment.pgp>
More information about the xorg-devel
mailing list