[Buildd-tools-devel] schroot filesystem union support

[Roger Leigh]

On Thu, Mar 26, 2009 at 10:13:38PM +0100, Jan-Marek Glogowski wrote:
> Hi everybody
> 
> A few days ago (and a year ago) I sent a large patch for filesystem union
> support for schroot. It took me a while to split it up. Final diffstat is
> 
>  80 files changed, 2486 insertions(+), 739 deletions(-)
> 
> This patchset can be sepearated into the following blocks:
> 
> 01-07 Autotools cleanup and debianisation
> 08-14 Minor features (sessions lists and list validation) and
>       cleanups of schroot code

I have now committed patches 1-8.  I'm happy with all but a tiny
minority of the changes, so most changes are committed verbatim
or with minor whitespace/spelling/grammar cleanup.  Some of the
changes were already made by me, so were dropped (or just the
missing bits added).

These are my comments on my review so far.  If you feel any of my
understanding of the patches or what I have committed is
incorrect, please do follow up with an explanation and/or a
new patch against the current git if needed.


fs-union patch set review
-------------------------

[PATCH 01/22] Just link schroot-mount to boost filesystem lib

==> Committed with no changes


[PATCH 02/22] Add debug symbols package

Separate .gitignore changes

==> Committed .gitignore simplification

libsbuild symbols are not included (currently only static)
Depend on schroot, dchroot and dchroot-dsa with strict binary:Version
Recommends on libsbuild-dev is not needed
Fix up package description: remove hyphenation, s/from/for,
add dchroot-dsa and remove libsbuild
Priority should be extra

==> Committed main patch with minor corrections


[PATCH 03/22] Fix building libsbuild-doc package

Path fixups not needed
Maintainer mode not needed
Dependency changes needed
Build-dep changes needed

With recent changes on the master branch (--enable-doxygen)
the only required part is the build-dep on graphviz and
depenency fixes

==> Committed build-dep and dependency changes only


[PATCH 04/22] Run check before install when building packages

Should be safe with removal of sbuild-lock test.
Is run from install target, so fakeroot should be in effect.

==> Committed with no changes


[PATCH 05/22] Use 'make -C' and 'mkdir -p' in debian/rules

mkdir -p not necessary since debian dir always exists
make -C is OK
Some horizontal whitespace removal which is not necessary

==> Committed all $(MAKE) -C changes.


[PATCH 06/22] Adapt boostrap from libtoolize suggestions.

ACLOCAL_AMFLAGS OK.
AC_CONFIG_MACRO_DIR not present, but does the same job as ACLOCAL_AMFLAGS,
is undocumented and should probably not be used (deprecated?).
autom4te.cache removal is unnecessary?  Seems fast enough without, so
should be OK.

==> Committed all except AC_CONFIG_MACRO_DIR

==> Committed additional change to use autoreconf


[PATCH 07/22] Use $(mkinstalldirs) to create directories

==> Committed with no changes


[PATCH 08/22] Add another error detail and add log info functions.

Check spelling and grammar of doxygen comments.

==> Commotted error changes with minor whitespace/indentation cleanup

Log function log_exception() makes no time/space saving over
existing implementation in switch statement.  This adds an extra
layer of complexity and indirection, without any additional benefit.
There is a space and size saving in reason logging which is duplicated
in all functions.  Therefore separate reason logging only into a
separate set of functions.

log_exception_info and log_ctty_exception_info are valid additions
to the API.  However, they were not added previously because
exceptions are by definition exceptional events which should
be at a minimum logged as a warning.  Reason information is
logged as an informational message.

==> Committed my own unification of reason logging
==> log_exception_info and log_ctty_exception_info not committed


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
Url : http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20090328/e11c59a7/attachment.pgp