This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]