From c459d32a1942b0003a9077ac87330cde835def46 Mon Sep 17 00:00:00 2001 From: Guillaume Ayoub Date: Wed, 12 Oct 2016 14:30:18 +0200 Subject: [PATCH] Use argparse to parse command arguments This commit also allows users to specify all the config values through the command line. Fix #154. --- config | 13 +++- radicale/__init__.py | 2 +- radicale/__main__.py | 112 +++++++++++------------------ radicale/config.py | 139 +++++++++++++++++++++++++++--------- radicale/storage.py | 15 ++-- radicale/tests/test_base.py | 19 ++--- 6 files changed, 176 insertions(+), 124 deletions(-) diff --git a/config b/config index 1cdfb75..feb732b 100644 --- a/config +++ b/config @@ -45,7 +45,7 @@ # SSL Protocol used. See python's ssl module for available values #protocol = PROTOCOL_SSLv23 -# Ciphers available. See python's ssl module for available ciphers +# Available ciphers. See python's ssl module for available ciphers #ciphers = # Reverse DNS to resolve client address in logs @@ -106,11 +106,16 @@ # Sync all changes to disk during requests. (This can impair performance.) # Disabling it increases the risk of data loss, when the system crashes or # power fails! -#fsync = True +#filesystem_fsync = True + +# Close the lock file when no more clients are waiting. +# This option is not very useful in general, but on Windows files that are +# opened cannot be deleted. +#filesystem_close_lock_file = False # Command that is run after changes to storage -#hook = # Example: git add -A && (git diff --cached --quiet || git commit -m "Changes by "%(user)s) +#hook = [logging] @@ -120,8 +125,10 @@ # For more information about the syntax of the configuration file, see: # http://docs.python.org/library/logging.config.html #config = /etc/radicale/logging + # Set the default logging level to debug #debug = False + # Store all environment variables (including those set in the shell) #full_environment = False diff --git a/radicale/__init__.py b/radicale/__init__.py index c5266e8..b672db0 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -389,7 +389,7 @@ class Application: status = client.UNAUTHORIZED realm = self.configuration.get("server", "realm") headers = dict(headers) - headers.update ({ + headers.update({ "WWW-Authenticate": "Basic realm=\"%s\"" % realm}) diff --git a/radicale/__main__.py b/radicale/__main__.py index 3db7420..6665f79 100644 --- a/radicale/__main__.py +++ b/radicale/__main__.py @@ -22,8 +22,8 @@ from a python programme with ``radicale.__main__.run()``. """ +import argparse import atexit -import optparse import os import select import signal @@ -36,74 +36,47 @@ from . import ( VERSION, Application, RequestHandler, ThreadedHTTPServer, ThreadedHTTPSServer, config, log) -opt_dict = { - '--server-daemon': { - 'help': 'launch as daemon', - 'aliases': ['-d', '--daemon']}, - '--server-pid': { - 'help': 'set PID filename for daemon mode', - 'aliases': ['-p', '--pid']}, - '--server-hosts': { - 'help': 'set server hostnames and ports', - 'aliases': ['-H', '--hosts'], - }, - '--server-ssl': { - 'help': 'use SSL connection', - 'aliases': ['-s', '--ssl'], - }, - '--server-key': { - 'help': 'set private key file', - 'aliases': ['-k', '--key'] - }, - '--server-certificate': { - 'help': 'set certificate file', - 'aliases': ['-c', '--certificate'] - }, - '--logging-debug': { - 'help': 'print debug informations', - 'aliases': ['-D', '--debug'] - } -} - def run(): """Run Radicale as a standalone server.""" - # Get command-line options - parser = optparse.OptionParser(version=VERSION) + # Get command-line arguments + parser = argparse.ArgumentParser(usage="radicale [OPTIONS]") + + parser.add_argument("--version", action="version", version=VERSION) + parser.add_argument( + "-C", "--config", help="use a specific configuration file") + + groups = {} for section, values in config.INITIAL_CONFIG.items(): - group = optparse.OptionGroup(parser, section) - for option, default_value in values.items(): - long_name = '--{0}-{1}'.format( - section, option.replace('_', '-')) - kwargs = {} - args = [long_name] - if default_value.lower() in ('true', 'false'): - kwargs['action'] = 'store_true' - if long_name in opt_dict: - args.extend(opt_dict[long_name].get('aliases')) - opt_dict[long_name].pop('aliases') - kwargs.update(opt_dict[long_name]) - group.add_option(*args, **kwargs) - if section == 'server': - group.add_option( - "-f", "--foreground", action="store_false", - dest="server_daemon", - help="launch in foreground (opposite of --daemon)") - group.add_option( - "-S", "--no-ssl", action="store_false", dest="server_ssl", - help="do not use SSL connection (opposite of --ssl)") + group = parser.add_argument_group(section) + groups[group] = [] + for option, data in values.items(): + kwargs = data.copy() + long_name = "--{0}-{1}".format( + section, option.replace("_", "-")) + args = kwargs.pop("aliases", []) + args.append(long_name) + kwargs["dest"] = "{0}_{1}".format(section, option) + groups[group].append(kwargs["dest"]) - parser.add_option_group(group) + if kwargs.pop("value") in ("True", "False"): + kwargs["action"] = "store_const" + kwargs["const"] = "True" + opposite_args = kwargs.pop("opposite", []) + opposite_args.append("--no{0}".format(long_name[1:])) + group.add_argument(*args, **kwargs) - parser.add_option( - "-C", "--config", - help="use a specific configuration file") + kwargs["const"] = "False" + kwargs["help"] = "do not {0} (opposite of {1})".format( + kwargs["help"], long_name) + group.add_argument(*opposite_args, **kwargs) + else: + group.add_argument(*args, **kwargs) - options = parser.parse_args()[0] - - if options.config: + args = parser.parse_args() + if args.config: configuration = config.load() - configuration_found = configuration.read(options.config) + configuration_found = configuration.read(args.config) else: configuration_paths = [ "/etc/radicale/config", @@ -113,16 +86,13 @@ def run(): configuration = config.load(configuration_paths) configuration_found = True - # Update Radicale configuration according to options - for group in parser.option_groups: + # Update Radicale configuration according to arguments + for group, actions in groups.items(): section = group.title - for option in group.option_list: - key = option.dest - config_key = key.split('_', 1)[1] - if key: - value = getattr(options, key) - if value is not None: - configuration.set(section, config_key, str(value)) + for action in actions: + value = getattr(args, action) + if value is not None: + configuration.set(section, action.split('_', 1)[1], value) # Start logging filename = os.path.expanduser(configuration.get("logging", "config")) @@ -131,7 +101,7 @@ def run(): # Log a warning if the configuration file of the command line is not found if not configuration_found: - logger.warning("Configuration file '%s' not found" % options.config) + logger.warning("Configuration file '%s' not found" % args.config) try: serve(configuration, logger) diff --git a/radicale/config.py b/radicale/config.py index ed551e3..654737f 100644 --- a/radicale/config.py +++ b/radicale/config.py @@ -30,51 +30,122 @@ from configparser import RawConfigParser as ConfigParser # Default configuration INITIAL_CONFIG = OrderedDict([ ("server", OrderedDict([ - ("hosts", "0.0.0.0:5232"), - ("daemon", "False"), - ("pid", ""), - ("max_connections", "20"), - ("max_content_length", "10000000"), - ("timeout", "10"), - ("ssl", "False"), - ("certificate", "/etc/apache2/ssl/server.crt"), - ("key", "/etc/apache2/ssl/server.key"), - ("protocol", "PROTOCOL_SSLv23"), - ("ciphers", ""), - ("dns_lookup", "True"), - ("base_prefix", "/"), - ("can_skip_base_prefix", "False"), - ("realm", "Radicale - Password Required")])), + ("hosts", { + "value": "0.0.0.0:5232", + "help": "set server hostnames including ports", + "aliases": ["-H", "--hosts"]}), + ("daemon", { + "value": "False", + "help": "launch as daemon", + "aliases": ["-d", "--daemon"], + "opposite": ["-f", "--foreground"]}), + ("pid", { + "value": "", + "help": "set PID filename for daemon mode", + "aliases": ["-p", "--pid"]}), + ("max_connections", { + "value": "20", + "help": "maximum number of parallel connections"}), + ("max_content_length", { + "value": "10000000", + "help": "maximum size of request body in bytes"}), + ("timeout", { + "value": "10", + "help": "socket timeout"}), + ("ssl", { + "value": "False", + "help": "use SSL connection", + "aliases": ["-s", "--ssl"], + "opposite": ["-S", "--no-ssl"]}), + ("certificate", { + "value": "/etc/apache2/ssl/server.crt", + "help": "set certificate file", + "aliases": ["-c", "--certificate"]}), + ("key", { + "value": "/etc/apache2/ssl/server.key", + "help": "set private key file", + "aliases": ["-k", "--key"]}), + ("protocol", { + "value": "PROTOCOL_SSLv23", + "help": "SSL protocol used"}), + ("ciphers", { + "value": "", + "help": "available ciphers"}), + ("dns_lookup", { + "value": "True", + "help": "use reverse DNS to resolve client address in logs"}), + ("base_prefix", { + "value": "/", + "help": "root URL of Radicale, starting and ending with a slash"}), + ("can_skip_base_prefix", { + "value": "False", + "help": "allow URLs cleaned by a HTTP server"}), + ("realm", { + "value": "Radicale - Password Required", + "help": "message displayed when a password is needed"})])), ("encoding", OrderedDict([ - ("request", "utf-8"), - ("stock", "utf-8")])), + ("request", { + "value": "utf-8", + "help": "encoding for responding requests"}), + ("stock", { + "value": "utf-8", + "help": "encoding for storing local collections"})])), ("auth", OrderedDict([ - ("type", "None"), - ("htpasswd_filename", "/etc/radicale/users"), - ("htpasswd_encryption", "crypt")])), + ("type", { + "value": "None", + "help": "authentication method"}), + ("htpasswd_filename", { + "value": "/etc/radicale/users", + "help": "htpasswd filename"}), + ("htpasswd_encryption", { + "value": "crypt", + "help": "htpasswd encryption method"})])), ("rights", OrderedDict([ - ("type", "None"), - ("file", "~/.config/radicale/rights")])), + ("type", { + "value": "None", + "help": "rights backend"}), + ("file", { + "value": "~/.config/radicale/rights", + "help": "file for rights management from_file"})])), ("storage", OrderedDict([ - ("type", "multifilesystem"), - ("filesystem_folder", os.path.expanduser( - "~/.config/radicale/collections")), - ("fsync", "True"), - ("hook", ""), - ("close_lock_file", "False")])), + ("type", { + "value": "multifilesystem", + "help": "storage backend"}), + ("filesystem_folder", { + "value": os.path.expanduser( + "~/.config/radicale/collections"), + "help": "file for rights management from_file"}), + ("filesystem_fsync", { + "value": "True", + "help": "sync all changes to filesystem during requests"}), + ("filesystem_close_lock_file", { + "value": "False", + "help": "close the lock file when no more clients are waiting"}), + ("hook", { + "value": "", + "help": "command that is run after changes to storage"})])), ("logging", OrderedDict([ - ("config", "/etc/radicale/logging"), - ("debug", "False"), - ("full_environment", "False"), - ("mask_passwords", "True")]))]) + ("config", { + "value": "/etc/radicale/logging", + "help": "logging configuration file"}), + ("debug", { + "value": "False", + "help": "print debug information", + "aliases": ["-D", "--debug"]}), + ("full_environment", { + "value": "False", + "help": "store all environment variables"}), + ("mask_passwords", { + "value": "True", + "help": "mask passwords in logs"})]))]) def load(paths=(), extra_config=None): config = ConfigParser() for section, values in INITIAL_CONFIG.items(): config.add_section(section) - for key, value in values.items(): - config.set(section, key, value) + for key, data in values.items(): + config.set(section, key, data["value"]) if extra_config: for section, values in extra_config.items(): for key, value in values.items(): diff --git a/radicale/storage.py b/radicale/storage.py index a67fbf9..ff28f93 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -389,7 +389,7 @@ class Collection(BaseCollection): delete=False, prefix=".Radicale.tmp-", newline=newline) try: yield tmp - if self.configuration.getboolean("storage", "fsync"): + if self.configuration.getboolean("storage", "filesystem_fsync"): if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"): fcntl.fcntl(tmp.fileno(), fcntl.F_FULLFSYNC) else: @@ -418,7 +418,7 @@ class Collection(BaseCollection): This only works on POSIX and does nothing on other systems. """ - if not cls.configuration.getboolean("storage", "fsync"): + if not cls.configuration.getboolean("storage", "filesystem_fsync"): return if os.name == "posix": fd = os.open(path, 0) @@ -550,13 +550,15 @@ class Collection(BaseCollection): new_collection = vobject.iCalendar() for item in items: new_collection.add(item) - href = self._find_available_file_name(vobject_items.get) + href = self._find_available_file_name( + vobject_items.get) vobject_items[href] = new_collection self.upload_all_nonatomic(vobject_items) elif props.get("tag") == "VCARD": vobject_items = {} for card in collection: - href = self._find_available_file_name(vobject_items.get) + href = self._find_available_file_name( + vobject_items.get) vobject_items[href] = card self.upload_all_nonatomic(vobject_items) @@ -583,7 +585,7 @@ class Collection(BaseCollection): fs.append(open(path, "w", encoding=self.encoding, newline="")) fs[-1].write(item.serialize()) fsync_fn = lambda fd: None - if self.configuration.getboolean("storage", "fsync"): + if self.configuration.getboolean("storage", "filesystem_fsync"): if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"): fsync_fn = lambda fd: fcntl.fcntl(fd, fcntl.F_FULLFSYNC) else: @@ -811,7 +813,8 @@ class Collection(BaseCollection): cls._lock_file_locked = False if cls._waiters: cls._waiters[0].notify() - if (cls.configuration.getboolean("storage", "close_lock_file") + if (cls.configuration.getboolean( + "storage", "filesystem_close_lock_file") and cls._readers == 0 and not cls._waiters): cls._lock_file.close() cls._lock_file = None diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 0b604cf..31e5e7b 100644 --- a/radicale/tests/test_base.py +++ b/radicale/tests/test_base.py @@ -792,15 +792,15 @@ class BaseRequestsMixIn: def test_fsync(self): """Create a directory and file with syncing enabled.""" - self.configuration.set("storage", "fsync", "True") + self.configuration.set("storage", "filesystem_fsync", "True") status, headers, answer = self.request("MKCALENDAR", "/calendar.ics/") assert status == 201 def test_hook(self): """Run hook.""" self.configuration.set( - "storage", "hook", "mkdir %s" % os.path.join("collection-root", - "created_by_hook")) + "storage", "hook", "mkdir %s" % os.path.join( + "collection-root", "created_by_hook")) status, headers, answer = self.request("MKCOL", "/calendar.ics/") assert status == 201 status, headers, answer = self.request("PROPFIND", "/created_by_hook/") @@ -809,8 +809,8 @@ class BaseRequestsMixIn: def test_hook_read_access(self): """Verify that hook is not run for read accesses.""" self.configuration.set( - "storage", "hook", "mkdir %s" % os.path.join("collection-root", - "created_by_hook")) + "storage", "hook", "mkdir %s" % os.path.join( + "collection-root", "created_by_hook")) status, headers, answer = self.request("GET", "/") assert status == 200 status, headers, answer = self.request("GET", "/created_by_hook/") @@ -828,8 +828,8 @@ class BaseRequestsMixIn: def test_hook_principal_collection_creation(self): """Verify that the hooks runs when a new user is created.""" self.configuration.set( - "storage", "hook", "mkdir %s" % os.path.join("collection-root", - "created_by_hook")) + "storage", "hook", "mkdir %s" % os.path.join( + "collection-root", "created_by_hook")) status, headers, answer = self.request("GET", "/", REMOTE_USER="user") assert status == 200 status, headers, answer = self.request("PROPFIND", "/created_by_hook/") @@ -852,7 +852,8 @@ class BaseRequestsMixIn: status, headers, answer = self.request("GET", "/") assert headers.get("test") == "123" # Test if header is set on failure - status, headers, answer = self.request("GET", "/.well-known/does not exist") + status, headers, answer = self.request( + "GET", "/.well-known/does not exist") assert headers.get("test") == "123" @@ -867,7 +868,7 @@ class BaseFileSystemTest(BaseTest): self.colpath = tempfile.mkdtemp() self.configuration.set("storage", "filesystem_folder", self.colpath) # Disable syncing to disk for better performance - self.configuration.set("storage", "fsync", "False") + self.configuration.set("storage", "filesystem_fsync", "False") # Required on Windows, doesn't matter on Unix self.configuration.set("storage", "close_lock_file", "True") self.application = Application(self.configuration, self.logger)