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

login
register
mail settings
Submitter Augie Fackler
Date June 8, 2017, 3:04 p.m.
Message ID <eed6a79c768cad00dcca.1496934244@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/21250/
State Superseded
Headers show

Comments

Augie Fackler - June 8, 2017, 3:04 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1496758188 14400
#      Tue Jun 06 10:09:48 2017 -0400
# Node ID eed6a79c768cad00dcca30b150eb8cd65b3e0d67
# Parent  bf999f47f7deff5a53e4eea18c22465ec738dd31
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.
Yuya Nishihara - June 8, 2017, 4 p.m.
On Thu, 08 Jun 2017 11:04:04 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1496758188 14400
> #      Tue Jun 06 10:09:48 2017 -0400
> # Node ID eed6a79c768cad00dcca30b150eb8cd65b3e0d67
> # Parent  bf999f47f7deff5a53e4eea18c22465ec738dd31
> 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
> @@ -10,6 +10,7 @@ from __future__ import absolute_import
>  import imp
>  import inspect
>  import os
> +import sys
>  
>  from .i18n import (
>      _,
> @@ -167,17 +168,31 @@ 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:
> +            ui.traceback(sys.exc_info())

Nit: no need to pass exc_info() explicitly. It's the default.

> @@ -203,15 +218,20 @@ def loadall(ui, whitelist=None):
>                  ui.warn(_("*** (%s)\n") % inst.hint)
>              ui.traceback()
>  
> +    broken = set()
>      for name in _order[newindex:]:
> -        _runuisetup(name, ui)
> +        if not _runuisetup(name, ui):
> +            broken.add(name)
>  
>      for name in _order[newindex:]:
> -        _runextsetup(name, ui)
> +        if name in broken:
> +            continue
> +        if not _runextsetup(name, ui):
> +            broken.add(name)

Still reposetup() would be called for broken extensions.
"_extensions[name] = None" looks ugly, but that's what we do right now.

>      # Call aftercallbacks that were never met.
>      for shortname in _aftercallbacks:
> -        if shortname in _extensions:
> +        if shortname in _extensions or shortname in broken:
>              continue

Perhaps _aftercallbacks had to check if _extensions[shortname] is None or not.
That's an old bug.
Augie Fackler - June 8, 2017, 4:57 p.m.
> On Jun 8, 2017, at 12:00, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>>     for name in _order[newindex:]:
>> -        _runextsetup(name, ui)
>> +        if name in broken:
>> +            continue
>> +        if not _runextsetup(name, ui):
>> +            broken.add(name)
> 
> Still reposetup() would be called for broken extensions.
> "_extensions[name] = None" looks ugly, but that's what we do right now.

Hm. I'm not sure I follow. I think I did the right thing.

> 
>>     # Call aftercallbacks that were never met.
>>     for shortname in _aftercallbacks:
>> -        if shortname in _extensions:
>> +        if shortname in _extensions or shortname in broken:
>>             continue
> 
> Perhaps _aftercallbacks had to check if _extensions[shortname] is None or not.
> That's an old bug.

Oh, yeah, it should. I fixed it in this patch, but can split it if v3 looks right.
Yuya Nishihara - June 9, 2017, 1:15 p.m.
On Thu, 8 Jun 2017 12:57:30 -0400, Augie Fackler wrote:
> 
> > On Jun 8, 2017, at 12:00, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >>     # Call aftercallbacks that were never met.
> >>     for shortname in _aftercallbacks:
> >> -        if shortname in _extensions:
> >> +        if shortname in _extensions or shortname in broken:
> >>             continue
> > 
> > Perhaps _aftercallbacks had to check if _extensions[shortname] is None or not.
> > That's an old bug.
> 
> Oh, yeah, it should. I fixed it in this patch, but can split it if v3 looks right.

Oops, my bad. _aftercallbacks here should never be called once extension is
successfully imported. So loaded is always False.

Patch

diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -10,6 +10,7 @@  from __future__ import absolute_import
 import imp
 import inspect
 import os
+import sys
 
 from .i18n import (
     _,
@@ -167,17 +168,31 @@  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:
+            ui.traceback(sys.exc_info())
+            msg = _forbytes(inst)
+            ui.warn(_("*** failed to set up extension %s: %s\n") % (name, msg))
+            return False
+    return True
 
 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:
+            ui.traceback(sys.exc_info())
+            msg = _forbytes(inst)
+            ui.warn(_("*** failed to set up extension %s: %s\n") % (name, msg))
+            return False
+    return True
 
 def loadall(ui, whitelist=None):
     result = ui.configitems("extensions")
@@ -203,15 +218,20 @@  def loadall(ui, whitelist=None):
                 ui.warn(_("*** (%s)\n") % inst.hint)
             ui.traceback()
 
+    broken = set()
     for name in _order[newindex:]:
-        _runuisetup(name, ui)
+        if not _runuisetup(name, ui):
+            broken.add(name)
 
     for name in _order[newindex:]:
-        _runextsetup(name, ui)
+        if name in broken:
+            continue
+        if not _runextsetup(name, ui):
+            broken.add(name)
 
     # Call aftercallbacks that were never met.
     for shortname in _aftercallbacks:
-        if shortname in _extensions:
+        if shortname in _extensions or shortname in broken:
             continue
 
         for fn in _aftercallbacks[shortname]:
diff --git a/tests/test-extension.t b/tests/test-extension.t
--- a/tests/test-extension.t
+++ b/tests/test-extension.t
@@ -1625,9 +1625,35 @@  Make sure a broken uisetup doesn't globa
   > baduisetup = $PWD/baduisetup.py
   > EOF
 
-Broken: an extension that triggers the import of bdiff during uisetup
-can't be easily debugged:
+Even though the extension fails during uisetup, hg is still basically usable:
   $ 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.
+
+  $ hg version --traceback
+  Traceback (most recent call last):
+    File "*/mercurial/extensions.py", line *, in _runuisetup (glob)
+      uisetup(ui)
+    File "$TESTTMP/baduisetup.py", line 10, in uisetup
+      extensions.wrapfunction(bdiff, 'blocks', blockswrapper)
+    File "*/mercurial/extensions.py", line *, in wrapfunction (glob)
+      origfn = getattr(container, funcname)
+    File "*/hgdemandimport/demandimportpy2.py", line *, in __getattr__ (glob)
+      self._load()
+    File "*/hgdemandimport/demandimportpy2.py", line *, in _load (glob)
+      mod = _hgextimport(_import, head, globals, locals, None, level)
+    File "*/hgdemandimport/demandimportpy2.py", line *, in _hgextimport (glob)
+      return importfunc(name, globals, *args, **kwargs)
+  ImportError: No module named bdiff
+  *** 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]
 
   $ cd ..