This is the mail archive of the
cygwin-apps@cygwin.com
mailing list for the Cygwin project.
Re: [PATCH] Postinstall script ordering in setup
- From: Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
- To: Robert Collins <rbcollins at cygwin dot com>
- Cc: cygwin-apps at cygwin dot com
- Date: Tue, 4 Mar 2003 09:58:01 -0500 (EST)
- Subject: Re: [PATCH] Postinstall script ordering in setup
- Reply-to: cygwin-apps at cygwin dot com
Rob,
Thanks for the review. Comments below...
On 4 Mar 2003, Robert Collins wrote:
> On Tue, 2003-03-04 at 15:36, Igor Pechtchanski wrote:
> > Hi,
> >
> > This patch adds a dependence tracking mechanism to postinstall scripts.
> > The idea is essentially the one described in
> > <http://cygwin.com/ml/cygwin-apps/2003-03/msg00022.html>. This patch is
> > very preliminary, and not tested much beyond its ability to sort scripts
> > correctly (compiles, though). This should provide a foundation for
> > further refinement, however. Please comment.
>
> Commenting from the changelog for now...
>
> I'll do a more indepth review when the obvious issues below are sorted
> out. BTW: I agree with this concept as a short term solution.
>
> > Igor
> > ==============================================================================
> > ChangeLog:
> > 2003-03-03 Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
> >
> > * postinstall.cc (RunFindVisitor::executeAll): New
> > member function that propagates script dependences,
> > topologically sorts the script list, and then executes
> > the scripts.
>
> This does too much. When a method does more than one thing... split it
> up.
This does it by calling 3 functions. It's a 7-line function. :-D
I guess my eagerness to make the changelog detailed enough got the best of
me.
> > (RunFindVisitor::visitFile): Add file to list instead of
> > running it.
> > (RunFindVisitor::files): New member variable.
> > (processFile): New static helper function that extracts
> > dependences from each script file.
>
> I suspect at this point we may want a class to handle this - to extract,
> remember and sort dependencies.
There is a class that contains the dependences (FileDesc). I could put
this function in that class (as static, though, so doesn't change much).
> > (do_postinstall): Add executeAll call.
> > (FileDesc): New helper class.
> > (sharpbang): New static helper function.
>
> This should be in a class, IMO.
> Rob
I've forgotten a lot of C++ in the recent years. In Java, I would have
made this a private method. How expensive are non-virtual private
functions in C++? If same as regular functions, I'll put it in the class
(along with processFile).
Igor
--
http://cs.nyu.edu/~pechtcha/
|\ _,,,---,,_ pechtcha at cs dot nyu dot edu
ZZZzz /,`.-'`' -. ;-;;,_ igor at watson dot ibm dot com
|,4- ) )-,_. ,\ ( `'-' Igor Pechtchanski
'---''(_/--' `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-. Meow!
Oh, boy, virtual memory! Now I'm gonna make myself a really *big* RAMdisk!
-- /usr/games/fortune