Re: [PATCH] inform user if any postinstall script failed to run

[Christopher Faylor <cgf-use-the-mailinglist-please at cygwin dot com>]

On Fri, Jul 23, 2010 at 06:45:47PM +0100, Jon TURNEY wrote:
>
>Here's a small patch for setup.exe which causes setup to indicate if a 
>postinstall script didn't run successfully.
>
>This should help avoid the situation where the postinstall scripts fail to run 
>and the user has a broken installation, but they don't notice until they try 
>to run something which relies on postinstall scripts, e.g [1] and I'm sure 
>there are other examples.
>
>[1] http://cygwin.com/ml/cygwin-xfree/2010-07/msg00084.html
>
>ChangeLog:
>
>	* postinstall.cc : Note if any postinstall script fails and tell
>	the user with an error messagebox
>	* script.cc (run): Don't rename script as .done if it didn't run
>	successfully

Thanks for doing this.  I like the idea but I have some stylistic comments.

>Index: postinstall.cc
>===================================================================
>RCS file: /cvs/cygwin-apps/setup/postinstall.cc,v
>retrieving revision 2.23
>diff -u -p -r2.23 postinstall.cc
>--- postinstall.cc	11 May 2009 10:49:15 -0000	2.23
>+++ postinstall.cc	23 Jul 2010 17:34:30 -0000
>@@ -64,7 +64,7 @@ private:
> class RunScript : public unary_function<Script const &, int>
> {
> public:
>-  RunScript(const std::string& name, int num) : _num(num), _cnt(0)
>+  RunScript(const std::string& name, int num) : _num(num), _cnt(0), _no_errors(TRUE)
>     {
>       Progress.SetText2 (name.c_str());
>       Progress.SetBar1 (_cnt, _num);
>@@ -80,16 +80,26 @@ public:
>       retval = aScript.run();
>       ++_cnt;
>       Progress.SetBar1 (_cnt, _num);
>+
>+      if ((retval != 0) && (retval != -ERROR_INVALID_DATA))
>+        _no_errors = FALSE;
>+
>       return retval;
>     }
>+  bool success(void)
>+  {
>+    return _no_errors;
>+  }
> private:
>   int _num;
>   int _cnt;
>+  bool _no_errors;
> };
> 
>-static void
>+static bool
> do_postinstall_thread (HINSTANCE h, HWND owner)
> {
>+  bool success = TRUE;
>   Progress.SetText1 ("Running...");
>   Progress.SetText2 ("");
>   Progress.SetText3 ("");
>@@ -114,8 +124,8 @@ do_postinstall_thread (HINSTANCE h, HWND
>     {
>       packagemeta & pkg = **i;
> 
>-      for_each (pkg.installed.scripts().begin(), pkg.installed.scripts().end(),
>-		RunScript(pkg.name, pkg.installed.scripts().size()));
>+      success &= for_each (pkg.installed.scripts().begin(), pkg.installed.scripts().end(),
>+                           RunScript(pkg.name, pkg.installed.scripts().size())).success();

I'd prefer a simple if here like:
if (success)
    success = ...

But, OTOH, if I'm reading this correctly, you stop doing the install
scripts after the first one fails.  I think it may be better to record
the failing scripts, keep going, and then report the package names which
failed rather than forcing the user to look around.

cgf