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

[Andres Mejia]

On Monday 19 October 2009 15:46:19 Roger Leigh wrote:
> 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();

Ok.

> > > 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.

Ok. I'll see about taking care of that so that they don't show up in any log 
files.

> > > 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.

For 3, I think I'll use the saved stdout stream.

> 
> Regards,
> Roger
> 

-- 
Regards,
Andres