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
