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

[Roger Leigh]

On Sun, Oct 18, 2009 at 09:01:56PM -0400, Andres Mejia wrote:
> On Sunday 18 October 2009 18:02:02 Roger Leigh wrote:
> > 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?
> 
> Well, STDOUT can be used, so long as STDOUT becomes unbuffered.

Or do an explict flush at the end of the loop so it it flushed each
time through:

  STDOUT->flush();

> > 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.
> 
> I would keep the progress indicator. There are various packages that are very 
> large (> 100MB), some of which I work on.

Sure, but it will make the log unreadable.  You don't have an
interactive progress bar for apt-get downloads in sbuild, so I don't
really see a compelling reason to do this for other cases either.

To check if a progress indicator is needed:
1) Has --nolog been specified?  If so, then display indicator.
2) Are we running in buildd mode?  If so, then don't display.
   if ($conf->get('SBUILD_MODE') ne 'buildd')
   ...

There may be better criteria for deciding this, but I think this is a
decent enough starting point to avoid messed up log files.  It's also
currently possible to log to $saved_stdout and avoid the logger, which
is another possibility and this could allow logging in all cases except
when in buildd mode.

> > 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.
> 
> I think what's best is that there's a per-package build log from sbuild, 
> regardless if sbuild drops support for processing multiple packages.

It will continue to do this.

To summarise the changes I'd like done:

1) Switch from STDERR to STDOUT
2) Explict flush of STDOUT at loop end
3) Display progress only if nolog is true and sbuild_mode is not buildd.
   Alternatively, log to the saved stdout stream
   $Sbuild::LogBase::saved_stdout.

For the last, you'll need to pass in $conf as for other utility
functions to get access to the configuration state.


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/20091019/002ade63/attachment.pgp>