Re: [PATCH] Run postinstall scripts in a thread with progress bars - take 3

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

On 25 Mar 2003, Robert Collins wrote:

> On Tue, 2003-03-25 at 09:48, Igor Pechtchanski wrote:
> > On 21 Mar 2003, Robert Collins wrote:
> > Rob,
> >
> > I wanted (and still want) to keep the two bar progress on this.  The
> > second bar would show progress through packages, and the first - progress
> > through the scripts *in the current package*.
>
> Ok. I don't really see the end-user benefit, but don't object either.
>
> > > Alteratively/also you could extract the script running and screen
> > > updating code to a new method.
> > > Rob
> >
> > Done.  I've also done some restructuring of the Script class and enabled
> > the "#if 0"'d code from the previous patch, now that the progress bars
> > exist.
> >
> > I've attached the next iteration.  Should apply cleanly to HEAD.
> >       Igor
> > ==============================================================================
> > ChangeLog:
> > 2003-03-24  Igor Pechtchanski <pechtcha at cs dot nyu dot edu>
> >
> >       * threebar.h (WM_APP_START_POSTINSTALL): New message.
> >       (WM_APP_POSTINSTALL_THREAD_COMPLETE): New message.
> >       * threebar.cc (ThreeBarProgressPage::OnMessageApp):
> >       Add handling for WM_APP_START_POSTINSTALL and
> >       WM_APP_POSTINSTALL_THREAD_COMPLETE.
> >       * install.cc (do_install_thread): Set next_dialog to
> >       IDD_S_POSTINSTALL.
> >       * desktop.cc (DesktopSetupPage::OnFinish): Move the
> >       do_postinstall call to ThreeBarProgressPage::OnMessageApp.
> >       * script.h (Script::fullName): New member function.
> >       (Script::run): New member function.
> >       * script.cc (Script::fullName): Implement.
> >       (Script::run): Implement.
> >       (run): Enable "#if 0"'d code.
> >       * postinstall.cc (Progress): New extern variable.
> >       (RunFindVisitor::visitFile): Add script to vector
> >       instead of running.
> >       (RunFindVisitor::_scripts): New member variable.
> >       (run_package_scripts): New static function.
> >       (do_postinstall_thread): Rename do_postinstall to.  Add
> >       Progress bar and text setting.  Add package count.
> >       (do_postinstall_reflector): New static function.
> >       (do_postinstall): Rename to do_postinstall_thread.
> >       Create a thread instead.
>
> etc_postinstall should be:
> 1) a static public const member of Script.
> 2) called ETCPostinstall. (Leading capital for statics).

Agreed.  Will do.

> 3) reused in RunFindVisitor.

No.  In fact, the code currently in RunFindVisitor is broken and will not
work if there are subdirectories under /etc/postinstall.  What I would
like to do (in a separate patch) is change FindVisitor so that basePath is
the POSIX path, rather than the Windows one.  We can then simply
concatenate the filename to it and use that.

> run_package_scripts cries our for a helper class IMO.
>
> i.e. ScriptRunner with
> a) constructor
> b) destructor
> c) run(std::vector<Script> const &) method.
> d) operator () (Script const &aScript) method.

I don't see the benefit of run(); it'll be subsumed by operator(), IMO.
Otherwise, I'll give it a shot.

> this:
> for (std::vector<Script>::iterator script = scripts.begin();
> +       script != scripts.end();
> +       ++script)
>
> then becomes
> *this = for_each (scripts.begin(), scripts.end(), *this);

We could probably just lose the return value...

> This is looking very good, and nearly ready to apply.
> Cheers,
> Rob

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