On Monday 19 October 2009 17:00:32 Andres Mejia wrote:
> 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
>
Here's a new patch that addresses these concerns.
--
Regards,
Andres
From 5f7a528a266a9993888db58293ae85f2c347a021 Mon Sep 17 00:00:00 2001
From: Andres Mejia <[email protected]>
Date: Tue, 20 Oct 2009 17:55:30 -0400
Subject: [PATCH] Print to the saved stdout stream from Sbuild::Logbase instead of STDERR.
This will ensure output from download() subroutine don't end up in any build
logs.
---
lib/Sbuild/Utility.pm | 36 ++++++++++++++++++++----------------
1 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/lib/Sbuild/Utility.pm b/lib/Sbuild/Utility.pm
index 7b418c3..6ae6159 100644
--- a/lib/Sbuild/Utility.pm
+++ b/lib/Sbuild/Utility.pm
@@ -173,6 +173,9 @@ sub download {
# The parameters will be any URL and a location to save the file to.
my($url, $file) = @_;
+ # Print output from this subroutine to saved stdout stream of sbuild.
+ my $stdout = $Sbuild::LogBase::saved_stdout;
+
# If $url is a readable plain file on the local system, just return the
# $url.
return $url if (-f $url && -r $url);
@@ -199,7 +202,7 @@ sub download {
}
# Download the file.
- print STDOUT "Downloading $url to $file.\n";
+ print $stdout "Downloading $url to $file.\n";
my $expected_length; # Total size we expect of content
my $bytes_received = 0; # Size of content as it is received
my $percent; # The percentage downloaded
@@ -243,25 +246,25 @@ sub download {
if (($tick == 250) or ($percent == 100)) {
if ($tick == 1) {
# In case we reach 100% from tick 1.
- printf STDERR "%8s", sprintf("%d",
+ printf $stdout "%8s", sprintf("%d",
$bytes_received / 1024) . "KB";
- print STDERR " [.";
+ print $stdout " [.";
}
while ($tick != 250) {
# In case we reach 100% before reaching 250 ticks
- print STDERR "." if ($tick % 5 == 0);
+ print $stdout "." if ($tick % 5 == 0);
$tick++;
}
- print STDERR ".]";
- printf STDERR "%5s", "$percent%";
- printf STDERR "%12s", "$speed\n";
+ print $stdout ".]";
+ printf $stdout "%5s", "$percent%";
+ printf $stdout "%12s", "$speed\n";
$tick = 0;
} elsif ($tick == 1) {
- printf STDERR "%8s", sprintf("%d",
+ printf $stdout "%8s", sprintf("%d",
$bytes_received / 1024) . "KB";
- print STDERR " [.";
+ print $stdout " [.";
} elsif ($tick % 5 == 0) {
- print STDERR ".";
+ print $stdout ".";
}
}
# Write the contents of the download to our specified file
@@ -269,9 +272,10 @@ sub download {
print $fh $chunk; # Print content to file
} else {
# Print message upon failure during download
- print STDERR "\n" . $response->status_line . "\n";
+ print $stdout "\n" . $response->status_line . "\n";
return 0;
}
+ $stdout->flush();
}
); # End of our content callback subroutine
close $fh; # Close the destination file
@@ -284,15 +288,15 @@ sub download {
# Print out amount of content received before returning the path of the
# file.
- print STDOUT "Download of $url sucessful.\n";
- print STDOUT "Size of content downloaded: ";
+ print $stdout "Download of $url sucessful.\n";
+ print $stdout "Size of content downloaded: ";
if ($bytes_received >= 1024 * 1024) {
- print STDOUT sprintf("%.4g MB",
+ print $stdout sprintf("%.4g MB",
$bytes_received / (1024.0 * 1024)) . "\n";
} elsif ($bytes_received >= 1024) {
- print STDOUT sprintf("%.4g KB", $bytes_received / 1024.0) . "\n";
+ print $stdout sprintf("%.4g KB", $bytes_received / 1024.0) . "\n";
} else {
- print STDOUT sprintf("%.4g B", $bytes_received) . "\n";
+ print $stdout sprintf("%.4g B", $bytes_received) . "\n";
}
return $file;
--
1.6.5
