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.
Index: Makefile.am
===================================================================
RCS file: /cvs/cygwin-apps/setup/Makefile.am,v
retrieving revision 2.81
diff -u -p -r2.81 Makefile.am
--- Makefile.am 8 Apr 2010 15:50:38 -0000 2.81
+++ Makefile.am 28 Jul 2010 13:48:56 -0000
@@ -236,6 +236,8 @@ setup_SOURCES = \
PickView.cc \
PickView.h \
postinstall.cc \
+ postinstallresults.cc \
+ postinstallresults.h \
prereq.cc \
prereq.h \
proppage.cc \
Index: main.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/main.cc,v
retrieving revision 2.63
diff -u -p -r2.63 main.cc
--- main.cc 15 Feb 2010 00:45:01 -0000 2.63
+++ main.cc 28 Jul 2010 13:48:56 -0000
@@ -64,6 +64,7 @@ static const char *cvsid =
#include "prereq.h"
#include "threebar.h"
#include "desktop.h"
+#include "postinstallresults.h"
#include "getopt++/GetOption.h"
#include "getopt++/BoolOption.h"
@@ -117,8 +118,9 @@ set_cout ()
}
}
-// Other threads talk to this page, so we need to have it externable.
+// Other threads talk to these pages, so we need to have it externable.
ThreeBarProgressPage Progress;
+PostInstallResultsPage PostInstallResults;
// This is a little ugly, but the decision about where to log occurs
// after the source is set AND the root mount obtained
@@ -197,6 +199,7 @@ main_display ()
Chooser.Create ();
Prereq.Create ();
Progress.Create ();
+ PostInstallResults.Create ();
Desktop.Create ();
// Add pages to sheet
@@ -210,6 +213,7 @@ main_display ()
MainWindow.AddPage (&Chooser);
MainWindow.AddPage (&Prereq);
MainWindow.AddPage (&Progress);
+ MainWindow.AddPage (&PostInstallResults);
MainWindow.AddPage (&Desktop);
// Create the PropSheet main window
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 28 Jul 2010 13:48:56 -0000
@@ -32,12 +32,18 @@ static const char *cvsid =
#include "resource.h"
#include "threebar.h"
#include "Exception.h"
+#include "postinstallresults.h"
-#include <algorithm>
+#include <sstream>
using namespace std;
extern ThreeBarProgressPage Progress;
+extern PostInstallResultsPage PostInstallResults;
+
+// ---------------------------------------------------------------------------
+//
+// ---------------------------------------------------------------------------
class RunFindVisitor : public FindVisitor
{
@@ -61,33 +67,62 @@ private:
vector<Script> *_scripts;
};
-class RunScript : public unary_function<Script const &, int>
+// ---------------------------------------------------------------------------
+//
+// ---------------------------------------------------------------------------
+
+class RunScript
{
public:
- RunScript(const std::string& name, int num) : _num(num), _cnt(0)
+ RunScript(const std::string& name, const vector<Script> &scripts) :
_name(name), _scripts(scripts), _cnt(0)
{
Progress.SetText2 (name.c_str());
- Progress.SetBar1 (_cnt, _num);
+ Progress.SetBar1 (0, _scripts.size());
}
virtual ~RunScript()
{
Progress.SetText3 ("");
}
- int operator() (Script const &aScript)
+ int run_one(Script const &aScript)
{
int retval;
Progress.SetText3 (aScript.fullName().c_str());
retval = aScript.run();
++_cnt;
- Progress.SetBar1 (_cnt, _num);
+ Progress.SetBar1 (_cnt, _scripts.size());
return retval;
}
+ void run_all(std::string &s)
+ {
+ bool package_name_recorded = FALSE;
+
+ for (std::vector <Script>::const_iterator j = _scripts.begin();
+ j != _scripts.end();
+ j++)
+ {
+ int retval = run_one(*j);
+
+ if ((retval != 0) && (retval != -ERROR_INVALID_DATA))
+ {
+ if (!package_name_recorded)
+ {
+ s = s + "Package: " + _name + "\r\n";
+ package_name_recorded = TRUE;
+ }
+
+ std::ostringstream fs;
+ fs << "\t" << j->baseName() << " exit code " << retval << "\r\n";
+ s = s + fs.str();
+ }
+ }
+ }
private:
- int _num;
+ std::string _name;
+ const vector<Script> &_scripts;
int _cnt;
};
-static void
+static std::string
do_postinstall_thread (HINSTANCE h, HWND owner)
{
Progress.SetText1 ("Running...");
@@ -108,26 +143,39 @@ do_postinstall_thread (HINSTANCE h, HWND
packages.push_back(&pkg);
++i;
}
+
+ std::string s = "";
+
+ // For each package we installed, we noted anything installed into
/etc/postinstall.
+ // run those scripts now
int numpkg = packages.size() + 1;
int k = 0;
for (i = packages.begin (); i != packages.end (); ++i)
{
packagemeta & pkg = **i;
- for_each (pkg.installed.scripts().begin(), pkg.installed.scripts().end(),
- RunScript(pkg.name, pkg.installed.scripts().size()));
+ RunScript scriptRunner(pkg.name, pkg.installed.scripts());
+ scriptRunner.run_all(s);
++k;
Progress.SetBar2 (k, numpkg);
}
+
+ // Look for any scripts in /etc/postinstall which haven't been renamed .done,
+ // and try to run them...
std::string postinst = cygpath ("/etc/postinstall");
vector<Script> scripts;
RunFindVisitor myVisitor (&scripts);
- Progress.SetBar1 (0, 1);
Find (postinst).accept (myVisitor);
- for_each (scripts.begin(), scripts.end(),
- RunScript("No package", scripts.size()));
+
+ {
+ RunScript scriptRunner("No package", scripts);
+ scriptRunner.run_all(s);
+ }
+
Progress.SetBar2 (numpkg, numpkg);
+
+ return s;
}
static DWORD WINAPI
@@ -138,10 +186,14 @@ do_postinstall_reflector (void *p)
try
{
- do_postinstall_thread ((HINSTANCE) context[0], (HWND) context[1]);
+ std::string s = do_postinstall_thread ((HINSTANCE) context[0], (HWND)
context[1]);
+
+ // Tell the postinstall results page the results string
+ PostInstallResults.SetResultsString(s);
// Tell the progress page that we're done running scripts
- Progress.PostMessage (WM_APP_POSTINSTALL_THREAD_COMPLETE);
+ Progress.PostMessage (WM_APP_POSTINSTALL_THREAD_COMPLETE,
+ s.empty() ? IDD_DESKTOP : IDD_POSTINSTALL);
}
TOPLEVEL_CATCH("postinstall");
Index: postinstallresults.cc
===================================================================
RCS file: postinstallresults.cc
diff -N postinstallresults.cc
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ postinstallresults.cc 28 Jul 2010 13:48:56 -0000
@@ -0,0 +1,81 @@
+/*
+ * Copyright (c) 2010 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * A copy of the GNU General Public License can be found at
+ * http://www.gnu.org/
+ *
+ */
+
+#include "postinstallresults.h"
+#include "resource.h"
+
+// ---------------------------------------------------------------------------
+// implements class PostInstallResultsPage
+//
+// postinstall errors page
+// postinstall script running itself is part of the progress page
+// ---------------------------------------------------------------------------
+
+// Sizing information.
+static ControlAdjuster::ControlInfo PostInstallResultsControlsInfo[] = {
+ {IDC_POSTINSTALL_EDIT, CP_STRETCH, CP_STRETCH},
+ {0, CP_LEFT, CP_TOP}
+};
+
+PostInstallResultsPage::PostInstallResultsPage ()
+{
+ sizeProcessor.AddControlInfo (PostInstallResultsControlsInfo);
+}
+
+bool
+PostInstallResultsPage::Create ()
+{
+ return PropertyPage::Create (IDD_POSTINSTALL);
+}
+
+void
+PostInstallResultsPage::OnInit ()
+{
+ // set the edit-area to a larger font
+ SetDlgItemFont(IDC_POSTINSTALL_EDIT, "MS Shell Dlg", 10);
+}
+
+void
+PostInstallResultsPage::OnActivate()
+{
+ SetDlgItemText(GetHWND(), IDC_POSTINSTALL_EDIT, _results.c_str ());
+}
+
+long
+PostInstallResultsPage::OnNext ()
+{
+ // one or more postinstall scripts failed to run successfully
+ // installation may be broken
+ MessageBox (NULL,
+ "You will need to investigate and correct these errors "
+ "before your Cygwin installation will function properly.\n"
+ "Check setup.log for details.",
+ "ERROR - postinstall scripts failed",
+ MB_OK | MB_ICONERROR | MB_SETFOREGROUND | MB_TOPMOST);
+
+ return IDD_DESKTOP;
+}
+
+long
+PostInstallResultsPage::OnBack ()
+{
+ return 0;
+}
+
+long
+PostInstallResultsPage::OnUnattended ()
+{
+ // in unattended mode, we have logged the errors, so just carry on
+ // XXX: would be nice to set program exit status if errors occured...
+ return 0;
+}
Index: postinstallresults.h
===================================================================
RCS file: postinstallresults.h
diff -N postinstallresults.h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ postinstallresults.h 28 Jul 2010 13:48:56 -0000
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2010 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * A copy of the GNU General Public License can be found at
+ * http://www.gnu.org/
+ *
+ */
+
+#ifndef SETUP_POSTINSTALL_H
+#define SETUP_POSTINSTALL_H
+
+#include "proppage.h"
+
+class PostInstallResultsPage:public PropertyPage
+{
+public:
+ PostInstallResultsPage ();
+ virtual ~PostInstallResultsPage () { };
+ bool Create ();
+ virtual void OnInit ();
+ virtual void OnActivate ();
+ virtual long OnNext ();
+ virtual long OnBack ();
+ virtual long OnUnattended ();
+ void SetResultsString (std::string results) { _results = results; };
+ private:
+ std::string _results;
+};
+
+#endif /* SETUP_POSTINSTALL_H */
Index: res.rc
===================================================================
RCS file: /cvs/cygwin-apps/setup/res.rc,v
retrieving revision 2.84
diff -u -p -r2.84 res.rc
--- res.rc 23 Jul 2010 10:21:54 -0000 2.84
+++ res.rc 28 Jul 2010 13:48:57 -0000
@@ -422,6 +422,25 @@ BEGIN
END
+IDD_POSTINSTALL DIALOG DISCARDABLE 0, 0, SETUP_STANDARD_DIALOG_W, 142
+STYLE DS_MODALFRAME | DS_3DLOOK | DS_CENTER | WS_CHILD | WS_VISIBLE |
+ WS_CAPTION | WS_SYSMENU
+CAPTION "Cygwin Setup - Running postinstall scripts"
+FONT 8, "MS Shell Dlg"
+BEGIN
+ CONTROL "",IDC_HEADSEPARATOR,"Static",SS_BLACKFRAME | SS_SUNKEN,
+ 0,28,SETUP_STANDARD_DIALOG_W,1
+ ICON IDI_CYGWIN,IDC_HEADICON,SETUP_HEADICON_X,0,21,20
+ LTEXT "Postinstall script errors",IDC_STATIC_HEADER_TITLE
+ ,7,0,258,8,NOT WS_GROUP
+ LTEXT "The following errors occured executing postinstall
scripts",
+ IDC_STATIC,21,9,239,16,NOT WS_GROUP
+ EDITTEXT IDC_POSTINSTALL_EDIT,7,41,303,124,WS_VSCROLL | WS_HSCROLL |
+ ES_LEFT | ES_MULTILINE | ES_READONLY | ES_AUTOHSCROLL |
+ ES_AUTOVSCROLL
+
+END
+
/////////////////////////////////////////////////////////////////////////////
//
// Manifest
Index: resource.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/resource.h,v
retrieving revision 2.40
diff -u -p -r2.40 resource.h
--- resource.h 8 Dec 2009 22:26:44 -0000 2.40
+++ resource.h 28 Jul 2010 13:48:57 -0000
@@ -64,6 +64,7 @@
#define IDD_DESKTOP 219
#define IDD_PREREQ 220
#define IDD_DROPPED 221
+#define IDD_POSTINSTALL 222
// Bitmaps
@@ -171,3 +172,4 @@
#define IDC_CHOOSE_SEARCH_LABEL 586
#define IDC_CHOOSE_CLEAR_SEARCH 587
#define IDC_LOCAL_DIR_DESC 588
+#define IDC_POSTINSTALL_EDIT 589
Index: script.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/script.cc,v
retrieving revision 2.35
diff -u -p -r2.35 script.cc
--- script.cc 26 Jun 2009 12:08:54 -0000 2.35
+++ script.cc 28 Jul 2010 13:48:57 -0000
@@ -285,11 +285,13 @@ Script::run() const
if (retval)
log(LOG_PLAIN) << "abnormal exit: exit code=" << retval << endLog;
- /* if file exists then delete it otherwise just ignore no file error */
+ /* if .done file exists then delete it otherwise just ignore no file error */
io_stream::remove ("cygfile://" + scriptName + ".done");
- io_stream::move ("cygfile://" + scriptName,
- "cygfile://" + scriptName + ".done");
+ /* don't rename the script as .done if it didn't run successfully */
+ if (!retval)
+ io_stream::move ("cygfile://" + scriptName,
+ "cygfile://" + scriptName + ".done");
return retval;
}
Index: threebar.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/threebar.cc,v
retrieving revision 2.16
diff -u -p -r2.16 threebar.cc
--- threebar.cc 22 Nov 2009 18:58:00 -0000 2.16
+++ threebar.cc 28 Jul 2010 13:48:57 -0000
@@ -208,9 +208,7 @@ ThreeBarProgressPage::OnMessageApp (UINT
}
case WM_APP_POSTINSTALL_THREAD_COMPLETE:
{
- // Re-enable and "Push" the Next button
- GetOwner ()->SetButtons (PSWIZB_NEXT);
- GetOwner ()->PressButton (PSBTN_NEXT);
+ GetOwner ()->SetActivePageByID (lParam);
break;
}
case WM_APP_START_SITE_INFO_DOWNLOAD:
