From debbugs-submit-bounces@debbugs.gnu.org Fri Jan 08 06:37:14 2021 Received: (at 45712) by debbugs.gnu.org; 8 Jan 2021 11:37:15 +0000 Received: from localhost ([127.0.0.1]:49347 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kxq4w-0002Gn-F0 for submit@debbugs.gnu.org; Fri, 08 Jan 2021 06:37:14 -0500 Received: from mail-out.m-online.net ([212.18.0.9]:55122) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kxq4t-0002Ge-Uv for 45712@debbugs.gnu.org; Fri, 08 Jan 2021 06:37:13 -0500 Received: from frontend01.mail.m-online.net (unknown [192.168.8.182]) by mail-out.m-online.net (Postfix) with ESMTP id 4DC1Kf3QtQz1qs3Y; Fri, 8 Jan 2021 12:37:10 +0100 (CET) Received: from localhost (dynscan1.mnet-online.de [192.168.6.70]) by mail.m-online.net (Postfix) with ESMTP id 4DC1Kf34Khz1tSQH; Fri, 8 Jan 2021 12:37:10 +0100 (CET) X-Virus-Scanned: amavisd-new at mnet-online.de Received: from mail.mnet-online.de ([192.168.8.182]) by localhost (dynscan1.mail.m-online.net [192.168.6.70]) (amavisd-new, port 10024) with ESMTP id TDYsPToA6-Wj; Fri, 8 Jan 2021 12:37:09 +0100 (CET) Received: from hermia.goebel-consult.de (ppp-188-174-49-21.dynamic.mnet-online.de [188.174.49.21]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.mnet-online.de (Postfix) with ESMTPS; Fri, 8 Jan 2021 12:37:09 +0100 (CET) Received: from lenashee.goebel-consult.de (lenashee.goebel-consult.de [192.168.110.2]) by hermia.goebel-consult.de (Postfix) with SMTP id B4E566012F; Fri, 8 Jan 2021 12:37:05 +0100 (CET) Received: by lenashee.goebel-consult.de (sSMTP sendmail emulation); Fri, 08 Jan 2021 12:37:05 +0100 From: "Hartmut Goebel" To: Lars-Dominik Braun Subject: Re: [bug#45712] [PATCHES] Improve Python package quality References: Date: Fri, 08 Jan 2021 12:37:05 +0100 In-Reply-To: (Lars-Dominik Braun's message of "Thu, 7 Jan 2021 14:26:20 +0100") Message-ID: <87mtxj1wym.fsf@lenashee.goebel-consult.de> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: -0.7 (/) X-Debbugs-Envelope-To: 45712 Cc: 45712@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.7 (-) Hi Lars, thanks for the patch set. Please please find some comments. (I did not check all changes to the packages, assuming you did it right :-) > +(define validate-script > + "from __future__ import print_function # Python 2 support. Please but this int a new line - makeing it easier to copy & paste. (Leading emptry lines doe nor effect "from __future__ import"). > +import pkg_resources, sys, importlib, traceback > +try: > + from importlib.machinery import PathFinder > +except ImportError: > + PathFinder =3D None > +ws =3D pkg_resources.find_distributions(sys.argv[1]) > +for dist in ws: > + print('validating', repr(dist.project_name), dist.location) > + try: > + print('...checking requirements', end=3D': ') This looks very uncommon (and make my mind hooking on it). Please use this, which is more common and less mindbogling ;-) print('...checking requirements: ', end=3D'') > + req =3D str(dist.as_requirement()) > + # dist.activate() is not enough to actually check requirements, = we have to > + # .require() it. > + pkg_resources.require(req) Any reason for converting the req into a string first? IC, "`requirements` must be a string". So I'd move the "str()" to the function call: req =3D dist.as_requirement() # dist.activate() is not enough to actually check requirements, # we have to .require() it. pkg_resources.require(str(req)) > + print('OK') > + except Exception as e: > + print('ERROR:', req, e) > + ret =3D 1 > + continue > + # Try to load entry points of console scripts too, making sure they = work. They > + # should be removed if they don=E2=80=99t. Other groups may not be s= afe, as they can > + # depend on optional packages. > + for group, v in dist.get_entry_map().items(): > + if group not in {'console_scripts', }: if group not in {'console_scripts', 'gui_scripts'}: Why not gui-scripts? If not adding gui-scripts, just use "if group !=3D 'concolse_scrips':" > + continue > + for name, ep in v.items(): > + try: > + print('...trying to load endpoint', group, name, end=3D':= ') Here it is fine ;-) > + # And finally try to load top level modules. This should not have any > + # side-effects. Does it make sence to try loading the top-level modules *after* the the entry-point? Chances are very high that the entry-points implicitly test the top-level modules already. IMHO it would make more sense to first try the top-level modules, and even stop processing if this fails. > + for name in dist.get_metadata_lines('top_level.txt'): > + # Only available on Python 3. > + if PathFinder and PathFinder.find_spec(name) is None: > + # Ignore unavailable modules. Cannot use Please add a short explanation why there can be unavailable top-level modules. > + (add-after 'check 'validate-loadable validate-loadable) While not having antoher idea, I'm not happy with this name. "loadable" is not easy to get. (See also below, where this term is used in commit mess= age.) top-level-modules-are-importable? > +(define python-dummy-ok AFAIKS the packages only differ by some requirements. So I suggest using a function to return the packages. This makes it easier to spot the actull differences. > + (replace 'unpack > + (lambda _ > + (mkdir-p "src") > + (chdir "src") > + (mkdir-p "dummy") (mkdir-p "src/dummy") (chdir "src") > +setup( > + name=3D'dummy-fail-import', > + version=3D'0.1', > + packages=3D['dummy'], > + ) I would strip this down (version is not required AFAIK) and put it one line (her assuming you use (format) for creating the code within a function: setup(name=3D'~a', packages=3D~a, install_requires=3D~a) > Subject: [PATCH 03/15] gnu: python-pytest-xdist: Add missing input, relax > pytest requirement. > + (add-after 'unpack 'patch > + (lambda* (#:key inputs #:allow-other-keys) Arguments are not used? > + ;; Relax pytest requirement. > + (substitute* "setup.py" > + (("pytest>=3D6\\.0\\.0") "pytest")) > + #t)) Any reason for relaxing this? Why not use python-pytest-6 as input? > Subject: [PATCH 04/15] gnu: python-fixtures-bootstrap: Do not apply loada= ble > check. Please rephrase into something like Do not validate loadability Do not validate whetehr packag is loadable =E2=80=A6 > Subject: [PATCH 05/15] gnu: python-pytest-pep8: Fix package. > - `(#:tests? #f)) ; Fails with recent pytest and pep8. See upstream i= ssues #8 and #12. > + `(#:tests? #t ; Fails with recent pytest and pep8. See upstream iss= ues #8 and #12. Dosn't this change fix the checks? So this comment would be obsoltes and "#:tests #t" can be removed. > Subject: [PATCH 06/15] gnu: python-pyfakefs: Disable unreliable test. > - (add-after 'check 'check-pytest-plugin > + (replace 'check > (lambda _ > - (invoke > - "python" "-m" "pytest" > - "pyfakefs/pytest_tests/pytest_plugin_test.py") > - #t))))) > + (invoke "pytest" > + "pyfakefs/tests" > + ;; The default test suite does not run these extra tests. > + ;"pyfakefs/pytest_tests/pytest_plugin_test.py" > + ;; atime difference is larger than expected. > + "-k" "not test_copy_real_file")))))) I suggest keeping the old way, as this is will automatically update to upstream changes.