[PATCH] Postinstall script ordering in setup - take 4

[Igor Pechtchanski <pechtcha at cs dot nyu dot edu>]

On 15 Mar 2003, Robert Collins wrote:

> On Sat, 2003-03-15 at 05:06, Igor Pechtchanski wrote:
>
> > Ok.  Here's that alternative, more reasonably implemented, and *tested*
> > this time :-) (at least as far as reading files, sorting them, and
> > executing scripts is concerned).  The dependence-extraction mechanism
> > still needs to be verified, though.  Comments and suggestions for
> > improvement are welcome.
> >       Igor
> > ==============================================================================
> > ChangeLog:
> > 2003-03-03  Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
>
> I'd like you do keep the class declaration free of code. It's fine to
> have it in the .cc file if it's really trivial an private, but even then
> can you put the implementations outside the class declaration. (With one
> exception: one-liners.

Done.

> >       * postinstall.cc (RunFindVisitor::executeAll): New
> >       member function that propagates script dependencies,
> >       topologically sorts the script list, and then executes
> >       the scripts (via other calls).
> >       (RunFindVisitor::visitFile): Change to add script to
> >       list instead of running it immediately.
> >       (RunFindVisitor::files): New member variable.
> >       (RunFindVisitor::checkAndLogMissingDependencies): New
> >       member function.
> >       (FileDesc): New class that extracts and stores
> >       dependencies from a script file.
>
> The logic in here looks almost identical to that in
> packageversion::set_requirements. I don't want to add near-duplicate
> code if we can avoid it. Perhaps extracting the core for both to a
> convenience class?

Sure.  Not sure how to do it, though - the logic in
packageversion::set_requirements is a bit confusing (I think it does much
more than my code).

> Your operator > and < appear to have a problem.
>
> foo: bar
> gam: bar
>
> foo > gam    = false
> gam < foo    = false
> gam == foo   = false.
>
> I'd expect stl associative containers to choke on that.
>
> I suggest you do
> {
>   /* we are greater than any dependency of ours */
>   if (_dependencies.find(&f) != _dependencies.end())
>     return true;
>   return _path > f._path;
> }

See the note on partial order in
<http://cygwin.com/ml/cygwin-apps/2003-03/msg00440.html>.  FWIW, this
works fine if there are no dependences (i.e., the order is always
undefined).

> this
> +      const char *err = strerror (errno);
> +      if (!err)
> +       err = "(unknown error)";
>
> could be usefull extracted so that we can do
>   String const errorString ( StringError(errno));
> and always get back a string.

Agreed.  I put it in as a static function for now - where should it go?

> >       (DEPEND_STR): New #define.
>
> BUFLEN should be a static const member of the class.

Do you mean DEPEND_STR or BUFLEN?  In either case, BUFLEN is now inside
the FileDesc namespace (as an enum), and DEPEND_STR is a static const
member.

> >       (do_postinstall): Add executeAll() call.
>
> Thanks again for all the work you've put into this.
> [snip]
> Rob

New iteration attached.
	Igor
==============================================================================
ChangeLog:
2003-03-15  Igor Pechtchanski <pechtcha at cs dot nyu dot edu>

	* postinstall.cc (RunFindVisitor::executeAll): New
	member function that propagates script dependencies,
	topologically sorts the script list, and then executes
	the scripts (via other calls).
	(RunFindVisitor::visitFile): Change to add script to
	list instead of running it immediately.
	(RunFindVisitor::files): New member variable.
	(RunFindVisitor::checkAndLogMissingDependencies): New
	member function.
	(FileDesc): New class that extracts and stores
	dependencies from a script file.
	(StringError): New static helper function.
	(do_postinstall): Add executeAll() call.

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

Attachment: setup-postinstall-ordering.patch
Description: Text document