Merge pull request #241 from untitaker/click5

Factor out a few click-related things into extensions, new threading model

setup.py

@@ -32,7 +32,9 @@ setup(
     },
     install_requires=[
         # https://github.com/mitsuhiko/click/issues/200
-        'click>=3.1',
+        'click>=5.0',
+        'click-log',
+        'click-threading',
         'requests',
         'lxml>=3.0',
         # https://github.com/sigmavirus24/requests-toolbelt/pull/28

tests/cli/test_main.py

@@ -90,6 +90,7 @@ def test_empty_storage(tmpdir, runner):
 
     tmpdir.join('path_a/haha.txt').write('UID:haha')
     result = runner.invoke(['sync'])
+    assert not result.exception
     tmpdir.join('path_b/haha.txt').remove()
     result = runner.invoke(['sync'])
     lines = result.output.splitlines()

tests/conftest.py

@@ -2,15 +2,14 @@
 '''
 General-purpose fixtures for vdirsyncer's testsuite.
 '''
-import pytest
+import click_log
 
-import vdirsyncer.log
+import pytest
 
 
 @pytest.fixture(autouse=True)
 def setup_logging():
-    vdirsyncer.log.set_level(vdirsyncer.log.logging.DEBUG)
+    click_log.basic_config('vdirsyncer')
-    vdirsyncer.log.add_handler(vdirsyncer.log.stdout_handler)
 
 
 try:

tests/test_doubleclick.py

@@ -1,22 +0,0 @@
-# -*- coding: utf-8 -*-
-
-from click.testing import CliRunner
-
-from vdirsyncer.doubleclick import _ctx_stack, click, ctx as global_ctx
-
-
-def test_simple():
-    @click.command()
-    @click.pass_context
-    def cli(ctx):
-        assert global_ctx
-        assert ctx.obj is global_ctx.obj
-        assert _ctx_stack.top is ctx
-        click.echo('hello')
-
-    assert not global_ctx
-    runner = CliRunner()
-    result = runner.invoke(cli, catch_exceptions=False)
-    assert not global_ctx
-    assert not result.exception
-    assert result.output == 'hello\n'

tests/utils/test_main.py

@@ -1,5 +1,6 @@
 # -*- coding: utf-8 -*-
 
+import logging
 import os
 import platform
 import stat
@@ -7,11 +8,14 @@ import stat
 import click
 from click.testing import CliRunner
 
+import click_log
+
 import pytest
 
 import requests
 
-from vdirsyncer import doubleclick, log, utils
+from vdirsyncer import utils
+from vdirsyncer.cli import pass_context
 
 # These modules might be uninitialized and unavailable if not explicitly
 # imported
@@ -44,11 +48,12 @@ def empty_password_storages(monkeypatch):
 
 @pytest.fixture(autouse=True)
 def no_debug_output(request):
-    old = log._level
+    logger = click_log.basic_config('vdirsyncer')
-    log.set_level(log.logging.WARNING)
+    logger.setLevel(logging.WARNING)
+    old = logger.level
 
     def teardown():
-        log.set_level(old)
+        logger.setLevel(old)
 
     request.addfinalizer(teardown)
 
@@ -111,10 +116,10 @@ def test_get_password_from_command(tmpdir):
     st = os.stat(filepath)
     os.chmod(filepath, st.st_mode | stat.S_IEXEC)
 
-    @doubleclick.click.command()
+    @click.command()
-    @doubleclick.click.pass_context
+    @pass_context
     def fake_app(ctx):
-        ctx.obj = {'config': ({'password_command': filepath}, {}, {})}
+        ctx.config = {'password_command': filepath}, {}, {}
         _password = utils.password.get_password(username, resource)
         assert _password == password
 
@@ -158,10 +163,9 @@ def test_set_keyring_password(monkeypatch):
 
     monkeypatch.setattr(utils.password, 'keyring', KeyringMock())
 
-    @doubleclick.click.command()
+    @click.command()
-    @doubleclick.click.pass_context
+    @pass_context
     def fake_app(ctx):
-        ctx.obj = {}
         x = utils.password.get_password('foouser', 'http://example.com/a/b')
         click.echo('password is ' + x)
 
@@ -179,15 +183,14 @@ def test_get_password_from_cache(monkeypatch):
     user = 'my_user'
     resource = 'http://example.com'
 
-    @doubleclick.click.command()
+    @click.command()
-    @doubleclick.click.pass_context
+    @pass_context
     def fake_app(ctx):
-        ctx.obj = {}
         x = utils.password.get_password(user, resource)
         click.echo('Password is {}'.format(x))
-        monkeypatch.setattr(doubleclick.click, 'prompt', blow_up)
+        monkeypatch.setattr(click, 'prompt', blow_up)
 
-        assert (user, 'example.com') in ctx.obj['passwords']
+        assert (user, 'example.com') in ctx.passwords
         x = utils.password.get_password(user, resource)
         click.echo('Password is {}'.format(x))
 

vdirsyncer/cli/__init__.py

@@ -3,13 +3,26 @@
 import functools
 import sys
 
+import click
+
+import click_log
+
 from .. import __version__, log
-from ..doubleclick import click, ctx
 
 
 cli_logger = log.get(__name__)
 
 
+class AppContext(object):
+    def __init__(self):
+        self.config = None
+        self.passwords = {}
+        self.logger = None
+
+
+pass_context = click.make_pass_decorator(AppContext, ensure=True)
+
+
 class CliError(RuntimeError):
     def __init__(self, msg, problems=None):
         self.msg = msg
@@ -52,32 +65,25 @@ def validate_verbosity(ctx, param, value):
 
 
 @click.group()
-@click.option('--verbosity', '-v', default='INFO',
+@click_log.init('vdirsyncer')
-              callback=validate_verbosity,
+@click_log.simple_verbosity_option()
-              help='Either CRITICAL, ERROR, WARNING, INFO or DEBUG')
 @click.version_option(version=__version__)
+@pass_context
 @catch_errors
-def app(verbosity):
+def app(ctx):
     '''
     vdirsyncer -- synchronize calendars and contacts
     '''
     from .utils import load_config
-    log.add_handler(log.stdout_handler)
-    log.set_level(verbosity)
 
-    if ctx.obj is None:
+    if not ctx.config:
-        ctx.obj = {}
+        ctx.config = load_config()
-
-    ctx.obj['verbosity'] = verbosity
-
-    if 'config' not in ctx.obj:
-        ctx.obj['config'] = load_config()
 
 main = app
 
 
 def max_workers_callback(ctx, param, value):
-    if value == 0 and ctx.obj['verbosity'] == log.logging.DEBUG:
+    if value == 0 and click_log.get_level() == log.logging.DEBUG:
         value = 1
 
     cli_logger.debug('Using {} maximal workers.'.format(value))
@@ -99,8 +105,9 @@ max_workers_option = click.option(
               help=('Do/Don\'t abort synchronization when all items are about '
                     'to be deleted from both sides.'))
 @max_workers_option
+@pass_context
 @catch_errors
-def sync(pairs, force_delete, max_workers):
+def sync(ctx, pairs, force_delete, max_workers):
     '''
     Synchronize the given pairs. If no arguments are given, all will be
     synchronized.
@@ -119,27 +126,27 @@ def sync(pairs, force_delete, max_workers):
     '''
     from .tasks import prepare_pair, sync_collection
     from .utils import parse_pairs_args, WorkerQueue
-    general, all_pairs, all_storages = ctx.obj['config']
+    general, all_pairs, all_storages = ctx.config
 
     wq = WorkerQueue(max_workers)
 
-    for pair_name, collections in parse_pairs_args(pairs, all_pairs):
+    with wq.join():
-        wq.put(functools.partial(prepare_pair, pair_name=pair_name,
+        for pair_name, collections in parse_pairs_args(pairs, all_pairs):
-                                 collections=collections,
+            wq.put(functools.partial(prepare_pair, pair_name=pair_name,
-                                 general=general, all_pairs=all_pairs,
+                                     collections=collections,
-                                 all_storages=all_storages,
+                                     general=general, all_pairs=all_pairs,
-                                 force_delete=force_delete,
+                                     all_storages=all_storages,
-                                 callback=sync_collection))
+                                     force_delete=force_delete,
-        wq.spawn_worker()
+                                     callback=sync_collection))
-
+            wq.spawn_worker()
-    wq.join()
 
 
 @app.command()
 @click.argument('pairs', nargs=-1)
 @max_workers_option
+@pass_context
 @catch_errors
-def metasync(pairs, max_workers):
+def metasync(ctx, pairs, max_workers):
     '''
     Synchronize metadata of the given pairs.
 
@@ -147,58 +154,57 @@ def metasync(pairs, max_workers):
     '''
     from .tasks import prepare_pair, metasync_collection
     from .utils import parse_pairs_args, WorkerQueue
-    general, all_pairs, all_storages = ctx.obj['config']
+    general, all_pairs, all_storages = ctx.config
 
     wq = WorkerQueue(max_workers)
 
-    for pair_name, collections in parse_pairs_args(pairs, all_pairs):
+    with wq.join():
-        wq.put(functools.partial(prepare_pair, pair_name=pair_name,
+        for pair_name, collections in parse_pairs_args(pairs, all_pairs):
-                                 collections=collections,
+            wq.put(functools.partial(prepare_pair, pair_name=pair_name,
-                                 general=general, all_pairs=all_pairs,
+                                     collections=collections,
-                                 all_storages=all_storages,
+                                     general=general, all_pairs=all_pairs,
-                                 callback=metasync_collection))
+                                     all_storages=all_storages,
-        wq.spawn_worker()
+                                     callback=metasync_collection))
-
+            wq.spawn_worker()
-    wq.join()
 
 
 @app.command()
 @click.argument('pairs', nargs=-1)
 @max_workers_option
+@pass_context
 @catch_errors
-def discover(pairs, max_workers):
+def discover(ctx, pairs, max_workers):
     '''
     Refresh collection cache for the given pairs.
     '''
     from .tasks import discover_collections
     from .utils import WorkerQueue
-    general, all_pairs, all_storages = ctx.obj['config']
+    general, all_pairs, all_storages = ctx.config
     wq = WorkerQueue(max_workers)
 
-    for pair in (pairs or all_pairs):
+    with wq.join():
-        try:
+        for pair in (pairs or all_pairs):
-            name_a, name_b, pair_options = all_pairs[pair]
+            try:
-        except KeyError:
+                name_a, name_b, pair_options = all_pairs[pair]
-            raise CliError('Pair not found: {}\n'
+            except KeyError:
-                           'These are the pairs found: {}'
+                raise CliError('Pair not found: {}\n'
-                           .format(pair, list(all_pairs)))
+                               'These are the pairs found: {}'
+                               .format(pair, list(all_pairs)))
 
-        wq.put(functools.partial(
+            wq.put(functools.partial(
-            discover_collections,
+                discover_collections, status_path=general['status_path'],
-            status_path=general['status_path'], name_a=name_a, name_b=name_b,
+                name_a=name_a, name_b=name_b, pair_name=pair,
-            pair_name=pair, config_a=all_storages[name_a],
+                config_a=all_storages[name_a], config_b=all_storages[name_b],
-            config_b=all_storages[name_b], pair_options=pair_options,
+                pair_options=pair_options, skip_cache=True
-            skip_cache=True
+            ))
-        ))
+            wq.spawn_worker()
-        wq.spawn_worker()
-
-    wq.join()
 
 
 @app.command()
 @click.argument('collection')
+@pass_context
 @catch_errors
-def repair(collection):
+def repair(ctx, collection):
     '''
     Repair a given collection.
 
@@ -211,7 +217,7 @@ def repair(collection):
     collection of the `calendars_local` storage.
     '''
     from .tasks import repair_collection
-    general, all_pairs, all_storages = ctx.obj['config']
+    general, all_pairs, all_storages = ctx.config
 
     cli_logger.warning('This operation will take a very long time.')
     cli_logger.warning('It\'s recommended to turn off other client\'s '

vdirsyncer/cli/utils.py

@@ -1,5 +1,6 @@
 # -*- coding: utf-8 -*-
 
+import contextlib
 import errno
 import hashlib
 import importlib
@@ -7,14 +8,16 @@ import json
 import os
 import string
 import sys
-import threading
 from itertools import chain
 
 from atomicwrites import atomic_write
 
+import click
+
+import click_threading
+
 from . import CliError, cli_logger
 from .. import DOCS_HOME, PROJECT_HOME, exceptions
-from ..doubleclick import click
 from ..sync import IdentConflict, StorageEmpty, SyncConflict
 from ..utils import expand_path, get_class_init_args
 from ..utils.compat import text_type
@@ -560,46 +563,52 @@ class WorkerQueue(object):
         self._workers = []
         self._exceptions = []
         self._max_workers = max_workers
+        self._shutdown_handlers = []
+
+    def shutdown(self):
+        if not self._queue.unfinished_tasks:
+            for handler in self._shutdown_handlers:
+                try:
+                    handler()
+                except Exception:
+                    pass
 
     def _worker(self):
-        # This is a daemon thread. Since the global namespace is going to
-        # vanish on interpreter shutdown, redefine everything from the global
-        # namespace here.
-        _TypeError = TypeError
-        _Empty = queue.Empty
-        _handle_cli_error = handle_cli_error
-
         while True:
             try:
                 func = self._queue.get(False)
-            except (_TypeError, _Empty):
+            except queue.Empty:
-                # Any kind of error might be raised if vdirsyncer just finished
-                # processing all items and the interpreter is shutting down,
-                # yet the workers try to get new tasks.
-                # https://github.com/untitaker/vdirsyncer/issues/167
-                # http://bugs.python.org/issue14623
                 break
 
             try:
                 func(wq=self)
             except Exception as e:
-                _handle_cli_error()
+                handle_cli_error()
                 self._exceptions.append(e)
             finally:
                 self._queue.task_done()
+                if not self._queue.unfinished_tasks:
+                    self.shutdown()
 
     def spawn_worker(self):
         if self._max_workers and len(self._workers) >= self._max_workers:
             return
 
-        t = threading.Thread(target=self._worker)
+        t = click_threading.Thread(target=self._worker)
         t.daemon = True
         t.start()
         self._workers.append(t)
 
+    @contextlib.contextmanager
     def join(self):
-        assert self._workers or self._queue.empty()
+        assert self._workers or not self._queue.unfinished_tasks
-        self._queue.join()
+        ui_worker = click_threading.UiWorker()
+        self._shutdown_handlers.append(ui_worker.shutdown)
+        with ui_worker.patch_click():
+            yield
+            ui_worker.run()
+            self._queue.join()
+
         if self._exceptions:
             sys.exit(1)
 

vdirsyncer/doubleclick.py

@@ -1,144 +0,0 @@
-# -*- coding: utf-8 -*-
-'''
-Utilities for writing threaded applications with click:
-
-- There is a global ``ctx`` object to be used.
-
-- The ``click`` object's attributes are supposed to be used instead of the
-  click package's content.
-
-  - It wraps some UI functions such that they don't produce overlapping
-    output or prompt the user at the same time.
-
-  - It wraps BaseCommand subclasses such that their invocation changes the
-    ctx global, and also changes the shortcut decorators to use the new
-    classes.
-'''
-
-import functools
-import threading
-
-
-class _ClickProxy(object):
-    def __init__(self, wrappers, click=None):
-        if click is None:
-            import click
-        self._click = click
-        self._cache = {}
-        self._wrappers = dict(wrappers)
-
-    def __getattr__(self, name):
-        if name not in self._cache:
-            f = getattr(self._click, name)
-            f = self._wrappers.get(name, lambda x: x)(f)
-            self._cache[name] = f
-
-        return self._cache[name]
-
-
-_ui_lock = threading.Lock()
-
-
-def _ui_function(f):
-    @functools.wraps(f)
-    def inner(*a, **kw):
-        with _ui_lock:
-            rv = f(*a, **kw)
-        return rv
-    return inner
-
-
-class _Stack(object):
-    def __init__(self):
-        self._stack = []
-
-    @property
-    def top(self):
-        return self._stack[-1]
-
-    def push(self, value):
-        self._stack.append(value)
-
-    def pop(self):
-        return self._stack.pop()
-
-
-class _StackProxy(object):
-    def __init__(self, stack):
-        object.__setattr__(self, '_doubleclick_stack', stack)
-
-    def __bool__(self):
-        try:
-            self._doubleclick_stack.top
-        except IndexError:
-            return False
-        else:
-            return True
-
-    __nonzero__ = __bool__
-
-    __getattr__ = lambda s, n: getattr(s._doubleclick_stack.top, n)
-    __setattr__ = lambda s, n, v: setattr(s._doubleclick_stack.top, n, v)
-
-
-_ctx_stack = _Stack()
-ctx = _StackProxy(_ctx_stack)
-
-
-def _ctx_pushing_class(cls):
-    class ContextPusher(cls):
-        def command(self, *args, **kwargs):
-            # Also wrap commands created with @group.command()
-            def decorator(f):
-                cmd = click.command(*args, **kwargs)(f)
-                self.add_command(cmd)
-                return cmd
-            return decorator
-
-        def invoke(self, ctx):
-            _ctx_stack.push(ctx)
-            try:
-                cls.invoke(self, ctx)
-            finally:
-                new_ctx = _ctx_stack.pop()
-                if new_ctx is not ctx:
-                    raise RuntimeError(
-                        'While doubleclick is supposed to make writing '
-                        'threaded applications easier, it removes thread '
-                        'safety from click. It is therefore not recommended '
-                        'to launch more than one doubleclick application per '
-                        'process.'
-                    )
-
-    return ContextPusher
-
-
-def _command_class_wrapper(cls_name):
-    def inner(f):
-        def wrapper(name=None, **attrs):
-            attrs.setdefault('cls', getattr(click, cls_name))
-            return f(name, **attrs)
-        return wrapper
-    return inner
-
-
-WRAPPERS = {
-    'echo': _ui_function,
-    'echo_via_pager': _ui_function,
-    'prompt': _ui_function,
-    'confirm': _ui_function,
-    'clear': _ui_function,
-    'edit': _ui_function,
-    'launch': _ui_function,
-    'getchar': _ui_function,
-    'pause': _ui_function,
-    'BaseCommand': _ctx_pushing_class,
-    'Command': _ctx_pushing_class,
-    'MultiCommand': _ctx_pushing_class,
-    'Group': _ctx_pushing_class,
-    'CommandCollection': _ctx_pushing_class,
-    'command': _command_class_wrapper('Command'),
-    'group': _command_class_wrapper('Group')
-}
-
-click = _ClickProxy(WRAPPERS)

vdirsyncer/log.py

@@ -1,63 +1,5 @@
 # -*- coding: utf-8 -*-
 
 import logging
-import sys
 
-from .doubleclick import click
+get = logging.getLogger
-
-
-class ColorFormatter(logging.Formatter):
-    colors = {
-        'error': dict(fg='red'),
-        'exception': dict(fg='red'),
-        'critical': dict(fg='red'),
-        'debug': dict(fg='blue'),
-        'warning': dict(fg='yellow')
-    }
-
-    def format(self, record):
-        if not record.exc_info:
-            level = record.levelname.lower()
-            if level in self.colors:
-                prefix = click.style('{}: '.format(level),
-                                     **self.colors[level])
-                record.msg = '\n'.join(prefix + x
-                                       for x in str(record.msg).splitlines())
-
-        return logging.Formatter.format(self, record)
-
-
-class ClickStream(object):
-    def write(self, string):
-        click.echo(string, file=sys.stderr, nl=False)
-
-
-stdout_handler = logging.StreamHandler(ClickStream())
-stdout_handler.formatter = ColorFormatter()
-
-_level = logging.INFO
-_handlers = []
-
-_loggers = {}
-
-
-def get(name):
-    assert name.startswith('vdirsyncer.')
-    if name not in _loggers:
-        _loggers[name] = x = logging.getLogger(name)
-        x.handlers = _handlers
-        x.setLevel(_level)
-
-    return _loggers[name]
-
-
-def add_handler(handler):
-    if handler not in _handlers:
-        _handlers.append(handler)
-
-
-def set_level(level):
-    global _level
-    _level = level
-    for logger in _loggers.values():
-        logger.setLevel(_level)

vdirsyncer/utils/password.py

@@ -2,10 +2,12 @@
 
 import threading
 
+import click
+
 from . import expand_path
 from .compat import urlparse
 from .. import exceptions, log
-from ..doubleclick import click, ctx
+from ..cli import AppContext
 
 logger = log.get(__name__)
 password_key_prefix = 'vdirsyncer:'
@@ -40,28 +42,40 @@ def get_password(username, resource, _lock=threading.Lock()):
 
 
     """
-    if ctx:
+    # If no app is running, Click will automatically create an empty cache for
-        password_cache = ctx.obj.setdefault('passwords', {})
+    # us and discard it.
-    else:
+    try:
-        password_cache = {}  # discard passwords
+        ctx = click.get_current_context().find_object(AppContext)
+        if ctx is None:
+            raise RuntimeError()
+        password_cache = ctx.passwords
+    except RuntimeError:
+        password_cache = {}
 
     def _password_from_cache(username, host):
         '''internal cache'''
-        return password_cache.get((username, host), None)
+        rv = password_cache.get((username, host), None)
+        if isinstance(rv, BaseException):
+            raise rv
+        return rv
 
     with _lock:
-        host = urlparse.urlsplit(resource).hostname
+        try:
-        for func in (_password_from_cache, _password_from_command,
+            host = urlparse.urlsplit(resource).hostname
-                     _password_from_netrc, _password_from_keyring,
+            for func in (_password_from_cache, _password_from_command,
-                     _password_from_prompt):
+                         _password_from_netrc, _password_from_keyring,
-            password = func(username, host)
+                         _password_from_prompt):
-            if password is not None:
+                password = func(username, host)
-                logger.debug('Got password for {} from {}'
+                if password is not None:
-                             .format(username, func.__doc__))
+                    logger.debug('Got password for {} from {}'
-                break
+                                 .format(username, func.__doc__))
-
+                    break
-        password_cache[(username, host)] = password
+        except (click.Abort, KeyboardInterrupt) as e:
-        return password
+            password_cache[(username, host)] = e
+            raise
+        else:
+            password_cache[(username, host)] = password
+            return password
 
 
 def _password_from_prompt(username, host):
@@ -101,11 +115,17 @@ def _password_from_command(username, host):
     '''command'''
     import subprocess
 
-    if not ctx:
+    try:
+        ctx = click.get_current_context()
+    except RuntimeError:
+        return None
+
+    ctx = ctx.find_object(AppContext)
+    if ctx is None or not ctx.config:
         return None
 
     try:
-        general, _, _ = ctx.obj['config']
+        general, _, _ = ctx.config
         command = general['password_command'].split()
     except KeyError:
         return None