diff mbox series

[bug#45712,PATCHES] Improve Python package quality

Message ID X/cL/EAsHw0UNXXy@noor.fritz.box
State New
Headers show
Series [bug#45712,PATCHES] Improve Python package quality | expand

Checks

Context Check Description
cbaines/submitting builds success
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Lars-Dominik Braun Jan. 7, 2021, 1:26 p.m. UTC
Hi,

as announced in
https://lists.gnu.org/archive/html/guix-devel/2021-01/msg00021.html I’ve
been working on adding an additional phase to Python packages to check
whether they actually work. I cleaned up my patch, added tests and now
I’m pretty confident it works as expected. The first patch in this
series adds this phase, while the other ones fix build failures caused
by it. All of this should go to core-updates (or a separate wip-*
branch?), since it causes a massive number of rebuilds.

You can also pull my git repo at https://github.com/PromyLOPh/guix.git
branch work-python-importcheck.

Cheers,
Lars

Comments

Hartmut Goebel Jan. 8, 2021, 11:37 a.m. UTC | #1
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 = None



> +ws = pkg_resources.find_distributions(sys.argv[1])
> +for dist in ws:
> +    print('validating', repr(dist.project_name), dist.location)
> +    try:
> +        print('...checking requirements', end=': ')

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='')


> +        req = 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 = 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 = 1
> +        continue
> +    # Try to load entry points of console scripts too, making sure they work. They
> +    # should be removed if they don’t. Other groups may not be safe, 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 != 'concolse_scrips':"

> +           continue
> +       for name, ep in v.items():
> +           try:
> +               print('...trying to load endpoint', group, name, end=': ')

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 message.)

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='dummy-fail-import',
> +     version='0.1',
> +     packages=['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='~a', packages=~a, install_requires=~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>=6\\.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 loadable
>  check.

Please rephrase into something like

Do not validate loadability
Do not validate whetehr packag is loadable
…


> Subject: [PATCH 05/15] gnu: python-pytest-pep8: Fix package.

> -     `(#:tests? #f)) ; Fails with recent pytest and pep8. See upstream issues #8 and #12.
> +     `(#:tests? #t ; Fails with recent pytest and pep8. See upstream issues #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.
Ricardo Wurmus Jan. 8, 2021, 12:19 p.m. UTC | #2
Hartmut Goebel <hartmut@goebel-consult.de> writes:

> 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").

You can also escape the line break:

--8<---------------cut here---------------start------------->8---
(define validate-script "\
from __future__ import print_function # Python 2 support.
…")
--8<---------------cut here---------------end--------------->8---
diff mbox series

Patch

From ecc8eebadbeb8c75ac5025d7bce581423d4d1894 Mon Sep 17 00:00:00 2001
From: Lars-Dominik Braun <lars@6xq.net>
Date: Thu, 7 Jan 2021 14:17:20 +0100
Subject: [PATCH 15/15] gnu: python-traceback2: Add missing dependency.

* gnu/packages/python-xyz.scm (python-traceback2) [propagated-inputs]:
Add python-six.
---
 gnu/packages/python-xyz.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/python-xyz.scm b/gnu/packages/python-xyz.scm
index e78016221f..1df9807626 100644
--- a/gnu/packages/python-xyz.scm
+++ b/gnu/packages/python-xyz.scm
@@ -17266,7 +17266,8 @@  lines are read from a single file.")
     (native-inputs
      `(("python-pbr" ,python-pbr-minimal)))
     (propagated-inputs
-      `(("python-linecache2" ,python-linecache2)))
+      `(("python-linecache2" ,python-linecache2)
+        ("python-six" ,python-six)))
     (home-page
       "https://github.com/testing-cabal/traceback2")
     (synopsis "Backports of the traceback module")
-- 
2.26.2