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 23/07/2010 19:49, Christopher Faylor wrote:
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'm not sure I understand your point here.

The difference between

if (success)
      success = f();

and

success &= f();

is the the latter doesn't short-circuit.

For clarity, the iterators in do_postinstall() are:

for each package
  for each file in /etc/postinstall belonging to that package

for each file in /etc/postinstall that doesn't end in .done

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.

Anyhow, here's another attempt, which unfortunately changes rather more than I wanted to. It adds a new page, which is displayed if any script failed, and reports which packages and scripts failed.


Attachment: postinstall_failure_error2.patch
Description: Text document


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