On Tue, 8 Aug 2006, Igor Peshansky wrote:
> On Sun, 6 Aug 2006, Brian Dessent wrote:
>
> [snip]
> > http://cygwin.com/ml/cygwin-apps/2006-02/msg00099.html
> > So, we have a couple of issues here. Firstly, the bug/unintended
> > feature of -r causing the infinite retries until the file can be
> > written (if I understand correctly.) Second the patch by Igor that
> > adds a dialog when trying to replace an in-use file.
>
> Right.
>
> > Here is my opinion on the matter: I like the dialog idea, but I don't
> > think having "Abort" as an option is appropriate, as it will
> > potentially cause a really screwed up install, plus it was left
> > unimplemented in the patch submitted. So let's just have two
> > options: "Retry" and "Replace on Reboot". I know that this means we
> > can't use the stock "Abort/Retry/Ignore" dialog but I think it's
> > worth it for clarity.
>
> Agreed on all points. However, there is a technical issue here. Stock
> MessageBoxes come in many flavors -- there actually is a Retry/Cancel box.
> There is no Retry/Continue stock box, unfortunately.
>
> We can use the Retry/Cancel one, and perhaps play some games with the
> WNDPROC of the MessageBox class to make "Cancel" look like "Replace on
> reboot" (or "Continue", which I like better -- we can explain in the
> MessageBox text that pressing "Continue" will require a reboot later), but
> I'm not sure it'll work, and it'll be ugly. Plus, I don't know that much
> about the WNDPROC, so it'll take me a bit of time to get something like
> this working.
>
> OTOH, I can change my patch to use the Retry/Cancel box today, and add the
> following to the text: "Pressing 'Cancel' will cause setup to use Windows
> mechanisms for replacing in-use files. It will be necessary for you to
> reboot after setup completes." I know, the label "Cancel" is evocative of
> aborting the whole installation, but this functionality is so useful, IMO,
> that I, for one, would put up with a little annoyance of a wrong label.
> Changing the label in a way I've described above could be a later
> enhancement.
Well, lo and behold, I overreacted. It turned out to be much easier than
I anticipated, so attached is the patch with the Retry/Continue message
box.
> > http://cygwin.com/ml/cygwin-apps/2006-01/msg00204.html
> [snip]
> However, there was another issue in that thread (the inline patch). It
> seems that applying that will cause the code to be simpler, but I'm afraid
> there's some little issue I'm missing.
I still have that one in my private sandbox, and had it in my running copy
of setup for a while with no observed problems. Any opinions?
> > If we can finish off the "Retry/Replace" file-in-use thing and assuming
> > there are no reports of new issues with this snapshot then I think we
> > can push out a release.
>
> Sounds good. If people are fine with the Retry/Cancel box, I can have a
> new patch by the end of this week.
So my weeks end on Wednesdays... :-) Since this change involved indenting
an 80-line chunk of code, I'm also attaching a whitespace-indifferent
patch for ease of reviewing. ChangeLog is below.
Igor
==============================================================================
2006-08-16 Igor Peshansky <[EMAIL PROTECTED]>
* install.cc (Installer::installOne): If file is in use, ask the user
to stop processes and retry.
(MB_RETRYCONTINUE, IDCONTINUE): New macros.
(hMsgBoxHook): New static field.
(CBTProc): New window hook function.
(_custom_MessageBox): New function.
* CHANGES: Update with the above.
--
http://cs.nyu.edu/~pechtcha/
|\ _,,,---,,_ [EMAIL PROTECTED] | [EMAIL PROTECTED]
ZZZzz /,`.-'`' -. ;-;;,_ Igor Peshansky, Ph.D. (name changed!)
|,4- ) )-,_. ,\ ( `'-' old name: Igor Pechtchanski
'---''(_/--' `-'\_) fL a.k.a JaguaR-R-R-r-r-r-.-.-. Meow!
"Las! je suis sot... -Mais non, tu ne l'es pas, puisque tu t'en rends compte."
"But no -- you are no fool; you call yourself a fool, there's proof enough in
that!" -- Rostand, "Cyrano de Bergerac"Index: CHANGES
===================================================================
RCS file: /cvs/cygwin-apps/setup/CHANGES,v
retrieving revision 1.13
diff -u -p -r1.13 CHANGES
--- CHANGES 13 Jun 2006 14:00:23 -0000 1.13
+++ CHANGES 16 Aug 2006 03:56:05 -0000
@@ -3,6 +3,8 @@ Note: For easier maintenance try to keep
Version 2.548 (HEAD)
+ - Allow interactively retrying to replace open files.
+
- Fix unreadable chooser page due to bad background colour problem.
- Make categories named with an initial "." default to expanded display.
Index: install.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/install.cc,v
retrieving revision 2.78
diff -u -p -r2.78 install.cc
--- install.cc 16 Apr 2006 15:37:49 -0000 2.78
+++ install.cc 16 Aug 2006 03:56:06 -0000
@@ -178,6 +178,44 @@ Installer::replaceOnRebootSucceeded (con
rebootneeded = true;
}
+#define MB_RETRYCONTINUE 7
+#if !defined(IDCONTINUE)
+#define IDCONTINUE IDCANCEL
+#endif
+
+static HHOOK hMsgBoxHook;
+LRESULT CALLBACK CBTProc(int nCode, WPARAM wParam, LPARAM lParam) {
+ HWND hWnd;
+ switch (nCode) {
+ case HCBT_ACTIVATE:
+ hWnd = (HWND)wParam;
+ if (GetDlgItem(hWnd, IDCANCEL) != NULL)
+ SetDlgItemText(hWnd, IDCANCEL, "Continue");
+ UnhookWindowsHookEx(hMsgBoxHook);
+ }
+ return CallNextHookEx(hMsgBoxHook, nCode, wParam, lParam);
+}
+
+int _custom_MessageBox(HWND hWnd, LPCTSTR szText, LPCTSTR szCaption, UINT
uType) {
+ int retval;
+ bool retry_continue = (uType & MB_TYPEMASK) == MB_RETRYCONTINUE;
+ if (retry_continue) {
+ uType &= ~MB_TYPEMASK; uType |= MB_RETRYCANCEL;
+ // Install a window hook, so we can intercept the message-box
+ // creation, and customize it
+ // Only install for THIS thread!!!
+ hMsgBoxHook = SetWindowsHookEx(WH_CBT, CBTProc, NULL,
GetCurrentThreadId());
+ }
+ retval = MessageBox(hWnd, szText, szCaption, uType);
+ // Intercept the return value for less confusing results
+ if (retry_continue && retval == IDCANCEL)
+ return IDCONTINUE;
+ return retval;
+}
+
+#undef MessageBox
+#define MessageBox _custom_MessageBox
+
/* install one source at a given prefix. */
void
Installer::installOne (packagemeta &pkgm, const packageversion &ver,
@@ -205,6 +243,7 @@ Installer::installOne (packagemeta &pkgm
io_stream *tmp = io_stream::open (source.Cached (), "rb");
io_stream *tmp2 = 0;
archive *thefile = 0;
+ bool ignoreExtractErrors = unattended_mode;
if (tmp)
{
tmp2 = compress::decompress (tmp);
@@ -253,74 +292,101 @@ Installer::installOne (packagemeta &pkgm
Progress.SetText3 (canonicalfn.c_str());
log (LOG_BABBLE) << "Installing file " << prefixURL << prefixPath
<< fn << endLog;
- if (archive::extract_file (thefile, prefixURL, prefixPath) != 0)
+ bool firstIteration = true;
+ while (archive::extract_file (thefile, prefixURL, prefixPath) != 0)
{
- if (NoReplaceOnReboot)
+ if (!ignoreExtractErrors)
{
- ++errors;
- error_in_this_package = true;
- log (LOG_PLAIN) << "Not replacing in-use file "
- << prefixURL << prefixPath << fn << endLog;
+ char msg[fn.size() + 300];
+ sprintf (msg,
+ "%snable to extract /%s -- the file is in use.\r\n"
+ "Please stop %s Cygwin processes and select
\"Retry\", or\r\n"
+ "select \"Continue\" to go on anyway (you will need
to reboot).\r\n",
+ firstIteration?"U":"Still u", fn.c_str(),
firstIteration?"all":"ALL");
+ switch (MessageBox
+ (NULL, msg, "In-use files detected",
+ MB_RETRYCONTINUE | MB_ICONWARNING | MB_TASKMODAL))
+ {
+ case IDRETRY:
+ // retry
+ firstIteration = false;
+ continue;
+ case IDCONTINUE:
+ ignoreExtractErrors = true;
+ break;
+ default:
+ break;
+ }
+ // fall through to previous functionality
}
- else
- //extract to temp location
- if (archive::extract_file (thefile, prefixURL, prefixPath,
".new") != 0)
+ if (NoReplaceOnReboot)
{
- log (LOG_PLAIN) << "Unable to install file "
- << prefixURL << prefixPath << fn << endLog;
++errors;
- error_in_this_package = true;
+ error_in_this_package = true;
+ log (LOG_PLAIN) << "Not replacing in-use file "
+ << prefixURL << prefixPath << fn << endLog;
}
else
- {
- if (!IsWindowsNT())
- {
- /* Get the short file names */
- char source[MAX_PATH];
- unsigned int len =
- GetShortPathName(cygpath("/" + fn + ".new").c_str(),
- source, MAX_PATH);
- if (!len || len > MAX_PATH)
- {
- replaceOnRebootFailed(fn);
- }
- else
- {
- char dest[MAX_PATH];
- len =
- GetShortPathName(cygpath("/" + fn).c_str(),
- dest, MAX_PATH);
- if (!len || len > MAX_PATH)
- replaceOnRebootFailed (fn);
- else
- /* trigger a replacement on reboot */
- if (!WritePrivateProfileString
- ("rename", dest, source, "WININIT.INI"))
- replaceOnRebootFailed (fn);
- else
- replaceOnRebootSucceeded (fn, rebootneeded);
- }
- }
- else
- {
- /* XXX FIXME: prefix may not be / for in use files -
- * although it most likely is
- * - we need a io method to get win32 paths
- * or to wrap this system call
- */
- if (!MoveFileEx(cygpath("/" + fn + ".new").c_str(),
- cygpath("/" + fn).c_str(),
- MOVEFILE_DELAY_UNTIL_REBOOT |
- MOVEFILE_REPLACE_EXISTING))
- {
- replaceOnRebootFailed (fn);
- }
- else
- {
- replaceOnRebootSucceeded (fn, rebootneeded);
- }
- }
- }
+ //extract to temp location
+ if (archive::extract_file (thefile, prefixURL, prefixPath,
".new") != 0)
+ {
+ log (LOG_PLAIN) << "Unable to install file "
+ << prefixURL << prefixPath << fn << endLog;
+ ++errors;
+ error_in_this_package = true;
+ }
+ else
+ {
+ if (!IsWindowsNT())
+ {
+ /* Get the short file names */
+ char source[MAX_PATH];
+ unsigned int len =
+ GetShortPathName(cygpath("/" + fn + ".new").c_str(),
+ source, MAX_PATH);
+ if (!len || len > MAX_PATH)
+ {
+ replaceOnRebootFailed(fn);
+ }
+ else
+ {
+ char dest[MAX_PATH];
+ len =
+ GetShortPathName(cygpath("/" + fn).c_str(),
+ dest, MAX_PATH);
+ if (!len || len > MAX_PATH)
+ replaceOnRebootFailed (fn);
+ else
+ /* trigger a replacement on reboot */
+ if (!WritePrivateProfileString
+ ("rename", dest, source, "WININIT.INI"))
+ replaceOnRebootFailed (fn);
+ else
+ replaceOnRebootSucceeded (fn, rebootneeded);
+ }
+ }
+ else
+ {
+ /* XXX FIXME: prefix may not be / for in use files -
+ * although it most likely is
+ * - we need a io method to get win32 paths
+ * or to wrap this system call
+ */
+ if (!MoveFileEx(cygpath("/" + fn + ".new").c_str(),
+ cygpath("/" + fn).c_str(),
+ MOVEFILE_DELAY_UNTIL_REBOOT |
+ MOVEFILE_REPLACE_EXISTING))
+ {
+ replaceOnRebootFailed (fn);
+ }
+ else
+ {
+ replaceOnRebootSucceeded (fn, rebootneeded);
+ }
+ }
+ }
+ // We're done with this file
+ break;
}
progress (tmp->tell ());
num_installs++;
Index: CHANGES
===================================================================
RCS file: /cvs/cygwin-apps/setup/CHANGES,v
retrieving revision 1.13
diff -u -p -b -B -r1.13 CHANGES
--- CHANGES 13 Jun 2006 14:00:23 -0000 1.13
+++ CHANGES 16 Aug 2006 04:21:08 -0000
@@ -3,6 +3,8 @@ Note: For easier maintenance try to keep
Version 2.548 (HEAD)
+ - Allow interactively retrying to replace open files.
+
- Fix unreadable chooser page due to bad background colour problem.
- Make categories named with an initial "." default to expanded display.
Index: install.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/install.cc,v
retrieving revision 2.78
diff -u -p -b -B -r2.78 install.cc
--- install.cc 16 Apr 2006 15:37:49 -0000 2.78
+++ install.cc 16 Aug 2006 04:21:09 -0000
@@ -178,6 +178,44 @@ Installer::replaceOnRebootSucceeded (con
rebootneeded = true;
}
+#define MB_RETRYCONTINUE 7
+#if !defined(IDCONTINUE)
+#define IDCONTINUE IDCANCEL
+#endif
+
+static HHOOK hMsgBoxHook;
+LRESULT CALLBACK CBTProc(int nCode, WPARAM wParam, LPARAM lParam) {
+ HWND hWnd;
+ switch (nCode) {
+ case HCBT_ACTIVATE:
+ hWnd = (HWND)wParam;
+ if (GetDlgItem(hWnd, IDCANCEL) != NULL)
+ SetDlgItemText(hWnd, IDCANCEL, "Continue");
+ UnhookWindowsHookEx(hMsgBoxHook);
+ }
+ return CallNextHookEx(hMsgBoxHook, nCode, wParam, lParam);
+}
+
+int _custom_MessageBox(HWND hWnd, LPCTSTR szText, LPCTSTR szCaption, UINT
uType) {
+ int retval;
+ bool retry_continue = (uType & MB_TYPEMASK) == MB_RETRYCONTINUE;
+ if (retry_continue) {
+ uType &= ~MB_TYPEMASK; uType |= MB_RETRYCANCEL;
+ // Install a window hook, so we can intercept the message-box
+ // creation, and customize it
+ // Only install for THIS thread!!!
+ hMsgBoxHook = SetWindowsHookEx(WH_CBT, CBTProc, NULL,
GetCurrentThreadId());
+ }
+ retval = MessageBox(hWnd, szText, szCaption, uType);
+ // Intercept the return value for less confusing results
+ if (retry_continue && retval == IDCANCEL)
+ return IDCONTINUE;
+ return retval;
+}
+
+#undef MessageBox
+#define MessageBox _custom_MessageBox
+
/* install one source at a given prefix. */
void
Installer::installOne (packagemeta &pkgm, const packageversion &ver,
@@ -205,6 +243,7 @@ Installer::installOne (packagemeta &pkgm
io_stream *tmp = io_stream::open (source.Cached (), "rb");
io_stream *tmp2 = 0;
archive *thefile = 0;
+ bool ignoreExtractErrors = unattended_mode;
if (tmp)
{
tmp2 = compress::decompress (tmp);
@@ -253,8 +292,33 @@ Installer::installOne (packagemeta &pkgm
Progress.SetText3 (canonicalfn.c_str());
log (LOG_BABBLE) << "Installing file " << prefixURL << prefixPath
<< fn << endLog;
- if (archive::extract_file (thefile, prefixURL, prefixPath) != 0)
+ bool firstIteration = true;
+ while (archive::extract_file (thefile, prefixURL, prefixPath) != 0)
+ {
+ if (!ignoreExtractErrors)
+ {
+ char msg[fn.size() + 300];
+ sprintf (msg,
+ "%snable to extract /%s -- the file is in use.\r\n"
+ "Please stop %s Cygwin processes and select
\"Retry\", or\r\n"
+ "select \"Continue\" to go on anyway (you will need
to reboot).\r\n",
+ firstIteration?"U":"Still u", fn.c_str(),
firstIteration?"all":"ALL");
+ switch (MessageBox
+ (NULL, msg, "In-use files detected",
+ MB_RETRYCONTINUE | MB_ICONWARNING | MB_TASKMODAL))
{
+ case IDRETRY:
+ // retry
+ firstIteration = false;
+ continue;
+ case IDCONTINUE:
+ ignoreExtractErrors = true;
+ break;
+ default:
+ break;
+ }
+ // fall through to previous functionality
+ }
if (NoReplaceOnReboot)
{
++errors;
@@ -321,6 +385,8 @@ Installer::installOne (packagemeta &pkgm
}
}
}
+ // We're done with this file
+ break;
}
progress (tmp->tell ());
num_installs++;
