[bug#45712,PATCHES] Improve Python package quality

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

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.

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---

Hi!

Lars-Dominik Braun <lars@6xq.net> writes:

> Hi,
>
> as announced in
> https://lists.gnu.org/archive/html/guix-devel/2021-01/msg00021.html Ive
> 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
> Im 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.

Thanks for the initiative!  It looks good, on first sight.  One question
I have is this: does it rely on the Python package having been built
with setuptools/distutils?  The Python world is moving toward a
plurality of PEP 517 compliant build systems; any idea if the checker
will continue working for these new packages?

We have currently only one such package in our collection, on
core-updates.  It's python-isort, added with commit
812a2931de553d12c01b0a4d53d03613b09adaaf on the core-updates branch.

Thank you,

Maxim

Hi again,

[...]

>>From cf9ae80b59e86a60c27734c8cc27637757490d70 Mon Sep 17 00:00:00 2001
> From: Lars-Dominik Braun <lars@6xq.net>
> Date: Thu, 7 Jan 2021 13:25:56 +0100
> Subject: [PATCH 02/15] gnu: pytest@6: Add missing propagated-input.
>
> * gnu/packages/check.scm (python-pytest-6) [native-inputs]: Remove
> python-iniconfig.
> [propagated-inputs]: Move it here.
> ---
>  gnu/packages/check.scm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/gnu/packages/check.scm b/gnu/packages/check.scm
> index 1300f9e1a6..9d1e0b8173 100644
> --- a/gnu/packages/check.scm
> +++ b/gnu/packages/check.scm
> @@ -964,13 +964,13 @@ and many external plugins.")
>      (propagated-inputs
>       (append (alist-delete "python-py"
>                             (package-propagated-inputs python-pytest))
> -             `(("python-py" ,python-py-next))))
> +             `(("python-py" ,python-py-next)
> +               ("python-iniconfig" ,python-iniconfig))))
>      (native-inputs
>       (append (alist-delete "python-pytest"
>                             (package-native-inputs python-pytest))
>               `(("python-pytest" ,python-pytest-6-bootstrap)
> -               ("python-toml" ,python-toml)
> -               ("python-iniconfig" ,python-iniconfig))))))
> +               ("python-toml" ,python-toml))))))
>  
>  ;; Pytest 4.x are the last versions that support Python 2.
>  (define-public python2-pytest

I hadn't seen this patch but I fixed that problem on core-updates with
be7061cea30b59676fc473d6bd4a56a0f2fbd7cf on the 14 of January.  Note
that python-pytest-6 became just 'python-pytest' there.

Hi Maxim,

> Thanks for the initiative!  It looks good, on first sight.  One question
> I have is this: does it rely on the Python package having been built
> with setuptools/distutils?  The Python world is moving toward a
> plurality of PEP 517 compliant build systems; any idea if the checker
> will continue working for these new packages?
yes and no. pkg_resources is part of setuptools, so my patch depends on
setuptools. But I think dependencies are specified in a standardized
format[1], which any tool can (and should?) write.

top_level.txt seems to be part of python eggs, which are deprecated[2].
I guess we could just scan for top-level directories under
site-packages, which have a __init__.py and try to load them.

I can’t find any standard for the entry points, so I’m guessing they’re
setuptools-specific too.

My assumption is that if no metadata is found the checks are just
skipped and nothing bad happens, but we may have to verify this.

As for PEP 517, I’m working on updating python-build-system, see
https://lists.gnu.org/archive/html/guix-devel/2021-01/msg00294.html
I’ll send an updated patch to guix-patches@ as soon as I have the 'check
phase working.

Cheers,
Lars

[1] https://packaging.python.org/specifications/core-metadata/#requires-dist-multiple-use
[2] https://setuptools.readthedocs.io/en/latest/deprecated/python_eggs.html#top-level-txt-conflict-management-metadata

Lars-Dominik Braun <lars@6xq.net> writes:

[...]

>>From 9bc53a8e9706440668ec70d88db1ebc7d5e2c71d Mon Sep 17 00:00:00 2001
> From: Lars-Dominik Braun <lars@6xq.net>
> Date: Thu, 7 Jan 2021 13:27:55 +0100
> Subject: [PATCH 03/15] gnu: python-pytest-xdist: Add missing input, relax
>  pytest requirement.
>
> * gnu/packages/check.scm: (python-pytest-xdist)
> [arguments]: Relax pytest version requirements.
> [propagated-inputs]: Add python-pytest-forked.
> ---
>  gnu/packages/check.scm | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/gnu/packages/check.scm b/gnu/packages/check.scm
> index 9d1e0b8173..32a1a2d6a3 100644
> --- a/gnu/packages/check.scm
> +++ b/gnu/packages/check.scm
> @@ -1216,20 +1216,27 @@ same arguments.")
>             #t))))
>      (build-system python-build-system)
>      (arguments
> -     '(#:tests? #f)) ;FIXME: Some tests are failing.
> -       ;; #:phases
> -       ;; (modify-phases %standard-phases
> -       ;;   (delete 'check)
> -       ;;   (add-after 'install 'check
> -       ;;     (lambda* (#:key inputs outputs #:allow-other-keys)
> -       ;;       (add-installed-pythonpath inputs outputs)
> -       ;;       (zero? (system* "py.test" "-v")))))
> +     '(#:tests? #f ; Lots of tests fail.
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'patch
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             ;; Relax pytest requirement.
> +             (substitute* "setup.py"
> +               (("pytest>=6\\.0\\.0") "pytest"))

I'm not sure what the above was caused by, but its'
been fixed on core-updates (the version of pytest as per its metadata is
reported correctly there).