From 43c4f00a1197467fd57b59d1281bc70672c893f1 Mon Sep 17 00:00:00 2001 From: Matthias Frei Date: Thu, 15 Oct 2020 10:21:26 +0200 Subject: [PATCH] python: fix type annotations (#3888) Various type annotations were broken or half-broken. This prevented even running the scripts under some setups. In particular, the annotations for the ipaddress types used the "construction functions" (e.g. `ip_address`) instead of the type of the returned objects (e.g. `IPv4Address`). Relatedly, simplify the flake8 lint step and configuration and enable style checks for acceptance tests and all python tools. Additionally, fix up the last few remaining imports in order to simplify the PYTHONPATH to only need the base directory, instead of the base directory _and_ python/ -- mypy didn't like this. Finally (and somewhat unrelatedly), revert some recently introduced use of f-strings (new in python-3.6) back to plain %-interpolation. This was breaking compatibility with python-3.5 without real benefit. --- acceptance/flake8.ini => .flake8 | 0 acceptance/cert_renewal_acceptance/test | 6 +++-- acceptance/common/base.py | 4 ++-- acceptance/common/scion.py | 4 ++-- doc/conf.py | 2 +- proto/__init__.py | 0 python/flake8.ini | 2 -- python/topology/common.py | 12 +++++----- python/topology/config.py | 14 ++++++------ python/topology/docker.py | 5 ++--- python/topology/go.py | 11 +++++---- python/topology/net.py | 30 ++++++++++++++++++------- python/topology/prometheus.py | 6 ++--- python/topology/supervisor.py | 6 ++--- scion.sh | 15 +++++-------- tools/package-version | 19 ++++++---------- 16 files changed, 70 insertions(+), 66 deletions(-) rename acceptance/flake8.ini => .flake8 (100%) delete mode 100644 proto/__init__.py delete mode 100644 python/flake8.ini diff --git a/acceptance/flake8.ini b/.flake8 similarity index 100% rename from acceptance/flake8.ini rename to .flake8 diff --git a/acceptance/cert_renewal_acceptance/test b/acceptance/cert_renewal_acceptance/test index f0cf7c0ba4..2a4a6b5c48 100755 --- a/acceptance/cert_renewal_acceptance/test +++ b/acceptance/cert_renewal_acceptance/test @@ -68,7 +68,8 @@ class TestSetup(CmdBase): self.scion.topology('topology/tiny4.topo') logger.info('==> Remove client certificate for 112') - path = local.path('gen/ISD1/ASff00_0_110/cs1-ff00_0_110-1/crypto/ca/clients/ISD1-ASff00_0_112.pem') + path = local.path('gen/ISD1/ASff00_0_110/cs1-ff00_0_110-1/' + 'crypto/ca/clients/ISD1-ASff00_0_112.pem') path.delete() logger.info('==> Start the topology') @@ -81,7 +82,8 @@ class TestSetup(CmdBase): cert = local.path('gen/certs/ISD1-ASff00_0_112.pem') cert.copy(path) - logger.info('==> Sleep thirty seconds to make sure the CS has picked up the client certificate') + logger.info('==> Sleep thirty seconds to make sure the CS ' + 'has picked up the client certificate') time.sleep(30) diff --git a/acceptance/common/base.py b/acceptance/common/base.py index 84a5642fa2..35fe712b79 100644 --- a/acceptance/common/base.py +++ b/acceptance/common/base.py @@ -29,7 +29,7 @@ logger = logging.getLogger(__name__) -def set_name(file: dir): +def set_name(file: str): global NAME global DIR DIR = local.path(file).dirname.name @@ -64,7 +64,7 @@ class TestBase(cli.Application): TestBase is used to implement the test entry point. Tests should sub-class it and only define the doc string. """ - test_state = None + test_state = None # type: TestState @cli.switch('disable-docker', envname='DISABLE_DOCKER', help='Run in supervisor environment.') diff --git a/acceptance/common/scion.py b/acceptance/common/scion.py index a847252aed..8cbed0d144 100644 --- a/acceptance/common/scion.py +++ b/acceptance/common/scion.py @@ -15,7 +15,7 @@ import logging import json from abc import ABC, abstractmethod -from typing import Any, Dict, List +from typing import Any, Dict, List, MutableMapping import toml from plumbum import local @@ -187,7 +187,7 @@ def path_to_dict(path: str, val: Any) -> Dict: return d -def merge_dict(change_dict: Dict[str, Any], orig_dict: Dict[str, Any]): +def merge_dict(change_dict: Dict[str, Any], orig_dict: MutableMapping[str, Any]): """ Merge changes into the original dictionary. Leaf values in the change dict overwrite the values in the original dictionary. diff --git a/doc/conf.py b/doc/conf.py index 1021c15cdf..4469e6dfda 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -14,7 +14,7 @@ # import sys # sys.path.insert(0, os.path.abspath('.')) -import sphinx_rtd_theme +import sphinx_rtd_theme # noqa # -- Project information ----------------------------------------------------- diff --git a/proto/__init__.py b/proto/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/python/flake8.ini b/python/flake8.ini deleted file mode 100644 index 7da1f9608e..0000000000 --- a/python/flake8.ini +++ /dev/null @@ -1,2 +0,0 @@ -[flake8] -max-line-length = 100 diff --git a/python/topology/common.py b/python/topology/common.py index ec6b97e62b..e25e01721c 100644 --- a/python/topology/common.py +++ b/python/topology/common.py @@ -13,15 +13,15 @@ # limitations under the License. # Stdlib -from ipaddress import ip_address, ip_network +from ipaddress import ip_address import os import subprocess from urllib.parse import urlsplit -from typing import Mapping +from typing import Mapping, Tuple # SCION from python.lib.scion_addr import ISD_AS -from python.topology.net import AddressProxy, NetworkDescription +from python.topology.net import AddressProxy, NetworkDescription, IPNetwork COMMON_DIR = 'endhost' @@ -98,7 +98,7 @@ def prom_addr(addr: str, port: int) -> str: return join_host_port(ip, port) -def split_host_port(addr: str) -> (str, int): +def split_host_port(addr: str) -> Tuple[str, int]: parts = urlsplit('//' + addr) if parts.port is None: raise ValueError("missing port in addr: {}".format(addr)) @@ -114,7 +114,7 @@ def join_host_port(host: str, port: int) -> str: return '[{}]:{}'.format(host, port) -def sciond_ip(docker, topo_id, networks: Mapping[ip_network, NetworkDescription]): +def sciond_ip(docker, topo_id, networks: Mapping[IPNetwork, NetworkDescription]): for net_desc in networks.values(): for prog, ip_net in net_desc.ip_net.items(): if prog == 'sd%s' % topo_id.file_fmt(): @@ -123,7 +123,7 @@ def sciond_ip(docker, topo_id, networks: Mapping[ip_network, NetworkDescription] def prom_addr_dispatcher(docker, topo_id, - networks: Mapping[ip_network, NetworkDescription], + networks: Mapping[IPNetwork, NetworkDescription], port, name): if not docker: return "[127.0.0.1]:%s" % port diff --git a/python/topology/config.py b/python/topology/config.py index 3bfcae72b9..ef855081e1 100755 --- a/python/topology/config.py +++ b/python/topology/config.py @@ -23,7 +23,6 @@ import os import sys from io import StringIO -from ipaddress import ip_network from typing import Mapping # SCION @@ -44,6 +43,7 @@ from python.topology.jaeger import JaegerGenArgs, JaegerGenerator from python.topology.net import ( NetworkDescription, + IPNetwork, SubnetGenerator, DEFAULT_NETWORK, ) @@ -184,19 +184,19 @@ def _write_ca_files(self, topo_dicts, ca_files): write_file(os.path.join(base, path), value.decode()) def _write_networks_conf(self, - networks: Mapping[ip_network, NetworkDescription], + networks: Mapping[IPNetwork, NetworkDescription], out_file: str): config = configparser.ConfigParser(interpolation=None) for net, net_desc in networks.items(): sub_conf = {} for prog, ip_net in net_desc.ip_net.items(): - sub_conf[prog] = ip_net.ip - config[net] = sub_conf + sub_conf[prog] = str(ip_net.ip) + config[str(net)] = sub_conf text = StringIO() config.write(text) write_file(os.path.join(self.args.output_dir, out_file), text.getvalue()) - def _write_sciond_conf(self, networks: Mapping[ip_network, NetworkDescription], out_file: str): + def _write_sciond_conf(self, networks: Mapping[IPNetwork, NetworkDescription], out_file: str): d = dict() for net_desc in networks.values(): for prog, ip_net in net_desc.ip_net.items(): @@ -207,8 +207,8 @@ def _write_sciond_conf(self, networks: Mapping[ip_network, NetworkDescription], json.dump(d, f, sort_keys=True, indent=4) -def remove_v4_nets(nets: Mapping[ip_network, NetworkDescription] - ) -> Mapping[ip_network, NetworkDescription]: +def remove_v4_nets(nets: Mapping[IPNetwork, NetworkDescription] + ) -> Mapping[IPNetwork, NetworkDescription]: res = {} for net, net_desc in nets.items(): if net_desc.name.endswith('_v4'): diff --git a/python/topology/docker.py b/python/topology/docker.py index 8ae4cb88e6..a3c22e1c59 100644 --- a/python/topology/docker.py +++ b/python/topology/docker.py @@ -15,7 +15,6 @@ # Stdlib import copy import os -from ipaddress import ip_network from typing import Mapping # External packages import yaml @@ -31,14 +30,14 @@ sciond_svc_name ) from python.topology.docker_utils import DockerUtilsGenArgs, DockerUtilsGenerator -from python.topology.net import NetworkDescription +from python.topology.net import NetworkDescription, IPNetwork from python.topology.sig import SIGGenArgs, SIGGenerator DOCKER_CONF = 'scion-dc.yml' class DockerGenArgs(ArgsTopoDicts): - def __init__(self, args, topo_dicts, networks: Mapping[ip_network, NetworkDescription]): + def __init__(self, args, topo_dicts, networks: Mapping[IPNetwork, NetworkDescription]): """ :param object args: Contains the passed command line arguments as named attributes. :param dict topo_dicts: The generated topo dicts from TopoGenerator. diff --git a/python/topology/go.py b/python/topology/go.py index a62cf60937..ab5d43be94 100755 --- a/python/topology/go.py +++ b/python/topology/go.py @@ -20,7 +20,6 @@ import os import toml import yaml -from ipaddress import ip_network from typing import Mapping # SCION @@ -41,7 +40,7 @@ CO_CONFIG_NAME, ) -from python.topology.net import socket_address_str, NetworkDescription +from python.topology.net import socket_address_str, NetworkDescription, IPNetwork from python.topology.prometheus import ( CS_PROM_PORT, @@ -57,7 +56,7 @@ class GoGenArgs(ArgsTopoDicts): - def __init__(self, args, topo_dicts, networks: Mapping[ip_network, NetworkDescription]): + def __init__(self, args, topo_dicts, networks: Mapping[IPNetwork, NetworkDescription]): super().__init__(args, topo_dicts) self.networks = networks @@ -78,7 +77,7 @@ def generate_br(self): for k, v in topo.get("border_routers", {}).items(): base = topo_id.base_dir(self.args.output_dir) br_conf = self._build_br_conf(topo_id, topo["isd_as"], base, k, v) - write_file(os.path.join(base, f"{k}.toml"), toml.dumps(br_conf)) + write_file(os.path.join(base, "%s.toml" % k), toml.dumps(br_conf)) def _build_br_conf(self, topo_id, ia, base, name, v): config_dir = '/share/conf' if self.args.docker else base @@ -104,7 +103,7 @@ def generate_control_service(self): base = topo_id.base_dir(self.args.output_dir) bs_conf = self._build_control_service_conf( topo_id, topo["isd_as"], base, elem_id, elem, ca) - write_file(os.path.join(base, f"{elem_id}.toml"), + write_file(os.path.join(base, "%s.toml" % elem_id), toml.dumps(bs_conf)) def _build_control_service_conf(self, topo_id, ia, base, name, infra_elem, ca): @@ -278,7 +277,7 @@ def _gen_disp_docker(self): for k in elem_ids: disp_id = 'disp_%s' % k disp_conf = self._build_disp_conf(disp_id, topo_id) - write_file(os.path.join(base, f'{disp_id}.toml'), toml.dumps(disp_conf)) + write_file(os.path.join(base, '%s.toml' % disp_id), toml.dumps(disp_conf)) def _build_disp_conf(self, name, topo_id=None): prometheus_addr = prom_addr_dispatcher(self.args.docker, topo_id, diff --git a/python/topology/net.py b/python/topology/net.py index 26bb6869a0..80dbf2857c 100755 --- a/python/topology/net.py +++ b/python/topology/net.py @@ -21,8 +21,17 @@ import math import sys from collections import defaultdict -from ipaddress import ip_interface, ip_network -from typing import Mapping +from ipaddress import ( + ip_interface, + ip_network, + IPv4Address, + IPv6Address, + IPv4Network, + IPv6Network, + IPv4Interface, + IPv6Interface, +) +from typing import Mapping, Union # External packages import yaml @@ -34,9 +43,13 @@ DEFAULT_PRIV_NETWORK = "192.168.0.0/16" DEFAULT_SCN_DC_NETWORK = "172.20.0.0/20" +IPAddress = Union[IPv4Address, IPv6Address] +IPNetwork = Union[IPv4Network, IPv6Network] +IPInterface = Union[IPv4Interface, IPv6Interface] + class NetworkDescription(object): - def __init__(self, name: str, ip_net: Mapping[str, ip_interface]): + def __init__(self, name: str, ip_net: Mapping[str, IPInterface]): self.name = name self.ip_net = ip_net @@ -68,7 +81,7 @@ def __init__(self, docker): def register(self, id_: str) -> AddressProxy: return self._addrs[id_] - def alloc_addrs(self, subnet) -> Mapping[str, ip_interface]: + def alloc_addrs(self, subnet) -> Mapping[str, IPInterface]: hosts = subnet.hosts() interfaces = {} # With the docker backend, docker itself claims the first ip of every network @@ -97,7 +110,8 @@ def __init__(self, network: str, docker: bool): except ValueError: logging.critical("Invalid network '%s'", network) sys.exit(1) - self._subnets = defaultdict(lambda: AddressGenerator(self.docker)) + self._subnets = defaultdict(lambda: AddressGenerator(self.docker)) \ + # type: Mapping[str, AddressGenerator] self._allocations = defaultdict(list) # Initialise the allocations with the supplied network, making sure to # exclude 127.0.0.0/30 (for v4) and DEFAULT6_NETWORK_ADDR/126 (for v6) @@ -119,7 +133,7 @@ def __init__(self, network: str, docker: bool): def register(self, location: str) -> AddressGenerator: return self._subnets[location] - def alloc_subnets(self) -> Mapping[ip_network, NetworkDescription]: + def alloc_subnets(self) -> Mapping[IPNetwork, NetworkDescription]: max_prefix = self._net.max_prefixlen networks = {} for topo, subnet in sorted(self._subnets.items(), key=lambda x: str(x)): @@ -173,13 +187,13 @@ def register(self, id_: str) -> int: return p -def socket_address_str(ip: str, port: int) -> str: +def socket_address_str(ip: IPAddress, port: int) -> str: if ip.version == 4: return "%s:%d" % (ip, port) return "[%s]:%d" % (ip, port) -def _workaround_ip_network_hosts_py35(net: ip_network) -> ip_network: +def _workaround_ip_network_hosts_py35(net: IPNetwork) -> IPNetwork: """ Returns an _identical_ ipaddress.ip_network for which hosts() which will work as it should. diff --git a/python/topology/prometheus.py b/python/topology/prometheus.py index e87de67b49..af3095cacf 100755 --- a/python/topology/prometheus.py +++ b/python/topology/prometheus.py @@ -19,7 +19,6 @@ # Stdlib import os from collections import defaultdict -from ipaddress import ip_network from typing import Mapping # External packages @@ -35,7 +34,8 @@ sciond_ip, ) from python.topology.net import ( - NetworkDescription + NetworkDescription, + IPNetwork, ) CS_PROM_PORT = 30452 @@ -49,7 +49,7 @@ class PrometheusGenArgs(ArgsTopoDicts): - def __init__(self, args, topo_dicts, networks: Mapping[ip_network, NetworkDescription]): + def __init__(self, args, topo_dicts, networks: Mapping[IPNetwork, NetworkDescription]): super().__init__(args, topo_dicts) self.networks = networks diff --git a/python/topology/supervisor.py b/python/topology/supervisor.py index fd7bb509bc..07eafa9a2c 100755 --- a/python/topology/supervisor.py +++ b/python/topology/supervisor.py @@ -73,7 +73,7 @@ def _as_entries(self, topo_id, topo): def _br_entries(self, topo, cmd, base): entries = [] for k, v in topo.get("border_routers", {}).items(): - conf = os.path.join(base, f"{k}.toml") + conf = os.path.join(base, "%s.toml" % k) prog = self._common_entry(k, [cmd, "--config", conf]) prog['environment'] += ',GODEBUG="cgocheck=0"' entries.append((k, prog)) @@ -84,7 +84,7 @@ def _control_service_entries(self, topo, base): for k, v in topo.get("control_service", {}).items(): # only a single control service instance per AS is currently supported if k.endswith("-1"): - conf = os.path.join(base, f"{k}.toml") + conf = os.path.join(base, "%s.toml" % k) prog = self._common_entry(k, ["bin/cs", "--config", conf]) entries.append((k, prog)) return entries @@ -112,7 +112,7 @@ def _common_entry(self, name, cmd_args): 'autostart': 'false', 'autorestart': 'false', 'environment': 'TZ=UTC', - 'stdout_logfile': f"logs/{name}.log", + 'stdout_logfile': "logs/%s.log" % name, 'redirect_stderr': True, 'startretries': 0, 'startsecs': 5, diff --git a/scion.sh b/scion.sh index a611b684b0..c5a9b1c287 100755 --- a/scion.sh +++ b/scion.sh @@ -1,6 +1,6 @@ #!/bin/bash -export PYTHONPATH=python/:. +export PYTHONPATH=. EXTRA_NOSE_ARGS="-w python/ --with-xunit --xunit-file=logs/nosetests.xml" @@ -279,14 +279,11 @@ cmd_lint() { py_lint() { lint_header "python" - local ret=0 - for i in acceptance python; do - [ -d "$i" ] || continue - local cmd="flake8" - lint_step "$cmd /$i" - ( cd "$i" && $cmd --config flake8.ini . ) | sort -t: -k1,1 -k2n,2 -k3n,3 || ((ret++)) - done - flake8 --config python/flake8.ini tools/gomocks || ((ret++)) + # Run linters on python/ and acceptance/ directories. + # Additionally, find all executable python files without .py extension + local py_executables=$(find -type f -executable -regex '.*/[^.]*$' | xargs file -i | grep 'text/x-python' | cut -d: -f1) + lint_step "flake8" + flake8 python acceptance ${py_executables} | sort -t: -k1,1 -k2n,2 -k3n,3 || ((ret++)) return $ret } diff --git a/tools/package-version b/tools/package-version index 9a86df98ae..eccaf9b963 100755 --- a/tools/package-version +++ b/tools/package-version @@ -1,13 +1,13 @@ #!/usr/bin/env python3 -# This script converts a version from ./tools/git-version to debian and rpm +# This script converts a version from ./tools/git-version to debian and rpm # version files that can be consumed by bazel. # Examples: -# - prefix-v0.8.0-rc1-1-g0aec09a +# - prefix-v0.8.0-rc1-1-g0aec09a # -> scion-utils-deb.version: 0.8.0~rc1~1~g0aec09a # -> scion-utils-rpm.version: 0.8.0 # -> scion-utils-rpm.release: rc1-1-g0aec09a -# - prefix-v0.8.0 +# - prefix-v0.8.0 # -> scion-utils-deb.version: 0.8.0 # -> scion-utils-rpm.version: 0.8.0 # -> scion-utils-rpm.release: 1 @@ -38,15 +38,10 @@ if m is None: version = version[m.start():] deb_version = version.replace('-', '~') parts = version.split('-', 1) -rpm_version, rpm_release = parts[0], 1 +rpm_version, rpm_release = parts[0], '1' if len(parts) > 1: rpm_release = parts[1] -with open(DEB_VERSION, 'w') as f: - f.write('%s' % deb_version) - -with open(RPM_VERSION, 'w') as f: - f.write('%s' % rpm_version) - -with open(RPM_RELEASE, 'w') as f: - f.write('%s' % rpm_release) +Path(DEB_VERSION).write_text(deb_version) +Path(RPM_VERSION).write_text(rpm_version) +Path(RPM_RELEASE).write_text(rpm_release)