From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id wKluMSNE+F8YEQAA0tVLHw (envelope-from ) for ; Fri, 08 Jan 2021 11:38:11 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id BGVJLSNE+F+sNAAA1q6Kng (envelope-from ) for ; Fri, 08 Jan 2021 11:38:11 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id C90D59402D4 for ; Fri, 8 Jan 2021 11:38:10 +0000 (UTC) Received: from localhost ([::1]:56966 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kxq5p-00013d-O9 for larch@yhetil.org; Fri, 08 Jan 2021 06:38:09 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:43016) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kxq5i-00013M-Nk for guix-patches@gnu.org; Fri, 08 Jan 2021 06:38:02 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:37801) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kxq5i-0004bS-GH for guix-patches@gnu.org; Fri, 08 Jan 2021 06:38:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kxq5i-0002Hr-DI for guix-patches@gnu.org; Fri, 08 Jan 2021 06:38:02 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#45712] [PATCHES] Improve Python package quality Resent-From: "Hartmut Goebel" Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Fri, 08 Jan 2021 11:38:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 45712 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Lars-Dominik Braun Received: via spool by 45712-submit@debbugs.gnu.org id=B45712.16101058358734 (code B ref 45712); Fri, 08 Jan 2021 11:38:02 +0000 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" 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-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: 45712@debbugs.gnu.org Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN X-Migadu-Spam-Score: -2.35 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Queue-Id: C90D59402D4 X-Spam-Score: -2.35 X-Migadu-Scanner: scn0.migadu.com X-TUID: iE+ackY0JaKs 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.