Skip to content

Commit

Permalink
debugruntest: improve compatibility with extensions
Browse files Browse the repository at this point in the history
Summary:
debugruntest executes sl commands in-process to eliminate process startup overhead. This is incompatible with extensions changing during the test since the extensions are only loaded once, installing their function wrappers. If a later command disables the extension, the function wrappers (and other side effects) will still remain, probably messing things up.

In this diff I attempt to fix the function wrappers to be compatible with debugruntest. Basically, when wrapping things we install another layer of wrapping that checks at runtime whether the extension is still enabled (and doesn't run the wrapper if the extension is disabled).

Another approach would be to track all the wrappers and unwrap/rewrap them as we notice extensions have been disbled/enabled, but this way seemed simpler and maybe more reliable.

Reviewed By: quark-zju

Differential Revision: D54839559

fbshipit-source-id: 071bc0e832e48c7049f33c720007ab958a51e933
  • Loading branch information
muirdm authored and facebook-github-bot committed Mar 19, 2024
1 parent cd14439 commit bdf4505
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 4 deletions.
2 changes: 1 addition & 1 deletion eden/scm/sapling/dispatch.py
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,7 @@ def _dispatch(req):
progress.init()
# Configure extensions in phases: uisetup, extsetup, cmdtable, and
# reposetup
extensions.loadall(lui)
extensions.initialload(lui)
# Propagate any changes to lui.__class__ by extensions
ui.__class__ = lui.__class__

Expand Down
65 changes: 62 additions & 3 deletions eden/scm/sapling/extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,9 @@ def load(ui, name, path):
shortname = name
if shortname in _ignoreextensions:
return None

_loaded_extensions.add(shortname)

if shortname in _extensions:
return _extensions[shortname]
_extensions[shortname] = None
Expand Down Expand Up @@ -382,11 +385,31 @@ def _getattr(obj, name, default):
raise


_extension_being_loaded = None


class _current_extension:
def __init__(self, name):
self.name = name

def __enter__(self):
global _extension_being_loaded
self.prev_value = _extension_being_loaded
_extension_being_loaded = self.name
return None

def __exit__(self, exc_type, exc_value, traceback):
global _extension_being_loaded
_extension_being_loaded = self.prev_value
return None


def _runuisetup(name, ui):
uisetup = getattr(_extensions[name], "uisetup", None)
if uisetup:
try:
uisetup(ui)
with _current_extension(name):
uisetup(ui)
except Exception as inst:
ui.traceback(force=True)
msg = util.forcebytestr(inst)
Expand All @@ -395,6 +418,7 @@ def _runuisetup(name, ui):
notice=_("warning"),
)
return False

return True


Expand All @@ -403,7 +427,8 @@ def _runextsetup(name, ui):
if extsetup:
try:
try:
extsetup(ui)
with _current_extension(name):
extsetup(ui)
except TypeError:
# Try to use getfullargspec (Python 3) first, and fall
# back to getargspec only if it doesn't exist so as to
Expand All @@ -412,7 +437,8 @@ def _runextsetup(name, ui):
extsetup
).args:
raise
extsetup() # old extsetup with no ui argument
with _current_extension(name):
extsetup() # old extsetup with no ui argument
except Exception as inst:
ui.traceback(force=True)
msg = util.forcebytestr(inst)
Expand All @@ -424,6 +450,17 @@ def _runextsetup(name, ui):
return True


# Track which extensions are enabled for the current command invocation. This is for
# debugruntest to disable extensions that were previously loaded in-process but are not
# currently active.
_loaded_extensions = set()


def initialload(ui):
_loaded_extensions.clear()
loadall(ui)


def loadall(ui, include_list=None):
result = ui.configitems("extensions")
resultkeys = set([name for name, loc in result])
Expand Down Expand Up @@ -647,6 +684,8 @@ def wrapcommand(table, command, wrapper, synopsis=None, docstring=None):
key = alias
break

wrapper = maybe_wrap_wrapper(wrapper)

origfn = entry[0]
wrap = functools.partial(util.checksignature(wrapper), util.checksignature(origfn))
_updatewrapper(wrap, origfn, wrapper)
Expand All @@ -668,6 +707,8 @@ def wrapfilecache(cls, propname, wrapper):
"""
propname = propname
assert callable(wrapper)

wrapper = maybe_wrap_wrapper(wrapper)
for currcls in cls.__mro__:
if propname in currcls.__dict__:
origfn = currcls.__dict__[propname].func
Expand All @@ -683,6 +724,22 @@ def wrap(*args, **kwargs):
raise AttributeError(r"type '%s' has no property '%s'" % (cls, propname))


# For debugruntest, add an outer layer of wrapping so we can "disable" the inner wrapper
# if the extension that installed the wrapper is not currently enabled.
def maybe_wrap_wrapper(wrapper):
if not util.istest() or not _extension_being_loaded:
return wrapper

extname = _extension_being_loaded

def wrapperwrapper(origfn, *args, **kwargs):
if extname and not extname in _loaded_extensions:
return origfn(*args, **kwargs)
return wrapper(origfn, *args, **kwargs)

return wrapperwrapper


class wrappedfunction:
"""context manager for temporarily wrapping a function"""

Expand Down Expand Up @@ -734,6 +791,8 @@ def whatever(self, *args, **kwargs):
"""
assert callable(wrapper)

wrapper = maybe_wrap_wrapper(wrapper)

origfn = getattr(container, funcname)
assert callable(origfn)
if inspect.ismodule(container):
Expand Down
41 changes: 41 additions & 0 deletions eden/scm/tests/test-debugruntest-extensions.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#debugruntest-compatible

$ cat > ex1.py <<EOS
> from sapling import commands, extensions
> def uisetup(ui):
> def files(orig, ui, *args, **kwargs):
> ui.status("ex1\n")
> return orig(ui, *args, **kwargs)
> extensions.wrapcommand(commands.table, "files", files)
> EOS

$ cat > ex2.py <<EOS
> from sapling import commands, extensions
> def uisetup(ui):
> def files(orig, ui, *args, **kwargs):
> ui.status("ex2\n")
> return orig(ui, *args, **kwargs)
> extensions.wrapcommand(commands.table, "files", files)
> EOS

$ newrepo
$ echo foo > foo
$ hg ci -Aqm foo
$ hg files
foo

$ hg files --config extensions.ex1=~/ex1.py
ex1
foo

$ hg files --config extensions.ex2=~/ex2.py
ex2
foo

$ hg files --config extensions.ex2=~/ex2.py --config extensions.ex1=~/ex1.py
ex2
ex1
foo

$ hg files
foo

0 comments on commit bdf4505

Please sign in to comment.