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 Wed, Jul 28, 2010 at 03:25:17PM +0100, Jon TURNEY wrote:
>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.

Ah, you're right.  I had it exactly backwards.  Sorry for the noise.

>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.

That is great.  Please check in (with a ChangeLog of course).

I just gave you the ability to check into cygwin-apps and, if you want to update
the documentation to winsup/doc.

cgf


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