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

[Jon TURNEY]

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.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: postinstall_failure_error2.patch
URL: <http://cygwin.com/pipermail/cygwin-apps/attachments/20100728/1050af23/attachment.ksh>