Re: Setup postinstall logging (was Re: Pending setup patches(issue 2))

[Robert Collins <rbcollins at cygwin dot com>]

On Sat, 2003-03-15 at 04:32, Igor Pechtchanski wrote:


> > Howabout that?
> > Rob
> 
> Ok, here's the next iteration of this patch.  It still pops a console with
> nothing in it when running postinstall scripts -- I'm sure there's a way
> to remove it, but can't find it at the moment.

Don't worry. What would be good would be to make it start minimized.

>   It does, however,
> correctly redirect the output of postinstall scripts into the LOG_BABBLE
> file.  Most of the patch is OS-independent, but the output redirection
> mechanism has been tested on Win95 by Brian Keener (see
> <http://cygwin.com/ml/cygwin-apps/2003-03/msg00364.html>) and hasn't been
> changed in this patch.
> 	Igor
> P.S. Note that this patch conflicts slightly with my other patch
> (postinstall script ordering).  Nothing a human can't fix, but both at
> once will not apply cleanly OOTB.  Whichever one of them goes in first,
> I'll regenerate and resubmit the other one.

Ok, thank you.

> ==============================================================================
> ChangeLog:
> 2003-03-13  Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
> 
> 	* script.cc (run): Add lname parameter.
> 	Redirect output of subprocess to file, creating the
> 	path if necessary.
> 	(run_script): Add optional to_log boolean parameter.
> 	If to_log, redirect output to temporary file.
> 	(openOutputLog): New helper function.

This should return void and throw an exception on failure...
OR
use a class and set a status member.

> 	(closeOutputLog): New helper function.
> 	(removeOutputLog): New helper function.
> 	* script.h (run_script): Add optional to_log parameter.
> 	* log.h (log_file): New function.
> 	* log.cc (log_file): New Function
> 	(BUFLEN): New #define.
> 	* postinstall.cc (RunFindVisitor::visitFile): Pass
> 	filename to run_script().

The define BUFLEN should be a static const member of the class.

A general nit (just some search n replace for you) - I'd like new
methods and variables to make sense at first read. So 'lname' isn't
great. logFilename would be good. Likewise log_file isn't clear that it
imports a text file into the setup log. Oh, and I loath the MS idiom of
including the type in the variable name - bInheritHandles being a case
in point.

Other than those sugar things, the code looks great. Please update the
patch and then I'll rubber stamp it.

Rob

-- 
GPG key available at: <http://users.bigpond.net.au/robertc/keys.txt>.

Attachment: signature.asc
Description: This is a digitally signed message part