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.
signature.asc
Description: Digital signature
