This is the mail archive of the
cygwin-apps
mailing list for the Cygwin project.
Re: [PATCH] inform user if any postinstall script failed to run
- From: Christopher Faylor <cgf-use-the-mailinglist-please at cygwin dot com>
- To: cygwin-apps at cygwin dot com
- Date: Fri, 23 Jul 2010 14:49:20 -0400
- Subject: Re: [PATCH] inform user if any postinstall script failed to run
- References: <4C49D54B.1030900@dronecode.org.uk>
- Reply-to: cygwin-apps at cygwin dot com
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