From 224085c1394e01b2164ec6eee73584c73669d223 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 23 Nov 2020 14:14:52 -0500 Subject: [PATCH] Clean up version checks and fix the PyPy regression --- src/allmydata/node.py | 2 +- src/allmydata/test/test_version.py | 133 ++++++++++++++++--- src/allmydata/version_checks.py | 203 +++++++++++++++++++++++------ 3 files changed, 282 insertions(+), 56 deletions(-) diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 7051f6fba..272cefae5 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -83,7 +83,7 @@ def _common_valid_config(): for thing, things_version in list(get_package_versions().items()): app_versions.add_version( ensure_str(thing), - ensure_str(things_version) if things_version is not None else None, + None if things_version is None else ensure_str(things_version), ) # group 1 will be addr (dotted quad string), group 3 if any will be portnum (string) diff --git a/src/allmydata/test/test_version.py b/src/allmydata/test/test_version.py index f5f92ef9b..9a3d3fc5b 100644 --- a/src/allmydata/test/test_version.py +++ b/src/allmydata/test/test_version.py @@ -23,6 +23,10 @@ from allmydata.version_checks import ( _cross_check as cross_check, _extract_openssl_version as extract_openssl_version, _get_package_versions_and_locations as get_package_versions_and_locations, + _vers_and_locs_list, + get_package_versions, + get_package_versions_string, + _Dependency, ) from allmydata.util.verlib import NormalizedVersion as V, \ IrrationalVersionError, \ @@ -60,28 +64,63 @@ class CheckRequirement(unittest.TestCase): self.patch(pkg_resources, 'require', call_pkg_resources_require) (packages, errors) = get_package_versions_and_locations() - self.failUnlessIn(("foo", ("1.0", "/path", "according to pkg_resources")), packages) + self.assertIn( + _Dependency("foo", "1.0", "/path", "according to pkg_resources"), + packages, + ) self.failIfEqual(errors, []) self.failUnlessEqual([e for e in errors if "was not found by pkg_resources" not in e], []) + def test_cross_check_return_type(self): + """ + ``cross_check`` returns a ``list`` of ``str``. + """ + self._cross_check_return_type( + {"distribute": ("unparseable", "path")}, + [_Dependency("setuptools", "1.0", "path", None)], + ) + self._cross_check_return_type( + {}, + [_Dependency("foo", "1.0", "path", None)], + ) + self._cross_check_return_type( + {}, + [_Dependency("foo", "1.0", "path", None)], + ) + self._cross_check_return_type( + {"foo": ("unparseable", "path")}, + [_Dependency("foo", None, None, None)], + ) + self._cross_check_return_type( + {"foo": ("1.2.3", "path")}, + [_Dependency("foo", "unknown", None, None)], + ) + + def _cross_check_return_type(self, vers_and_locs, imported_vers_and_locs): + res = cross_check(vers_and_locs, imported_vers_and_locs) + self.assertIsInstance(res, list) + self.assertTrue(len(res) > 0) + for v in res: + self.assertIsInstance(v, str) + def test_cross_check_unparseable_versions(self): # The bug in #1355 is triggered when a version string from either pkg_resources or import # is not parseable at all by normalized_version. - res = cross_check({"foo": ("unparseable", "")}, [("foo", ("1.0", "", None))]) + res = cross_check({"foo": ("unparseable", "")}, [_Dependency("foo", "1.0", "", None)]) self.failUnlessEqual(res, []) - res = cross_check({"foo": ("1.0", "")}, [("foo", ("unparseable", "", None))]) + res = cross_check({"foo": ("1.0", "")}, [_Dependency("foo", "unparseable", "", None)]) self.failUnlessEqual(res, []) - res = cross_check({"foo": ("unparseable", "")}, [("foo", ("unparseable", "", None))]) + res = cross_check({"foo": ("unparseable", "")}, [_Dependency("foo", "unparseable", "", None)]) self.failUnlessEqual(res, []) def test_cross_check(self): res = cross_check({}, []) self.failUnlessEqual(res, []) - res = cross_check({}, [("tahoe-lafs", ("1.0", "", "blah"))]) + res = cross_check({}, [_Dependency("tahoe-lafs", "1.0", "", "blah")]) self.failUnlessEqual(res, []) res = cross_check({"foo": ("unparseable", "")}, []) @@ -90,48 +129,48 @@ class CheckRequirement(unittest.TestCase): res = cross_check({"argparse": ("unparseable", "")}, []) self.failUnlessEqual(res, []) - res = cross_check({}, [("foo", ("unparseable", "", None))]) + res = cross_check({}, [_Dependency("foo", "unparseable", "", None)]) self.failUnlessEqual(len(res), 1) self.assertTrue(("version 'unparseable'" in res[0]) or ("version u'unparseable'" in res[0])) self.failUnlessIn("was not found by pkg_resources", res[0]) - res = cross_check({"distribute": ("1.0", "/somewhere")}, [("setuptools", ("2.0", "/somewhere", "distribute"))]) + res = cross_check({"distribute": ("1.0", "/somewhere")}, [_Dependency("setuptools", "2.0", "/somewhere", "distribute")]) self.failUnlessEqual(res, []) - res = cross_check({"distribute": ("1.0", "/somewhere")}, [("setuptools", ("2.0", "/somewhere", None))]) + res = cross_check({"distribute": ("1.0", "/somewhere")}, [_Dependency("setuptools", "2.0", "/somewhere", None)]) self.failUnlessEqual(len(res), 1) self.failUnlessIn("location mismatch", res[0]) - res = cross_check({"distribute": ("1.0", "/somewhere")}, [("setuptools", ("2.0", "/somewhere_different", None))]) + res = cross_check({"distribute": ("1.0", "/somewhere")}, [_Dependency("setuptools", "2.0", "/somewhere_different", None)]) self.failUnlessEqual(len(res), 1) self.failUnlessIn("location mismatch", res[0]) - res = cross_check({"zope.interface": ("1.0", "")}, [("zope.interface", ("unknown", "", None))]) + res = cross_check({"zope.interface": ("1.0", "")}, [_Dependency("zope.interface", "unknown", "", None)]) self.failUnlessEqual(res, []) - res = cross_check({"zope.interface": ("unknown", "")}, [("zope.interface", ("unknown", "", None))]) + res = cross_check({"zope.interface": ("unknown", "")}, [_Dependency("zope.interface", "unknown", "", None)]) self.failUnlessEqual(res, []) - res = cross_check({"foo": ("1.0", "")}, [("foo", ("unknown", "", None))]) + res = cross_check({"foo": ("1.0", "")}, [_Dependency("foo", "unknown", "", None)]) self.failUnlessEqual(len(res), 1) self.failUnlessIn("could not find a version number", res[0]) - res = cross_check({"foo": ("unknown", "")}, [("foo", ("unknown", "", None))]) + res = cross_check({"foo": ("unknown", "")}, [_Dependency("foo", "unknown", "", None)]) self.failUnlessEqual(res, []) # When pkg_resources and import both find a package, there is only a warning if both # the version and the path fail to match. - res = cross_check({"foo": ("1.0", "/somewhere")}, [("foo", ("2.0", "/somewhere", None))]) + res = cross_check({"foo": ("1.0", "/somewhere")}, [_Dependency("foo", "2.0", "/somewhere", None)]) self.failUnlessEqual(res, []) - res = cross_check({"foo": ("1.0", "/somewhere")}, [("foo", ("1.0", "/somewhere_different", None))]) + res = cross_check({"foo": ("1.0", "/somewhere")}, [_Dependency("foo", "1.0", "/somewhere_different", None)]) self.failUnlessEqual(res, []) - res = cross_check({"foo": ("1.0-r123", "/somewhere")}, [("foo", ("1.0.post123", "/somewhere_different", None))]) + res = cross_check({"foo": ("1.0-r123", "/somewhere")}, [_Dependency("foo", "1.0.post123", "/somewhere_different", None)]) self.failUnlessEqual(res, []) - res = cross_check({"foo": ("1.0", "/somewhere")}, [("foo", ("2.0", "/somewhere_different", None))]) + res = cross_check({"foo": ("1.0", "/somewhere")}, [_Dependency("foo", "2.0", "/somewhere_different", None)]) self.failUnlessEqual(len(res), 1) self.assertTrue(("but version '2.0'" in res[0]) or ("but version u'2.0'" in res[0])) @@ -270,6 +309,64 @@ class T(unittest.TestCase): sys.modules["foolscap"] = None vers_and_locs, errors = get_package_versions_and_locations() - foolscap_stuffs = [stuff for (pkg, stuff) in vers_and_locs if pkg == 'foolscap'] + foolscap_stuffs = [pkg for pkg in vers_and_locs if pkg.name == 'foolscap'] self.failUnlessEqual(len(foolscap_stuffs), 1) self.failUnless([e for e in errors if "\'foolscap\' could not be imported" in e]) + + +class VersAndLocsTests(unittest.TestCase): + """ + Tests for ``_vers_and_locs_list``. + """ + def test_name_types(self): + """ + ``_vers_and_locs_list`` is a list of ``_Dependency`` instances with + ``name`` attributes which are instances of ``str``. + """ + for pkg in _vers_and_locs_list: + self.assertIsInstance(pkg.name, type(u"")) + + def test_version_types(self): + """ + ``_vers_and_locs_list`` is a list of ``_Dependency`` instances with + ``version`` attributes which are instances of ``str`` or + ``NoneType``.. + """ + for pkg in _vers_and_locs_list: + self.assertIsInstance(pkg.version, (type(u""), type(None))) + + +class GetPackageVersionsTests(unittest.TestCase): + """ + Tests for ``get_package_versions``. + """ + def test_key_types(self): + """ + Keys in the return value of ``get_package_versions`` are instances of + ``str`` + """ + for name, version in get_package_versions().items(): + self.assertIsInstance(name, type(u"")) + + def test_value_types(self): + """ + Values in the return value of ``get_package_versions`` are instances of + ``str`` or ``NoneType``. + """ + for name, version in get_package_versions().items(): + self.assertIsInstance(version, (type(u""), type(None))) + + +class GetPackageVersionsStringTests(unittest.TestCase): + """ + Tests for ``get_package_versions_string``. + """ + def test_type(self): + """ + The return value of ``get_package_versions_string`` is an instance of + ``str``. + """ + self.assertIsInstance( + get_package_versions_string(), + type(u""), + ) diff --git a/src/allmydata/version_checks.py b/src/allmydata/version_checks.py index d022055ea..ce0e07130 100644 --- a/src/allmydata/version_checks.py +++ b/src/allmydata/version_checks.py @@ -20,6 +20,8 @@ __all__ = [ "normalized_version", ] +import attr + import os, platform, re, sys, traceback, pkg_resources import six @@ -58,16 +60,32 @@ class PackagingError(EnvironmentError): """ def get_package_versions(): - return dict([(k, v) for k, (v, l, c) in _vers_and_locs_list]) + """ + :return {str: str|NoneType}: A mapping from dependency name to dependency version + for all discernable Tahoe-LAFS' dependencies. + """ + return { + dep.name: dep.version + for dep + in _vers_and_locs_list + } def get_package_versions_string(show_paths=False, debug=False): + """ + :return str: A string describing the version of all Tahoe-LAFS + dependencies. + """ + version_format = "{}: {}".format + comment_format = " [{}]".format + path_format = " ({})".format + res = [] - for p, (v, loc, comment) in _vers_and_locs_list: - info = str(p) + ": " + str(v) - if comment: - info = info + " [%s]" % str(comment) + for dep in _vers_and_locs_list: + info = version_format(dep.name, dep.version) + if dep.comment: + info = info + comment_format(dep.comment) if show_paths: - info = info + " (%s)" % str(loc) + info = info + path_format(dep.location) res.append(info) output = "\n".join(res) + "\n" @@ -119,15 +137,22 @@ def _get_error_string(errors, debug=False): return msg def _cross_check(pkg_resources_vers_and_locs, imported_vers_and_locs_list): - """This function returns a list of errors due to any failed cross-checks.""" + """ + This function returns a list of errors due to any failed cross-checks. + + :rtype: [str] + """ from ._auto_deps import not_import_versionable errors = [] not_pkg_resourceable = ['python', 'platform', __appname__.lower(), 'openssl'] - for name, (imp_ver, imp_loc, imp_comment) in imported_vers_and_locs_list: - name = name.lower() + for dep in imported_vers_and_locs_list: + name = dep.name.lower() + imp_ver = dep.version + imp_loc = dep.location + imp_comment = dep.comment if name not in not_pkg_resourceable: if name not in pkg_resources_vers_and_locs: if name == "setuptools" and "distribute" in pkg_resources_vers_and_locs: @@ -230,14 +255,58 @@ def _get_platform(): else: return platform.platform() + +def _ensure_text_optional(o): + """ + Convert a value to the maybe-Future-ized native string type or pass through + ``None`` unchanged. + + :type o: NoneType|bytes|str + + :rtype: NoneType|str + """ + if o is None: + return None + return six.ensure_text(o) + + +@attr.s +class _Dependency(object): + """ + A direct or indirect Tahoe-LAFS dependency. + + :ivar name: The name of this dependency. + :ivar version: If known, a string giving the version of this dependency. + :ivar location: If known, a string giving the path to this dependency. + :ivar comment: If relevant, some additional free-form information. + """ + name = attr.ib( + converter=six.ensure_text, + validator=attr.validators.instance_of(str), + ) + version = attr.ib( + converter=_ensure_text_optional, + validator=attr.validators.optional(attr.validators.instance_of(str)), + ) + location = attr.ib( + converter=_ensure_text_optional, + validator=attr.validators.optional(attr.validators.instance_of(str)), + ) + comment = attr.ib() + + def _get_package_versions_and_locations(): + """ + Look up information about the software available to this process. + + :return: A two tuple. The first element is a list of ``_Dependency`` + instances. The second element is like the value returned by + ``_cross_check``. + """ import warnings from ._auto_deps import package_imports, global_deprecation_messages, deprecation_messages, \ runtime_warning_messages, warning_imports, ignorable - def package_dir(srcfile): - return os.path.dirname(os.path.dirname(os.path.normcase(os.path.realpath(srcfile)))) - # pkg_resources.require returns the distribution that pkg_resources attempted to put # on sys.path, which can differ from the one that we actually import due to #1258, # or any other bug that causes sys.path to be set up incorrectly. Therefore we @@ -263,15 +332,67 @@ def _get_package_versions_and_locations(): for _ in runtime_warning_messages + deprecation_messages: warnings.filters.pop() - packages = [] - pkg_resources_vers_and_locs = dict() + pkg_resources_vers_and_locs = _compute_pkg_resources_vers_and_locs(_INSTALL_REQUIRES) + packages = list(_compute_imported_packages( + [(__appname__, 'allmydata')] + package_imports, + pkg_resources_vers_and_locs, + )) + + cross_check_errors = [] + + if len(pkg_resources_vers_and_locs) > 0: + imported_packages = set(dep.name.lower() for dep in packages) + extra_packages = [] + + for pr_name, (pr_ver, pr_loc) in pkg_resources_vers_and_locs.items(): + if pr_name not in imported_packages and pr_name not in ignorable: + extra_packages.append( + _Dependency( + pr_name, + pr_ver, + pr_loc, + "according to pkg_resources", + ), + ) + + cross_check_errors = _cross_check(pkg_resources_vers_and_locs, packages) + packages += extra_packages + + return packages, cross_check_errors + + +def _compute_pkg_resources_vers_and_locs(requires): + """ + Get the ``pkg_resources`` idea of the dependencies for all of the given + requirements. + + If the execution context is a frozen interpreter, just return an empty + dictionary. + + :param [str] requires: Information about the dependencies of these + requirements strings will be looked up and returned. + + :return {str: (str, str)}: A mapping from dependency name to a two-tuple + of dependency version and location. + """ if not hasattr(sys, 'frozen'): - pkg_resources_vers_and_locs = { + return { p.project_name.lower(): (str(p.version), p.location) for p - in pkg_resources.require(_INSTALL_REQUIRES) + in pkg_resources.require(requires) } + return {} + + +def _compute_imported_packages(packages, pkg_resources_vers_and_locs): + """ + Get the import system's idea of all of the given packages. + + :param packages: + """ + def package_dir(srcfile): + return os.path.dirname(os.path.dirname(os.path.normcase(os.path.realpath(srcfile)))) def get_version(module): if hasattr(module, '__version__'): @@ -285,7 +406,7 @@ def _get_package_versions_and_locations(): else: return 'unknown' - for pkgname, modulename in [(__appname__, 'allmydata')] + package_imports: + for pkgname, modulename in packages: if modulename: try: __import__(modulename) @@ -293,7 +414,12 @@ def _get_package_versions_and_locations(): except (ImportError, SyntaxError): etype, emsg, etrace = sys.exc_info() trace_info = (etype, str(emsg), ([None] + traceback.extract_tb(etrace))[-1]) - packages.append( (pkgname, (None, None, trace_info)) ) + yield _Dependency( + pkgname, + None, + None, + trace_info, + ) else: comment = None if pkgname == __appname__: @@ -307,28 +433,31 @@ def _get_package_versions_and_locations(): (pr_ver, pr_loc) = pkg_resources_vers_and_locs[pkgname] if loc == os.path.normcase(os.path.realpath(pr_loc)): ver = pr_ver - packages.append( (pkgname, (ver, loc, comment)) ) + yield _Dependency( + pkgname, + ver, + loc, + comment, + ) elif pkgname == 'python': - packages.append( (pkgname, (platform.python_version(), sys.executable, None)) ) + yield _Dependency( + pkgname, + platform.python_version(), + sys.executable, + None, + ) elif pkgname == 'platform': - packages.append( (pkgname, (_get_platform(), None, None)) ) + yield _Dependency( + pkgname, + _get_platform(), + None, + None, + ) elif pkgname == 'OpenSSL': - packages.append( (pkgname, _get_openssl_version()) ) - - cross_check_errors = [] - - if len(pkg_resources_vers_and_locs) > 0: - imported_packages = set([p.lower() for (p, _) in packages]) - extra_packages = [] - - for pr_name, (pr_ver, pr_loc) in pkg_resources_vers_and_locs.items(): - if pr_name not in imported_packages and pr_name not in ignorable: - extra_packages.append( (pr_name, (pr_ver, pr_loc, "according to pkg_resources")) ) - - cross_check_errors = _cross_check(pkg_resources_vers_and_locs, packages) - packages += extra_packages - - return packages, cross_check_errors + yield _Dependency( + pkgname, + *_get_openssl_version() + ) _vers_and_locs_list, _cross_check_errors = _get_package_versions_and_locations()