[buildd-tools-devel] Bug#546555: Bug#546555: Bug#546555: Updated patches for support of remote source (.dsc) files

[Roger Leigh]

On Sat, Oct 17, 2009 at 02:12:58PM -0400, Andres Mejia wrote:
> On Saturday 17 October 2009 01:24:24 Andres Mejia wrote:
> > Please ignore the two patches I submitted earlier. These new patches will
> >  give a better progress output than what's currently implemented for sbuild
> >  and will also avoid the issue that backspaces can't be done in the log
> >  files, which is what done by LWP::UserAgent progres() subroutine.
> > 
> > The first patch will make using LWP::UserAgent optional via the
> > Module::Load::Conditional module. It also gives a better output for the
> > progress indicator.
> > 
> > The second patch ensures all output from the Utility subroutines show up in
> > the log files.
> 
> Could these patches be applied instead. The first patch is the same as the 
> previous patch from the previous message except that this one keeps the 
> implementation to print a total size of content that's downloaded. It also 
> allows it to be printed in MB, KB, and B. It's also more careful not to bring 
> any unnecessary changes like changes to comments and formatting changes.

Applied.  Once thing I'm not sure I like (not in the patch here, but from
previously) is the use of STDERR for the progress indicator.  This will
cause the build log (rather than the package build log) to fill up with
junk, and result in spurious mails about sbuild problems being sent).

Why can't STDOUT be used in place?

Do we actually /need/ a progress indicator?  Can't we mirror the APT
behaviour here?  Note: you could check if the stream is a TTY (isatty)
or that we're running interactively before enabling the progress meter
to avoid filling the logfile with junk.  It should probably be disabled
in buildd mode or if not run interactively.

> The second patch is the same patch I supplied in the previous message.

The current design of the sbuild logging is that there are two logfiles:

1) the main log which logs everything except package build stuff
   (per-invocation)
2) the package build log (per-package)

STDOUT and STDERR are already redirected--to the main log.  See line
87 in open_log() in LogBase.pm.  Your patch would break this, and
it is also not correct since it doesn't revert the stream state on
package log close as the close_log() code does.

Now, I'm quite willing to revisit this design.  It might well make
sense to divert STDOUT and STDERR to the build log for the duration of
the build (while the package log is open).  However, it needs to use
the same sort of logic open_log and close_log do already.

The reason we stopped doing this in the first place was to allow for
multiple parallel builds, in which case each package needs its own
separate streams; hence the reason why we stopped using global streams.

I think the best policy here would be for those functions to do what
the chroot code does.  See STREAMIN/STREAMOUT/STREAMERR in Chroot.pm
for how we pass open file handles around.  This means the code is
completely agnostic as to which streams it's using, and hence is
rather more flexible.

Current consensus seems to be that sbuild should drop the multiple
package stuff in favour of buildd handling this.  This would allow
us to drop the two logs in favour of a single unified log, which
would make some of the logging code simpler.


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: 198 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091018/4c473649/attachment.pgp>