Patchwork [2,of,2] extensions: catch uisetup and extsetup failures and don't let them break hg

login
register
mail settings
Submitter via Mercurial-devel
Date June 6, 2017, 2:36 p.m.
Message ID <CAESOdVBZ+H61_-jCDxykcY4h59Y2-w-0pk5B8ULTkBzMwOxBng@mail.gmail.com>
Download mbox | patch
Permalink /patch/21219/
State Not Applicable
Headers show

Comments

via Mercurial-devel - June 6, 2017, 2:36 p.m.
On Jun 6, 2017 7:15 AM, "Augie Fackler" <raf@durin42.com> wrote:

# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1496758188 14400
#      Tue Jun 06 10:09:48 2017 -0400
# Node ID 9681ff165ea757e99bbb355412708f8d45f922c1
# Parent  656e5d8cab2048029eb16b7096bd44f9abe5bf7b
extensions: catch uisetup and extsetup failures and don't let them break hg

Otherwise users of the patience diff extension will be unable to run
anything at all in hg 4.3 until they figure out what's broken.

   $ cd ..
Augie Fackler - June 6, 2017, 2:57 p.m.
> On Jun 6, 2017, at 10:36, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
> 
>> 
>> On Jun 6, 2017 7:15 AM, "Augie Fackler" <raf@durin42.com> wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1496758188 14400
>> #      Tue Jun 06 10:09:48 2017 -0400
>> # Node ID 9681ff165ea757e99bbb355412708f8d45f922c1
>> # Parent  656e5d8cab2048029eb16b7096bd44f9abe5bf7b
>> extensions: catch uisetup and extsetup failures and don't let them break hg
>> 
>> Otherwise users of the patience diff extension will be unable to run
>> anything at all in hg 4.3 until they figure out what's broken.
>> 
>> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
>> --- a/mercurial/extensions.py
>> +++ b/mercurial/extensions.py
>> @@ -167,17 +167,25 @@ def load(ui, name, path):
>>  def _runuisetup(name, ui):
>>      uisetup = getattr(_extensions[name], 'uisetup', None)
>>      if uisetup:
>> -        uisetup(ui)
>> +        try:
>> +            uisetup(ui)
>> +        except Exception as inst:
>> +            msg = _forbytes(inst)
>> +            ui.warn(_("*** failed to set up extension %s: %s\n") % (name, msg))
>> 
> Maybe we should re-raise so the extension doesn't end up half loaded?

Maybe, but I'd rather try it this way in the hopes that it helps users by at least letting them run 'debuginstall' or similar without having to tweak their config.
Yuya Nishihara - June 7, 2017, 2:53 p.m.
On Tue, 6 Jun 2017 10:57:04 -0400, Augie Fackler wrote:
> 
> > On Jun 6, 2017, at 10:36, Martin von Zweigbergk via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> > 
> > 
> >> 
> >> On Jun 6, 2017 7:15 AM, "Augie Fackler" <raf@durin42.com> wrote:
> >> # HG changeset patch
> >> # User Augie Fackler <augie@google.com>
> >> # Date 1496758188 14400
> >> #      Tue Jun 06 10:09:48 2017 -0400
> >> # Node ID 9681ff165ea757e99bbb355412708f8d45f922c1
> >> # Parent  656e5d8cab2048029eb16b7096bd44f9abe5bf7b
> >> extensions: catch uisetup and extsetup failures and don't let them break hg
> >> 
> >> Otherwise users of the patience diff extension will be unable to run
> >> anything at all in hg 4.3 until they figure out what's broken.
> >> 
> >> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> >> --- a/mercurial/extensions.py
> >> +++ b/mercurial/extensions.py
> >> @@ -167,17 +167,25 @@ def load(ui, name, path):
> >>  def _runuisetup(name, ui):
> >>      uisetup = getattr(_extensions[name], 'uisetup', None)
> >>      if uisetup:
> >> -        uisetup(ui)
> >> +        try:
> >> +            uisetup(ui)
> >> +        except Exception as inst:
> >> +            msg = _forbytes(inst)
> >> +            ui.warn(_("*** failed to set up extension %s: %s\n") % (name, msg))
> >> 
> > Maybe we should re-raise so the extension doesn't end up half loaded?
> 
> Maybe, but I'd rather try it this way in the hopes that it helps users by at least letting them run 'debuginstall' or similar without having to tweak their config.

This seems a good improvement. Can you add ui.traceback() so more detailed
error will be printed with --traceback? And maybe we could nullify
_extensions[name] to e.g. stop calling extsetup() if uisetup() failed.

Patch

diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -167,17 +167,25 @@  def load(ui, name, path):
 def _runuisetup(name, ui):
     uisetup = getattr(_extensions[name], 'uisetup', None)
     if uisetup:
-        uisetup(ui)
+        try:
+            uisetup(ui)
+        except Exception as inst:
+            msg = _forbytes(inst)
+            ui.warn(_("*** failed to set up extension %s: %s\n") % (name,
msg))


Maybe we should re-raise so the extension doesn't end up half loaded?


 def _runextsetup(name, ui):
     extsetup = getattr(_extensions[name], 'extsetup', None)
     if extsetup:
         try:
-            extsetup(ui)
-        except TypeError:
-            if inspect.getargspec(extsetup).args:
-                raise
-            extsetup() # old extsetup with no ui argument
+            try:
+                extsetup(ui)
+            except TypeError:
+                if inspect.getargspec(extsetup).args:
+                    raise
+                extsetup() # old extsetup with no ui argument
+        except Exception as inst:
+            msg = _forbytes(inst)
+            ui.warn(_("*** failed to set up extension %s: %s\n") % (name,
msg))

 def loadall(ui, whitelist=None):
     result = ui.configitems("extensions")
diff --git a/tests/test-extension.t b/tests/test-extension.t
--- a/tests/test-extension.t
+++ b/tests/test-extension.t
@@ -1628,6 +1628,10 @@  Make sure a broken uisetup doesn't globa
 Broken: an extension that triggers the import of bdiff during uisetup
 can't be easily debugged:
   $ hg version
-  abort: No module named bdiff!
-  (did you forget to compile extensions?)
-  [255]
+  *** failed to set up extension baduisetup: No module named bdiff
+  Mercurial Distributed SCM (version *) (glob)
+  (see https://mercurial-scm.org for more information)
+
+  Copyright (C) 2005-2017 Matt Mackall and others
+  This is free software; see the source for copying conditions. There is NO
+  warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
PURPOSE.
diff --git a/tests/test-flagprocessor.t b/tests/test-flagprocessor.t
--- a/tests/test-flagprocessor.t
+++ b/tests/test-flagprocessor.t
@@ -161,7 +161,8 @@ 
   > EOF
   $ echo 'this should fail' > file
   $ hg commit -Aqm 'add file'
-  abort: cannot register multiple processors on flag '0x8'.
+  *** failed to set up extension duplicate: cannot register multiple
processors on flag '0x8'.
+  abort: missing processor for flag '0x1'!
   [255]