[buildd-tools-devel] Some schroot (union) fixes

Sat Aug 1 11:36:03 UTC 2009

On Thu, Jul 30, 2009 at 09:10:49PM +0200, Jan-Marek Glogowski wrote:
> Hi
> 
> > The major recent change has been the merging of the union filesystem
> > support from Jan-Marek Glogowski.  I'd like to release a development
> > version (1.3.0) for further testing once I have confirmation that what
> > was merged is working.  Could anyone verify this?  Jan?
> 
> The following patches contain my patches on the top of current master.
> At least the patches 07-11 are needed to build a package with working
> union and sbuild support.

I've applied most of the patches now.  Could you please check if this
is working correctly for you.

I've detailed which were applied.  Some were not applied, and I've given
the reasons why.  I'm particularly interested in some more information
about patch 3, and if patch 6 is still OK.  Some were not applied because
they were superceded by changes during the facet migration.

[PATCH 01/11] [test] Use sbuild::getcwd to get abs_testdata_dir

Applied.

[PATCH 02/11] [chroot_loopback] Reuse loopback device

Applied.  I've double-checked that the order of loopback mount
unmounting still results in correct and race-free device freeing.
This appears to be the case.  However, this is really functionality
that should be present in mount(8)--there's no reason this shouldn't
be automatic.

[PATCH 03/11] [sbuild_session] Don't rollback non-run scripts

Not applied.  I still have significant issues with this patch, as I've
mentioned previously:

* Why do you need to use a child process to run the scripts when this
  can all be done directly?  We have:

  main process
  └── run_parts child
      ├── script1
      ├── script2
      └── script3

  where this is entirely sufficient:

  main process
  ├── script1
  ├── script2
  └── script3

  I really don't understand what this extra complexity gains us.

* Why is it important to roll back the scripts from a defined point?

  Since the setup scripts may fail *during setup-stop* at any point,
  we don't know the failure point from one schroot invocation to the
  next.  As a result, it is a design choice that all setup scripts
  must be idempotent.  That is, they must be capable of repeated
  invocation during the setup-stop stage, and function correctly all
  the time.

  Again, I don't understand what this gains us.

I'm not opposed to the idea of the patch, but I don't like the current
implementation.  There's no reason why the class can't store a vector
containing each command and its exit status and allow the user to
query that and get the name of the failing command (or of all
commands).

Also, bear in mind run_parts is an implementation of the run-parts(8)
interface.  I'm happy to add additional functionality to add more of
the real run-parts(8) functionality, but if it deviates from what
run-parts offers, I want a really good reason (and use case) for it.

[PATCH 04/11] [test] Add set_run_setup_scripts tests

Applied.

[PATCH 05/11] [test] Unify environment tests

Applied, but with some changes due to it being broken (possibly due to
the rebasing).

[PATCH 06/11] [union] Cleanup union handling in scripts

Applied with a few changes (quoting issues, consistent use of
variables to mean the same thing throughout the script).

[PATCH 07/11] [Makefile.in] Use $(mkinstalldirs)

Applied.

[PATCH 08/11] [chroot_union] Read union type before source keys

Not applied.  This no longer works with the switch to facets.

This problem is worked around by calling
chroot_facet_source::set_keyfile in chroot_facet_union::set_keyfile.
It doesn't matter if the source facet is set twice, and this should
guarantee the source-* options are always set correctly.

[PATCH 09/11] [sbuild_session] Don't clone active sessions

Not applied.  SESSION_CREATE is now *only* set for unactive sessions,
and we check for this in the testsuite, so the extra check is no
longer needed.

[PATCH 10/11] Some cleanup for directory and union chroots

Applied, but to facet code rather than old code.  The directory
cleanup was already done with the facet migration.

[PATCH 11/11] [sbuild_session] Set session_id for session object

Not applied.  While this was a valid fix for the problem, it's better
to never call set_session_id ever, and always use the chroot
session_id (which is set from this value).  This also avoids a bug
whereby running using multiple chroots resulted in all the chroots
getting the same session ID.

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/20090801/0c430a07/attachment.pgp>