[PATCH] inform user if any postinstall script failed to run
Christopher Faylor
cgf-use-the-mailinglist-please@cygwin.com
Wed Jul 28 23:29:00 GMT 2010
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
More information about the Cygwin-apps
mailing list